add the function to edit register value

[QuocDoBV]

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]

Can one of the admins verify this patch?

[jonahgraham]

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]

Hi @jonahgraham thanks a lot for your information.

[QuocDoBV]

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]

Hi, this looks good - we need some tests. @quyettrinhrvc wrote the register read tests here: https://github.com/eclipse-cdt-cloud/cdt-gdb-adapter/blob/7ef1e9204dc750d77848e870595ea992a202d28d/src/integration-tests/var.spec.ts#L127-L129

The equivalent for setting variables is a little further down the file: https://github.com/eclipse-cdt-cloud/cdt-gdb-adapter/blob/7ef1e9204dc750d77848e870595ea992a202d28d/src/integration-tests/var.spec.ts#L189-L199

[QuocDoBV]

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]

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]

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]

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!

[QuocDoBV]

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

[QuocDoBV]

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

[QuocDoBV]

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

[jonahgraham]

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.

[QuocDoBV]

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]

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]

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]

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]

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