add the function to edit register value #199

QuocDoBV · 2022-08-15T11:06:21Z

Currently, we only can edit local value. We cannot edit register value on variable view. So, we need to add the function to do it.

eclipse-cdt-bot · 2022-08-15T11:11:02Z

Can one of the admins verify this patch?

jonahgraham · 2022-08-15T11:22:37Z

Hi @QuocDoBV thanks for they PR. Unfortunately it looks like this version has some problems as the change is in the wrong file making the diff very large. Also the ECA check fails. Perhaps @quyettrinhrvc can advise on how to resolve ECA issue.

QuocDoBV · 2022-08-15T11:26:32Z

Hi @jonahgraham thanks a lot for your information.

QuocDoBV · 2022-08-16T03:43:31Z

Hi @jonahgraham,
After @quyettrinhrvc support, ECA issue has been resolved.
Currently, we can only edit local value on Variable view. When we edit register value, Some errors will display. So, I have changed code to edit register value can work.
Could you please review it help me?
Thank a lot.

jonahgraham · 2022-08-16T12:23:18Z

Hi, this looks good - we need some tests. @quyettrinhrvc wrote the register read tests here:

cdt-gdb-adapter/src/integration-tests/var.spec.ts

Lines 127 to 129 in 7ef1e92

    
           it('can read registers in a program', async function () { 
        
               // read the registers 
        
               const vr = scope.scopes.body.scopes[1].variablesReference;

The equivalent for setting variables is a little further down the file:

cdt-gdb-adapter/src/integration-tests/var.spec.ts

Lines 189 to 199 in 7ef1e92

    
           // set the variables to something different 
        
           await dc.setVariableRequest({ 
        
               name: 'x', 
        
               value: '25', 
        
               variablesReference: childVR, 
        
           }); 
        
           await dc.setVariableRequest({ 
        
               name: 'y', 
        
               value: '10', 
        
               variablesReference: childVR, 
        
           });

QuocDoBV · 2022-08-18T01:13:34Z

Hi @jonahgraham,
Sorry for my reply late.
I have tried to run integration tests which cloned from https://github.com/quyettrinhrvc/cdt-gdb-adapter to make sure that I have a correct environment. And, the result is that all tests are passed. You can check the result here integration.zip. However, when I run integration tests in latest version from https://github.com/eclipse-cdt-cloud/cdt-gdb-adapter (There are some tests added to check for multiple threads), some problems occur tests that involve multiple threads . You can check the problem at integration.zip. And, when I run the tests from https://github.com/QuocDoBV/cdt-gdb-adapter(my pull request) there are some problems like when running with cdt-gdb-adapter latest version . You can check the result here integration.zip. So I thinks that the problem when I run integration tests from my pull request is not related to my changes.
Could you please help me check and confirm it?
Could you tell me that do I need to add more integration tests or just run the current integration tests and confirm it?
If yes, could you please give me some advices? I'm new here. So sorry I don't have much experience in this.
Thanks a lot.

jonahgraham · 2022-08-21T21:23:59Z

I don't see any failures in the integration.zip file you attached. I do see skipped tests, which is expected, some tests cannot be run on every machine. Rather than attach the machine readable xml file, you can provide the failed tests from the stdout, e.g. like this:

  Suite duration: 0.394 s, Tests: 1

  0 passing (405ms)
  1 failing

  1) stop
       handles segv:

      AssertionError: expected 'SIGSEGV' to equal 'SI2GSEGV'
      + expected - actual

      -SIGSEGV
      +SI2GSEGV
      
      at /scratch/debug/git/cdt-gdb-adapter/dist/integration-tests/stop.spec.js:33:57
      at Generator.next (<anonymous>)
      at fulfilled (dist/integration-tests/stop.spec.js:5:58)
      at processTicksAndRejections (internal/process/task_queues.js:95:5)

Could you tell me that do I need to add more integration tests or just run the current integration tests and confirm it?

You need new tests to cover your new functionality.

jonahgraham · 2022-08-21T21:24:53Z

If yes, could you please give me some advices? I'm new here. So sorry I don't have much experience in this.

Please refer to this comment for a starting point on writing tests for this use case.

QuocDoBV · 2022-08-22T12:42:03Z

Hi @jonahgraham,
Thanks a lot for your support!
I have created some tests to test for my new functionality. Could you take a look and review it help me?
This is the test reports EditRegisterValue_Report_22082022.zip
Thank you so much!

jonahgraham

Thanks @QuocDoBV for writing the tests. That makes it possible for me to properly review the change now. Please find my detailed line comments. Feel free to ask any follow up questions as needed.

src/integration-tests/var.spec.ts

src/mi/var.ts

src/GDBDebugSession.ts

QuocDoBV · 2022-08-24T08:09:53Z

Hi @jonahgraham ,
I updated follow your suggestion. Could you please help me review again.
This is the integration tests report.
testResult_24_08.zip

src/GDBDebugSession.ts

src/integration-tests/utils.ts

src/GDBDebugSession.ts

src/integration-tests/var.spec.ts

src/GDBDebugSession.ts

src/integration-tests/var.spec.ts

QuocDoBV · 2022-09-07T12:44:27Z

Dear @jonahgraham,
Could you please help me review again?
All integration tests passed.
This is the report file integration_09072022_windows.zip

QuocDoBV · 2022-09-07T12:47:16Z

Dear @jonahgraham,
I have also run integration tests on Linux OS.
This is the report file
integration_09072022_Linux.zip

jonahgraham · 2022-09-07T12:50:56Z

Dear @jonahgraham, I have also run integration tests on Linux OS. This is the report file integration_09072022_Linux.zip

Hi @QuocDoBV - as mentioned before please don't bother attaching xml files. The human readable output of the test is much more useful. Also if all the tests are passing I don't need to see anything, you can just tell me they all passed.

jonahgraham

This is mostly fine - just needs some polishing and the missing test.

src/GDBDebugSession.ts

src/integration-tests/utils.ts

src/integration-tests/var.spec.ts

src/GDBDebugSession.ts

QuocDoBV · 2022-09-08T03:31:15Z

Dear @jonahgraham,
Could you please help me review again?
This is the log file in my environment
log08092022.zip
Thank you so much for your helpful tips.

jonahgraham

I will do the requested change.

src/GDBDebugSession.ts

jonahgraham

There is still a serious bug in this code. I am sorry I didn't see it before now. Consider the use case where a user modified a register which has the same name as a local variable (e.g. rax).

Here is a log of the problem with my comments:


# VScode requests scopes, as you can see ref 1000 is for Local variables and 1001 is for registers
From client: scopes({"frameId":1000})
To client: {"seq":0,"type":"response","request_seq":17,"command":"scopes","success":true,"body":{"scopes":[{"name":"Local","variablesReference":1000,"expensive":false},{"name":"Registers","variablesReference":1001,"expensive":true}]}}

# Here VScode requests variables for reg 1000 (locals)...
From client: variables({"variablesReference":1000})
GDB command: 25 -stack-info-depth 100
GDB result: 25 done,depth="1"
GDB command: 26 -stack-list-variables --thread 1 --simple-values
GDB result: 26 done,variables=[{name="rax",type="int",value="1"}]
# ... and the adapter creates var4 for accessing local variable rax
GDB command: 27 -var-create --thread 1 --frame 0 - * "rax"
GDB result: 27 done,name="var4",numchild="0",value="1",type="int",thread-id="1",has_more="0"

# Here VScode resuests the registers
# eliding GDB responses for getting register names and values because they are very long
From client: variables({"variablesReference":1001})
GDB command: 28 -data-list-register-names --frame 0 --thread 1
GDB command: 29 -data-list-register-values --frame 0 --thread 1 x
To client: {"seq":0,"type":"response","request_seq":19,"command":"variables","success":true,"body":{"variables":[{"name":"rax","evaluateName":"$rax","value":"0x1","variablesReference":0}, # -- snip

# Here VSCode is trying to set the register rax
From client: setVariable({"variablesReference":1001,"name":"rax","value":"1234"})
GDB command: 30 -stack-info-depth 100
GDB result: 30 done,depth="1"

# But it is var4 that is assigned - which is the varobj for the local variable
GDB command: 31 -var-assign var4 1234
GDB result: 31 done,value="1234"

@QuocDoBV Please resolve this (and while at it you can add the comments covering the design decision to not delete the varobjs).

jonahgraham · 2022-09-08T13:05:26Z

src/GDBDebugSession.ts

+                    frame.frameId,
+                    frame.threadId,
+                    depth,
+                    args.name,


This varobj is stored in the varManager in a way that it indistinguishable from a local variable of the same name.

I have fixed it.

jonahgraham · 2022-09-08T13:18:30Z

There is still a serious bug in this code. I am sorry I didn't see it before now. Consider the use case where a user modified a register which has the same name as a local variable (e.g. rax).

PS Please create a test case for this usage scenario to make sure that it doesn't regress later.

QuocDoBV · 2022-09-12T04:26:28Z

There is still a serious bug in this code. I am sorry I didn't see it before now. Consider the use case where a user modified a register which has the same name as a local variable (e.g. rax).

PS Please create a test case for this usage scenario to make sure that it doesn't regress later.

I have created test cases to cover it.

QuocDoBV · 2022-09-12T04:29:01Z

Dear @jonahgraham,
I have fixed issue related to editing register which has the same name with local variable.
Could you please help me review it again.
You can check the log file to have more details log09122022.zip

jonahgraham · 2022-09-12T17:20:54Z

Thank you @QuocDoBV for the new code and for good new tests to keep it working into the future too.

add the function to edit register value

77b9d4e

QuocDoBV added 5 commits August 16, 2022 10:28

Add the function to edit register value

02db0f3

Add the function to edit register value

c162c61

Delete GDBDebugSession.ts

a350390

redundant file

17bde75

remove redundant code

3a5ff40

QuocDoBV added 2 commits August 22, 2022 18:51

Add integration tests for editing registers value

6b63e6b

Add integration tests for editing registers value

f03b4d5

fix typo

389084e

jonahgraham mentioned this pull request Aug 22, 2022

Test return values of setVariableRequest #200

Merged

jonahgraham requested changes Aug 22, 2022

View reviewed changes

QuocDoBV added 5 commits August 24, 2022 12:06

rebase code

4a875e5

Merge branch 'eclipse-cdt-cloud:main' into main

4803657

Change to a new solution

3af0b25

add the sendVarSetFormatToHex function

d8d846d

Update integration tests

80a87e2

QuocDoBV requested a review from jonahgraham August 24, 2022 08:23

jonahgraham requested changes Sep 6, 2022

View reviewed changes

QuocDoBV added 2 commits September 7, 2022 18:37

Create var objects for the registers

2e27c9c

remove test cases related to register "rn"

6fcd30e

QuocDoBV requested a review from jonahgraham September 7, 2022 12:47

jonahgraham requested changes Sep 7, 2022

View reviewed changes

QuocDoBV added 4 commits September 8, 2022 07:47

update format

9644018

Test return values of setVariableRequest

09531a7

enhance source code

1f1d23d

enhance source

526a69f

thamho0902 mentioned this pull request Sep 8, 2022

Fix issue related to cannot set breakpoints #201

Merged

QuocDoBV requested a review from jonahgraham September 8, 2022 11:40

jonahgraham requested changes Sep 8, 2022

View reviewed changes

src/GDBDebugSession.ts Show resolved Hide resolved

jonahgraham requested changes Sep 8, 2022

View reviewed changes

QuocDoBV added 4 commits September 12, 2022 08:00

add more parameter varType to varobj

b8ed4d8

add a variable which has the same name register

1afaa50

check register has same name with local variable

c56287f

issue when edit register have same name local

f6b22e7

QuocDoBV requested a review from jonahgraham September 12, 2022 04:29

jonahgraham merged commit 95d05ed into eclipse-cdt-cloud:main Sep 12, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add the function to edit register value #199

add the function to edit register value #199

QuocDoBV commented Aug 15, 2022 •

edited

Loading

eclipse-cdt-bot commented Aug 15, 2022

jonahgraham commented Aug 15, 2022

QuocDoBV commented Aug 15, 2022 •

edited

Loading

QuocDoBV commented Aug 16, 2022

jonahgraham commented Aug 16, 2022

QuocDoBV commented Aug 18, 2022 •

edited

Loading

jonahgraham commented Aug 21, 2022

jonahgraham commented Aug 21, 2022

QuocDoBV commented Aug 22, 2022

jonahgraham left a comment

QuocDoBV commented Aug 24, 2022 •

edited

Loading

QuocDoBV commented Sep 7, 2022

QuocDoBV commented Sep 7, 2022

jonahgraham commented Sep 7, 2022

jonahgraham left a comment

QuocDoBV commented Sep 8, 2022

jonahgraham left a comment

jonahgraham left a comment

jonahgraham Sep 8, 2022

QuocDoBV Sep 12, 2022

jonahgraham commented Sep 8, 2022

QuocDoBV commented Sep 12, 2022

QuocDoBV commented Sep 12, 2022

jonahgraham commented Sep 12, 2022

add the function to edit register value #199

add the function to edit register value #199

Conversation

QuocDoBV commented Aug 15, 2022 • edited Loading

eclipse-cdt-bot commented Aug 15, 2022

jonahgraham commented Aug 15, 2022

QuocDoBV commented Aug 15, 2022 • edited Loading

QuocDoBV commented Aug 16, 2022

jonahgraham commented Aug 16, 2022

QuocDoBV commented Aug 18, 2022 • edited Loading

jonahgraham commented Aug 21, 2022

jonahgraham commented Aug 21, 2022

QuocDoBV commented Aug 22, 2022

jonahgraham left a comment

Choose a reason for hiding this comment

QuocDoBV commented Aug 24, 2022 • edited Loading

QuocDoBV commented Sep 7, 2022

QuocDoBV commented Sep 7, 2022

jonahgraham commented Sep 7, 2022

jonahgraham left a comment

Choose a reason for hiding this comment

QuocDoBV commented Sep 8, 2022

jonahgraham left a comment

Choose a reason for hiding this comment

jonahgraham left a comment

Choose a reason for hiding this comment

jonahgraham Sep 8, 2022

Choose a reason for hiding this comment

QuocDoBV Sep 12, 2022

Choose a reason for hiding this comment

jonahgraham commented Sep 8, 2022

QuocDoBV commented Sep 12, 2022

QuocDoBV commented Sep 12, 2022

jonahgraham commented Sep 12, 2022

QuocDoBV commented Aug 15, 2022 •

edited

Loading

QuocDoBV commented Aug 15, 2022 •

edited

Loading

QuocDoBV commented Aug 18, 2022 •

edited

Loading

QuocDoBV commented Aug 24, 2022 •

edited

Loading