eclipse-cdt-cloud / cdt-gdb-adapter

CDT GDB Debug Adapter
Eclipse Public License 2.0
28 stars 40 forks source link

Add register view feature #190

Closed quyettrinhrvc closed 2 years ago

quyettrinhrvc commented 2 years ago

Please help to review it.

eclipse-cdt-bot commented 2 years ago

Can one of the admins verify this patch?

quyettrinhrvc commented 2 years ago

Thanks! Some minor changes, the big thing needed is a test for this still. Let me know if you want some additional guidance on how to write such a test

Honestly I have no more experiences in this testing and I am investigating about it. Currently, I am referring to var.spec.ts to add test for register (it also is a part of scopeRequest, same as register view). Is it OK as a reference? And yes, it is really good if you can share some additional documents or guidance on how to write such a test. Thank you in advance!

P/s: I also try to run "yarn test" and have some errors, could you please help me check it? integration.zip

jonahgraham commented 2 years ago

A couple of the failures are tests that need updating because of this change. Most of them look like Windows issues. Please run the tests without your change to identify which ones are caused by your change. The memory + target failures are probably just windows issues and we need to make tests on Windows more resilient. Please let me know which version of gdb + gdbserver are in use and I can replicate and resolve those failures independent of your work - do this by creating a new issue in this repo with those details.

As for general advice, look at https://github.com/eclipse-cdt-cloud/cdt-gdb-adapter/blob/main/src/integration-tests/README.md - the docker running is useful as it can avoid the problems of getting the expected tool versions.

For specific advice on your test here is a test method to add to var.spec.ts:

    it('can read registers in a program', async function () {
        // read the registers
        const vr = scope.scopes.body.scopes[1].variablesReference;
        const vars = await dc.variablesRequest({ variablesReference: vr });
        expect(
            vars.body.variables.length,
            'There is a different number of variables than expected'
        ).to.be.greaterThanOrEqual(5); // 5 is a good bet to make sure that code has probably worked
        const r0 = vars.body.variables[0];
        const r1 = vars.body.variables[1];
        // can't check specific names or register values easily as that
        // is not cross platform
        expect(r0.evaluateName).to.startWith('$');
        expect(r0.name).to.not.equal(r1.name);
        // add other useful tests here, especially ones that test boundary conditions
    });

That test has some additional dependencies, so add them to the project with:

$ yarn add --dev chai-string @types/chai-string

Then you can add the extra imports too:

import * as chai from 'chai';
import * as chaistring from 'chai-string';
chai.use(chaistring);

Finally, don't forget to fix the # of scopes test:

diff --git a/src/integration-tests/var.spec.ts b/src/integration-tests/var.spec.ts
index df6a71d..b358bc2 100644
--- a/src/integration-tests/var.spec.ts
+++ b/src/integration-tests/var.spec.ts
@@ -59,7 +62,7 @@ describe('Variables Test Suite', function () {
         expect(
             scope.scopes.body.scopes.length,
             'Unexpected number of scopes returned'
-        ).to.equal(1);
+        ).to.equal(2);
     });

     afterEach(async function () {
quyettrinhrvc commented 2 years ago

Sorry for my late reply.

A couple of the failures are tests that need updating because of this change. Most of them look like Windows issues. Please run the tests without your change to identify which ones are caused by your change. The memory + target failures are probably just windows issues and we need to make tests on Windows more resilient. Please let me know which version of gdb + gdbserver are in use and I can replicate and resolve those failures independent of your work - do this by creating a new issue in this repo with those details.

As for general advice, look at https://github.com/eclipse-cdt-cloud/cdt-gdb-adapter/blob/main/src/integration-tests/README.md - the docker running is useful as it can avoid the problems of getting the expected tool versions.

For specific advice on your test here is a test method to add to var.spec.ts:

    it('can read registers in a program', async function () {
        // read the registers
        const vr = scope.scopes.body.scopes[1].variablesReference;
        const vars = await dc.variablesRequest({ variablesReference: vr });
        expect(
            vars.body.variables.length,
            'There is a different number of variables than expected'
        ).to.be.greaterThanOrEqual(5); // 5 is a good bet to make sure that code has probably worked
        const r0 = vars.body.variables[0];
        const r1 = vars.body.variables[1];
        // can't check specific names or register values easily as that
        // is not cross platform
        expect(r0.evaluateName).to.startWith('$');
        expect(r0.name).to.not.equal(r1.name);
        // add other useful tests here, especially ones that test boundary conditions
    });

That test has some additional dependencies, so add them to the project with:

$ yarn add chai-string @types/chai-string

Then you can add the extra imports too:

import * as chai from 'chai';
import * as chaistring from 'chai-string';
chai.use(chaistring);

Finally, don't forget to fix the # of scopes test:

diff --git a/src/integration-tests/var.spec.ts b/src/integration-tests/var.spec.ts
index df6a71d..b358bc2 100644
--- a/src/integration-tests/var.spec.ts
+++ b/src/integration-tests/var.spec.ts
@@ -59,7 +62,7 @@ describe('Variables Test Suite', function () {
         expect(
             scope.scopes.body.scopes.length,
             'Unexpected number of scopes returned'
-        ).to.equal(1);
+        ).to.equal(2);
     });

     afterEach(async function () {

Thanks for instruction. I understood how to create test program. However, I am having an issue related to test execution.

Please let me know which version of gdb + gdbserver are in use and I can replicate and resolve those failures independent of your work - do this by creating a new issue in this repo with those details.

Sorry but how can I add gdb+gdbserver to the test? Could you please teach me?

I tried to execute the fresh version from github and got this result

<testsuites name="Mocha Tests">
  <testsuite name="Non-test failures" tests="1" errors="0" failures="1" skipped="0" timestamp="2022-05-09T08:43:46" time="0.003">
    <testcase classname="Non-test failures" name="&quot;before all&quot; hook in &quot;{root}&quot;" time="38.861">
      <failure message="Timeout of 20000ms exceeded. For async tests and hooks, ensure &quot;done()&quot; is called; if returning a Promise, ensure it resolves.">    at Context.&lt;anonymous&gt; (dist\integration-tests\utils.js:112:5)
    at processImmediate (node:internal/timers:464:21)</failure>
    </testcase>
  </testsuite>
</testsuites>

image It seems that testProgramsDir is not correct, I think. Do you have any idea about it?

jonahgraham commented 2 years ago

Sorry for my late reply. No worries.

Thanks for instruction. I understood how to create test program. However, I am having an issue related to test execution. Excellent :)

Please let me know which version of gdb + gdbserver are in use and I can replicate and resolve those failures independent of your work - do this by creating a new issue in this repo with those details.

Sorry but how can I add gdb+gdbserver to the test? Could you please teach me?

What I was asking is what gdb and gdbserver you are using (include g++ and make too for good measure) - Here is what that looks like on my main development machine:

$ gdb --version
GNU gdb (Ubuntu 9.2-0ubuntu1~20.04.1) 9.2
Copyright (C) 2020 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.
$ gdbserver --version
GNU gdbserver (Ubuntu 9.2-0ubuntu1~20.04.1) 9.2
Copyright (C) 2020 Free Software Foundation, Inc.
gdbserver is free software, covered by the GNU General Public License.
This gdbserver was configured as "x86_64-linux-gnu"
$ make --version
GNU Make 4.2.1
Built for x86_64-pc-linux-gnu
Copyright (C) 1988-2016 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.
$ g++ --version
g++ (Ubuntu 10.3.0-1ubuntu1~20.04) 10.3.0
Copyright (C) 2020 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

It seems that testProgramsDir is not correct, I think. Do you have any idea about it?

I am unsure - I don't know how testProgramsDir could be incorrect.

I haven't verified the tests are in good operation on Windows recently as Linux is my main development environment. I will confirm and get back to you.

quyettrinhrvc commented 2 years ago
C:\Users\quyettrinh>gdb --version
GNU gdb (GDB) 7.4
Copyright (C) 2012 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
and "show warranty" for details.
This GDB was configured as "i686-pc-mingw32".
For bug reporting instructions, please see:
<http://www.gnu.org/software/gdb/bugs/>.

C:\Users\quyettrinh>gdbserver --version
GNU gdbserver (GDB) 7.4
Copyright (C) 2012 Free Software Foundation, Inc.
gdbserver is free software, covered by the GNU General Public License.
This gdbserver was configured as "i686-pc-mingw32"

C:\Users\quyettrinh>make --version
GNU Make 4.2.1
Built for x86_64-unknown-cygwin
Copyright (C) 1988-2016 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.

C:\Users\quyettrinh>g++ --version
g++ (GCC) 4.6.2
Copyright (C) 2011 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

Here is current version of my tools in Windows environment. So it means that the test will execute with these gdb/gdbserver?

jonahgraham commented 2 years ago

So it means that the test will execute with these gdb/gdbserver?

Yes. That gdb are quite old and developing with newer gdb will be much better. I recommend msys2. I can put together step-by-step instructions for how to setup msys2 if needed. Let me know. There are other options, like cygwin and chocolatey, but I use msys2 with the mingw 64-bit toolchain. A number of people use mingw (note lack of 64 in name) but I think that is very dated now and I would not recommend it. For most of my heavy development I use Linux, only using Windows to verify behaviour.

quyettrinhrvc commented 2 years ago

I can put together step-by-step instructions for how to setup msys2 if needed. Let me know.

Yes, please share me guidance if any. Thank you in advance

quyettrinhrvc commented 2 years ago

It seems that testProgramsDir is not correct, I think. Do you have any idea about it?

I am unsure - I don't know how testProgramsDir could be incorrect.

I haven't verified the tests are in good operation on Windows recently as Linux is my main development environment. I will confirm and get back to you.

I found reason of this issue. Yes, it is not problem with testProgramsDir. I used wrong command in makefile so it deleted all built objects and cannot run test. It's OK now. Btw, I tried to run test with #191 and got this result. integration#191.zip Some failures are solved but there are some new failures, could you please check it?

quyettrinhrvc commented 2 years ago

@jonahgraham I updated var.spec.ts and vars_cpp.spec.ts for register test, and got this result integrationMay10.zip Currently, it seems that there is failures of environments only (old version of gdb, gdbserver, etc.). Would you please help me perform test on your environment?

jonahgraham commented 2 years ago

The tests all work fine, except the new test "can read registers in a program" which fails like this:

  1) Variables Test Suite
       can read registers in a program:
     TypeError: vars.body.variables.at is not a function
      at Context.<anonymous> (dist/integration-tests/var.spec.js:108:44)
      at Generator.next (<anonymous>)
      at fulfilled (dist/integration-tests/var.spec.js:14:58)
      at processTicksAndRejections (internal/process/task_queues.js:95:5)

I think this happens because the Array.at method is very new in javascript. The fact that this doesn't error on compilation indicates that there is probably a configuration error in the project that needs to be resolved. In the meantime please rewrite the test to not use the Array.at method.

Besides that this PR is ready to go.

I will get back to you soon on setting up Windows host using msys2.

jonahgraham commented 2 years ago

I think this happens because the Array.at method is very new in javascript.

The problem was we were building against node 17, but we need to support node 14. I fixed this in b4bd5c9bbe5261c25c2fb0ff1c9f62375af58143

In the meantime please rewrite the test to not use the Array.at method.

@quyettrinhrvc I decided to just do this and made a couple of small fixes (934f033fe8a237cdfda35c817c8dc66a0e72cc07 and 1a1041e681fe0f96b23d8d8c564c8afc57da224c) and merged this change. It is now available in npm so you can update your dependency version, see details on version 0.0.16-next.20220511231930.4eac625.0

Congratulations :tada: on your first merged PR!

quyettrinhrvc commented 2 years ago

Congratulations 🎉 on your first merged PR!

Thank you! 😄

I will get back to you soon on setting up Windows host using msys2.

OK, I will wait for this.

quyettrinhrvc commented 2 years ago

Hi @jonahgraham , I found some guidelines to install mysys2 on RVC Jira. Is it same as yours?

Which package manager are you using?

I recommend msys2 on Windows, but you may have reasons to use others. If you do use another, make sure it is up to date. If it reports up to date, but still give gdb 7.6, that means the whole package manager is probably not actively maintained anymore.

I use something like these setup instructions on Windows - but I haven't run through this list in a while so may need some massaging. See the msys2 for help https://www.msys2.org/wiki/MSYS2-installation/

Get msys2 http://www.msys2.org/ and install Start MSYS2 MSYS (from installer or start menu) pacman -Syu # update core packages close shell if instructed to, see instructions in shell start MSYS2 MSYS again (from start menu) pacman -Su # update remaining packages pacman -S base-devel # choose all to get all normal dev tools {{}}pacman -S mingw-w64-x86_64-toolchain # get all tools, gcc, gdb, etc MSYS2 MinGW 64-bit (from start menu) – this is the shell to build from gcc --version -v # show details about compiler being used gdb --version # show details about gdb being used https://chocolatey.org/{{}}

If there is any change points, please inform me!

jonahgraham commented 2 years ago

@quyettrinhrvc - I added details to https://github.com/eclipse-cdt-cloud/cdt-gdb-adapter/blob/main/src/integration-tests/README.md#setting-up-test-environmnt-on-windows-host - it is based on the same info that you quoted.

quyettrinhrvc commented 2 years ago

@quyettrinhrvc - I added details to https://github.com/eclipse-cdt-cloud/cdt-gdb-adapter/blob/main/src/integration-tests/README.md#setting-up-test-environmnt-on-windows-host - it is based on the same info that you quoted.

It's very helpful for us. Thank you very much!