eclipse-cdt-cloud / cdt-gdb-adapter

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

Ending an OpenOCD process at the end of debugging #253

Closed jonahgraham closed 1 year ago

jonahgraham commented 1 year ago

When launching target server (gdbserver, openocd, other) the current implementation of cdt-gdb-adapter assumes that the server will be launched in a way that the server terminates once gdb disconnects. That is what happens with gdbserver and the --once argument.

For targets like openocd, having a way to exit cleanly is needed.

Discussed in https://github.com/eclipse-cdt-cloud/cdt-cloud/discussions/27

Originally posted by **ConorDavenport** February 7, 2023 Hi, I'm using Theia Blueprint with the cdt-gdb-vscode extension to debug an application on the Polarfire SoC. The debug configuration can be seen below. An OpenOCD instance is started and gdb connects to it. I am able to step through code and debug as expected, however when ending the debug session the instance of OpenOCD stays active and needs to be killed manually. Is there any way to configure the debug session to kill the OpenOCD process when the deubg session ends? Thanks! ``` "configurations": [{ "name":"debug-smp", "type":"gdbtarget", "request":"launch", "program": "path/to/executable", "gdb": "path/to/gdb", "logFile": "path/to/logfile", "openGdbConsole": true, "initCommands": [ "set mem inaccessible-by-default off", "set $target_riscv=1", "set architecture riscv:rv64" ], "preRunCommands": [ "thread apply all set $pc=_start", "thread apply all set $mie=0", "thread apply all set $mip=0", "thread apply all set $mstatus=0" ], "imageAndSymbols": { "symbolFileName": "path/to/executable", "imageFileName": "path/to/executable" }, "target": { "host": "localhost", "port": "3333", "server": "path/to/openocd", "serverParameters": ["--command", "set DEVICE MPFS", "--file", "board/microsemi-riscv.cfg", "--command", "set COREID 1"], "serverStartupDelay": 5000, "cwd": "${workspaceFolder}" } }] } ```
ArsenInstigate commented 1 year ago

Hi @jonahgraham. I override disconnectRequest in GDBTargetDebugSession.ts of cdt-gdb-adapter.

    protected async disconnectRequest(
        response: DebugProtocol.DisconnectResponse,
        _args: DebugProtocol.DisconnectArguments
    ): Promise<void> {
        try {
            await this.gdb.sendGDBExit();
            await this.gdbserver?.kill();
            this.sendResponse(response);
        } catch (err) {
            this.sendErrorResponse(
                response,
                1,
                err instanceof Error ? err.message : String(err)
            );
        }
    }

In Contributing to Eclipse Theia guide is written that I should do Pull Request. But I have no permission even to make branch on the repo. Where would you advise me to start? Thank you.

santimchp commented 1 year ago

Thanks @ArsenInstigate . A potential fix to this issue, so it might be an interesting topic to be discussed @jonahgraham?

jonahgraham commented 1 year ago

I didn't realize that the cross reference from PR #256 didn't make it back here. @QuocDoBV is working on a comprehensive fix to this, but your fix is a good short term improvement, so a PR with that change will be more than welcome. We need a test for it too, I started the rough outline of a test that may work here:

diff --git a/src/integration-tests/launchRemote.spec.ts b/src/integration-tests/launchRemote.spec.ts
index 63f861a..f219274 100644
--- a/src/integration-tests/launchRemote.spec.ts
+++ b/src/integration-tests/launchRemote.spec.ts
@@ -16,8 +16,8 @@ import {
 import { CdtDebugClient } from './debugClient';
 import { fillDefaults, standardBeforeEach, testProgramsDir } from './utils';

-describe('launch remote', function () {
-    let dc: CdtDebugClient;
+describe.only('launch remote', function () {
+    let dc: CdtDebugClient | undefined;
     const emptyProgram = path.join(testProgramsDir, 'empty');
     const emptySrc = path.join(testProgramsDir, 'empty.c');

@@ -26,11 +26,11 @@ describe('launch remote', function () {
     });

     afterEach(async function () {
-        await dc.stop();
+        await dc?.stop();
     });

     it('can launch remote and hit a breakpoint', async function () {
-        await dc.hitBreakpoint(
+        await dc?.hitBreakpoint(
             fillDefaults(this.test, {
                 program: emptyProgram,
                 target: {
@@ -43,4 +43,25 @@ describe('launch remote', function () {
             }
         );
     });
+
+    it('closes target server at exit', async function () {
+        await dc?.hitBreakpoint(
+            fillDefaults(this.test, {
+                program: emptyProgram,
+                target: {
+                    // By default we use --once with gdbserver so that it naturally quits
+                    // when gdb disconnects. Here we leave that off and uses extended-remote
+                    // so that gdbserver doesn't quit automatically
+                    type: 'extended-remote',
+                    serverParameters: [':0', emptyProgram],
+                } as TargetLaunchArguments,
+            } as TargetLaunchRequestArguments),
+            {
+                path: emptySrc,
+                line: 3,
+            }
+        );
+
+        await dc?.stop();
+    });
 });

In Contributing to Eclipse Theia guide is written that I should do Pull Request. But I have no permission even to make branch on the repo. Where would you advise me to start? Thank you.

It is expected that you don't have permission to make a branch on the repo. The Pull Request should come from your fork of the repo. If this is your first time making a PR to a repo you don't own, some of the steps may be new to you. Please feel free to ask questions and I'll do my best to support you.

ArsenInstigate commented 1 year ago

Hi @jonahgraham. Thank you for this test. I want to know where it is come from. Because when I get the latest cdt-gdb-adapter it is slightly different from your original sources in the comment.

jonahgraham commented 1 year ago

Hi @jonahgraham. Thank you for this test. I want to know where it is come from. Because when I get the latest cdt-gdb-adapter it is slightly different from your original sources in the comment.

The diff I supplied is something I started writing to try and solve this problem. That new test isn't in the code base yet, but you can use it as a starting point.

I got an email notification about an issue with ptrace, I assume you either solved it and deleted the comment, or github had a hiccup and lost it. If it is the latter, please repost

ArsenInstigate commented 1 year ago

Thank you @jonahgraham. I will write the test.

Yes, I solve the "issue with ptrace". But in general, when if I solve my own issue, should I delete it or I should also add the solution at the end? Because I remove useless/by my opinion/ questions and if it is something valuable I leave it.

jonahgraham commented 1 year ago

Thank you @jonahgraham. I will write the test.

Great. Thanks.

Yes, I solve the "issue with ptrace". But in general, when if I solve my own issue, should I delete it or I should also add the solution at the end? Because I remove useless/by my opinion/ questions and if it is something valuable I leave it.

No opinion on that. I sometimes mark comments as hidden to make it easier for future people to follow the conversation. But because I got an email notification about the ptrace I didn't want to ignore it. github doesn't notify about deleted comments.

jonahgraham commented 1 year ago

Fixed in #258 with the new flag added to the launch.json in https://github.com/eclipse-cdt-cloud/cdt-gdb-vscode/pull/87

The new behaviour can be disabled with automaticallyKillServer.

cc: @bangdang1411 - you may want to set automaticallyKillServer to false in your extension if you are not overridding disconnectRequest

ArsenInstigate commented 1 year ago

Hi @jonahgraham I hope this message finds you well. We appreciate your efforts in maintaining this valuable tool for the community, and we understand that releasing a new version can be a complex process. If it is not too much trouble, could you please provide an estimated timeline for the next release of cdt-gdb-adapter, including our changes? We would like to plan our upcoming projects accordingly and ensure that we are using the latest version of the tool.

Thank you for your time and attention to this matter. Best regards, Arsen

jonahgraham commented 1 year ago

Hi @ArsenInstigate - I try to put out pre-releases whenever asked. Therefore please enjoy https://www.npmjs.com/package/cdt-gdb-adapter/v/0.0.20 which contains the improvement you have made in this PR and I have updated cdt-gdb-vscode to consume it: https://github.com/eclipse-cdt-cloud/cdt-gdb-vscode/commit/9847f0df1cffea9a652cb0dee96b34cc48a1c834 and bumped the version of cdt-gdb-vscode to 0.0.96 in https://github.com/eclipse-cdt-cloud/cdt-gdb-vscode/commit/7a79ce9b1c9f7210c3df4dc0a4f125e78d647162

If you find further problems I am happy to put out further pre-releases, so please just ask again.

ArsenInstigate commented 1 year ago

Thank you very much @jonahgraham