Fix missing thread names when attaching to a non-stopping target (#439)

* Add tests for thread names after attaching, currently failing

Right after attaching to a target that does not stop on attaching, the
threads request currently fails to return the correct thread names,
because they are not known to the adapter yet and are not retrieved from
the target until it stops.

The tests are only enabled under those circumstances where the problem
can easily be fixed: local non-stop, local all-stop async, remote
non-stop.

Signed-off-by: Christian Walther <walther@indel.ch>

* Fix missing thread names when attaching to a non-stopping target

Right after attaching to a target that does not stop on attaching, the
threads request would fail to return the correct thread names, because
they were not known to the adapter yet and were not retrieved from the
target until it stops. At that point, the adapter only knew about the
threads from their `thread-created` notifications, which do not include
the name.

Doing the required -thread-info command not only when the target is
stopped, but additionally under those circumstances where it is allowed
while the target is running (local non-stop, local all-stop async,
remote non-stop), fixes that under those circumstances.

Under the remaining circumstances (local all-stop sync, remote all-stop
async, remote all-stop sync) the behavior is unmodified, as explicitly
stopping the target every time to allow the -thread-info command is
probably undesired.

This makes the tests from the previous commit pass.

Signed-off-by: Christian Walther <walther@indel.ch>

---------

Signed-off-by: Christian Walther <walther@indel.ch>

Author: cwalther

CHANGELOG.md

@@ -1,5 +1,9 @@
 # Change Log
 
+## Unreleased
+
+- Fixes [`#439`](https://github.com/eclipse-cdt-cloud/cdt-gdb-adapter/pull/439): missing thread names when attaching to targets that don’t stop on attach.
+
 ## 1.3.0
 
 - Implements [`#422`](https://github.com/eclipse-cdt-cloud/cdt-gdb-adapter/issues/422): Initial support for data breakpoints.

src/desktop/GDBTargetDebugSession.ts

@@ -109,6 +109,7 @@ export class GDBTargetDebugSession extends GDBDebugSession {
         super(backendFactory || new GDBBackendFactory());
         this.gdbserverFactory = gdbserverFactory || new GDBServerFactory();
         this.logger = logger;
+        this.isRemote = true;
     }
 
     protected logGDBRemote(message: string, level = LogLevel.Verbose) {

src/gdb/GDBDebugSessionBase.ts

@@ -123,6 +123,12 @@ export abstract class GDBDebugSessionBase extends LoggingDebugSession {
 
     protected gdb!: IGDBBackend;
     protected isAttach = false;
+    /**
+     * True if GDB is using the "remote" or "extended-remote" target. In
+     * currently supported cases, always for type="gdbtarget"
+     * (GDBTargetDebugSession) and never for type="gdb" (GDBDebugSession).
+     */
+    protected isRemote = false;
     // isRunning === true means there are no threads stopped.
     protected isRunning = false;
 
@@ -138,6 +144,7 @@ export abstract class GDBDebugSessionBase extends LoggingDebugSession {
     protected logPointMessages: { [key: string]: string } = {};
 
     protected threads: ThreadWithStatus[] = [];
+    protected missingThreadNames = false;
 
     // promise that resolves once the target stops so breakpoints can be inserted
     protected waitPausedPromise?: Promise<void>;
@@ -1266,11 +1273,29 @@ export abstract class GDBDebugSessionBase extends LoggingDebugSession {
         response: DebugProtocol.ThreadsResponse
     ): Promise<void> {
         try {
-            if (!this.isRunning) {
+            // The -thread-info command is always allowed in non-stop mode or on
+            // local targets in async all-stop mode. In sync all-stop mode or
+            // over remote connections in all-stop mode, it is only allowed when
+            // the target is stopped. GDB docs
+            // (https://sourceware.org/gdb/current/onlinedocs/gdb.html/Asynchronous-and-non_002dstop-modes.html)
+            // seem to indicate it should also always be allowed in async
+            // all-stop mode (irrespective of remote or local), but the source
+            // code (remote.c remote_target::putpkt_binary) says otherwise.
+            // The effect in situations excluded by this logic is that missing
+            // thread names are not filled in until the client issues this
+            // request at a time when the target is stopped.
+            const threadInfoAllowedWhenRunning =
+                (!this.isRemote && this.gdb.getAsyncMode()) ||
+                this.gdb.isNonStopMode();
+            if (
+                !this.isRunning ||
+                (this.missingThreadNames && threadInfoAllowedWhenRunning)
+            ) {
                 const result = await mi.sendThreadInfoRequest(this.gdb, {});
                 this.threads = result.threads
                     .map((thread) => this.convertThread(thread))
                     .sort((a, b) => a.id - b.id);
+                this.missingThreadNames = false;
             }
 
             response.body = {
@@ -2305,6 +2330,7 @@ export abstract class GDBDebugSessionBase extends LoggingDebugSession {
         switch (notifyClass) {
             case 'thread-created':
                 this.threads.push(this.convertThread(notifyData));
+                this.missingThreadNames = true;
                 break;
             case 'thread-exited': {
                 const thread: mi.MIThreadInfo = notifyData;

src/integration-tests/attach.spec.ts

@@ -10,15 +10,19 @@
 
 import * as cp from 'child_process';
 import * as path from 'path';
+import * as os from 'os';
 import { AttachRequestArguments } from '../types/session';
 import { CdtDebugClient } from './debugClient';
 import {
     fillDefaults,
+    gdbAsync,
+    gdbNonStop,
     isRemoteTest,
     standardBeforeEach,
     testProgramsDir,
 } from './utils';
 import { expect } from 'chai';
+import { DebugProtocol } from '@vscode/debugprotocol';
 
 describe('attach', function () {
     let dc: CdtDebugClient;
@@ -64,4 +68,54 @@ describe('attach', function () {
         await dc.attachHitBreakpoint(attachArgs, { line: 25, path: src });
         expect(await dc.evaluate('argv[1]')).to.contain('running-from-spawn');
     });
+
+    it('can attach to a non-stopping target and has thread names from the beginning', async function () {
+        if (isRemoteTest) {
+            // attachRemote.spec.ts is the test for when isRemoteTest
+            this.skip();
+        }
+        if ((!gdbAsync && !gdbNonStop) || os.platform() === 'win32') {
+            // This functionality is currently only available in async (incl.
+            // non-stop) mode.
+            // Windows always belongs in this case because native debugging does
+            // not support async mode there in GDB < 13, so we are actually in
+            // sync even when we requested async.
+            this.skip();
+        }
+
+        const attachArgs = fillDefaults(this.test, {
+            processId: `${inferior.pid}`,
+            initCommands: [
+                'thread name mythreadname',
+                // Simulate a target that does not stop on attaching, unlike
+                // what gdbserver does when attaching to a Unix process.
+                '-exec-continue --all',
+            ],
+        } as AttachRequestArguments);
+
+        await Promise.all([
+            dc
+                .waitForEvent('initialized')
+                .then(() => dc.configurationDoneRequest()),
+            dc.initializeRequest().then(() => dc.attachRequest(attachArgs)),
+        ]);
+
+        try {
+            const threadsResponse = await dc.threadsRequest();
+            expect(threadsResponse.success).to.be.true;
+            expect(threadsResponse.body.threads)
+                .to.be.an('array')
+                .that.satisfies((threads: DebugProtocol.Thread[]) =>
+                    threads.some((t) => t.name === 'mythreadname')
+                );
+        } finally {
+            // This is redundant as long as this case is skipped above, but
+            // becomes necessary if and when that restriction is lifted.
+            if (!gdbAsync) {
+                // In sync mode we need to stop the program again, otherwise
+                // afterEach cannot send any commands and gets stuck.
+                dc.pauseRequest({ threadId: -1 });
+            }
+        }
+    });
 });

src/integration-tests/attachRemote.spec.ts

@@ -22,8 +22,10 @@ import {
     gdbServerPath,
     gdbAsync,
     fillDefaults,
+    gdbNonStop,
 } from './utils';
 import { expect } from 'chai';
+import { DebugProtocol } from '@vscode/debugprotocol';
 
 describe('attach remote', function () {
     let dc: CdtDebugClient;
@@ -146,6 +148,42 @@ describe('attach remote', function () {
         expect(await dc.evaluate('argv[1]')).to.contain('running-from-spawn');
     });
 
+    it('can attach to a non-stopping target and has thread names from the beginning', async function () {
+        if (!gdbNonStop) {
+            // Over a remote connection, this functionality is currently only
+            // available in non-stop mode.
+            this.skip();
+        }
+        const attachArgs = fillDefaults(this.test, {
+            program: program,
+            target: {
+                type: 'remote',
+                parameters: [`localhost:${port}`],
+            } as TargetAttachArguments,
+            initCommands: [
+                'thread name mythreadname',
+                // Simulate a target that does not stop on attaching, unlike
+                // what gdbserver does when attaching to a Unix process.
+                '-exec-continue --all',
+            ],
+        } as TargetAttachRequestArguments);
+
+        await Promise.all([
+            dc
+                .waitForEvent('initialized')
+                .then(() => dc.configurationDoneRequest()),
+            dc.initializeRequest().then(() => dc.attachRequest(attachArgs)),
+        ]);
+
+        const threadsResponse = await dc.threadsRequest();
+        expect(threadsResponse.success).to.be.true;
+        expect(threadsResponse.body.threads)
+            .to.be.an('array')
+            .that.satisfies((threads: DebugProtocol.Thread[]) =>
+                threads.some((t) => t.name === 'mythreadname')
+            );
+    });
+
     it('can detach from a running program', async function () {
         const attachArgs = fillDefaults(this.test, {
             program: program,

src/web/GDBTargetDebugSession.ts

@@ -97,6 +97,7 @@ export class GDBTargetDebugSession extends GDBDebugSession {
         super(backendFactory);
         this.gdbserverFactory = gdbserverFactory;
         this.logger = logger;
+        this.isRemote = true;
     }
 
     protected logGDBRemote(message: string, level = LogLevel.Verbose) {