Add API to pause target and wait for debugger to be suspended (#339)

* require async mode for pausing before exiting * add warning comments for sync mode Unreliable behaviour due to race conditions is a known issue in sync mode. See #339 for more details. Signed-off-by: Luke Zhang <[email protected]>
eclipse-cdt-cloud · Nov 14, 2024 · 6ba0de8 · 6ba0de8
1 parent afa6d13
commit 6ba0de8
Show file tree

Hide file tree

Showing 3 changed files with 58 additions and 57 deletions.
diff --git a/src/desktop/GDBTargetDebugSession.ts b/src/desktop/GDBTargetDebugSession.ts
@@ -462,6 +462,10 @@ export class GDBTargetDebugSession extends GDBDebugSession {
         });
     }
 
+    /**
+     * WARNING: `disconnectRequest` is unreliable in sync mode.
+     * @see {@link https://github.com/eclipse-cdt-cloud/cdt-gdb-adapter/pull/339#discussion_r1840549671}
+     */
     protected async disconnectRequest(
         response: DebugProtocol.DisconnectResponse,
         _args: DebugProtocol.DisconnectArguments
@@ -471,15 +475,8 @@ export class GDBTargetDebugSession extends GDBDebugSession {
                 this.serialPort.close();
 
             if (this.targetType === 'remote') {
-                if (this.gdb.getAsyncMode() && this.isRunning) {
-                    // See #295 - this use of "await" is to try to slightly delay the
-                    // call to disconnect. A proper solution that waits for the
-                    // interrupt to be successful is needed to avoid future
-                    // "Cannot execute this command while the target is running"
-                    // errors
-                    await this.gdb.sendCommand('interrupt');
-                }
-
+                // Need to pause first, then disconnect and exit
+                await this.pauseIfNeeded(true);
                 await this.gdb.sendCommand('disconnect');
             }
 

diff --git a/src/gdb/GDBDebugSessionBase.ts b/src/gdb/GDBDebugSessionBase.ts
@@ -328,13 +328,29 @@ export abstract class GDBDebugSessionBase extends LoggingDebugSession {
         return this.gdb?.spawn(args);
     }
 
-    protected async setBreakPointsRequest(
-        response: DebugProtocol.SetBreakpointsResponse,
-        args: DebugProtocol.SetBreakpointsArguments
-    ): Promise<void> {
-        this.waitPausedNeeded = this.isRunning;
+    /**
+     * Sends a pause command to GDBBackend, and resolves when the debugger is
+     * actually paused. The paused thread ID is saved to `this.waitPausedThreadId`.
+     * @param requireAsync - require gdb to be in async mode to pause
+     */
+    protected async pauseIfNeeded(requireAsync?: false): Promise<void>;
+
+    /**
+     * Sends a pause command to GDBBackend, and resolves when the debugger is
+     * actually paused. The paused thread ID is saved to `this.waitPausedThreadId`.
+     *
+     * @param requireAsync - require gdb to be in async mode to pause
+     * @deprecated the `requireAsync` parameter should not be used and will be
+     * removed in the future.
+     * See {@link https://github.com/eclipse-cdt-cloud/cdt-gdb-adapter/pull/339#discussion_r1840549671}
+     */
+    protected async pauseIfNeeded(requireAsync: true): Promise<void>;
+
+    protected async pauseIfNeeded(requireAsync = false): Promise<void> {
+        this.waitPausedNeeded =
+            this.isRunning && (!requireAsync || this.gdb.getAsyncMode());
+
         if (this.waitPausedNeeded) {
-            // Need to pause first
             const waitPromise = new Promise<void>((resolve) => {
                 this.waitPaused = resolve;
             });
@@ -349,8 +365,29 @@ export abstract class GDBDebugSessionBase extends LoggingDebugSession {
             } else {
                 this.gdb.pause();
             }
+
+            // This promise resolves when handling GDBAsync for the "stopped"
+            // result class, which indicates that the call to `GDBBackend::pause`
+            // is actually finished.
             await waitPromise;
         }
+    }
+
+    protected async continueIfNeeded(): Promise<void> {
+        if (this.waitPausedNeeded) {
+            if (this.gdb.isNonStopMode()) {
+                await mi.sendExecContinue(this.gdb, this.waitPausedThreadId);
+            } else {
+                await mi.sendExecContinue(this.gdb);
+            }
+        }
+    }
+
+    protected async setBreakPointsRequest(
+        response: DebugProtocol.SetBreakpointsResponse,
+        args: DebugProtocol.SetBreakpointsArguments
+    ): Promise<void> {
+        await this.pauseIfNeeded();
 
         try {
             // Need to get the list of current breakpoints in the file and then make sure
@@ -518,38 +555,14 @@ export abstract class GDBDebugSessionBase extends LoggingDebugSession {
             );
         }
 
-        if (this.waitPausedNeeded) {
-            if (this.gdb.isNonStopMode()) {
-                mi.sendExecContinue(this.gdb, this.waitPausedThreadId);
-            } else {
-                mi.sendExecContinue(this.gdb);
-            }
-        }
+        await this.continueIfNeeded();
     }
 
     protected async setFunctionBreakPointsRequest(
         response: DebugProtocol.SetFunctionBreakpointsResponse,
         args: DebugProtocol.SetFunctionBreakpointsArguments
     ) {
-        this.waitPausedNeeded = this.isRunning;
-        if (this.waitPausedNeeded) {
-            // Need to pause first
-            const waitPromise = new Promise<void>((resolve) => {
-                this.waitPaused = resolve;
-            });
-            if (this.gdb.isNonStopMode()) {
-                const threadInfo = await mi.sendThreadInfoRequest(this.gdb, {});
-
-                this.waitPausedThreadId = parseInt(
-                    threadInfo['current-thread-id'],
-                    10
-                );
-                this.gdb.pause(this.waitPausedThreadId);
-            } else {
-                this.gdb.pause();
-            }
-            await waitPromise;
-        }
+        await this.pauseIfNeeded();
 
         try {
             const result = await mi.sendBreakList(this.gdb);
@@ -645,13 +658,7 @@ export abstract class GDBDebugSessionBase extends LoggingDebugSession {
             );
         }
 
-        if (this.waitPausedNeeded) {
-            if (this.gdb.isNonStopMode()) {
-                mi.sendExecContinue(this.gdb, this.waitPausedThreadId);
-            } else {
-                mi.sendExecContinue(this.gdb);
-            }
-        }
+        await this.continueIfNeeded();
     }
 
     /**

diff --git a/src/web/GDBTargetDebugSession.ts b/src/web/GDBTargetDebugSession.ts
@@ -320,21 +320,18 @@ export class GDBTargetDebugSession extends GDBDebugSession {
         return this.gdbserverProcessManager?.stop();
     }
 
+    /**
+     * WARNING: `disconnectRequest` is unreliable in sync mode.
+     * @see {@link https://github.com/eclipse-cdt-cloud/cdt-gdb-adapter/pull/339#discussion_r1840549671}
+     */
     protected async disconnectRequest(
         response: DebugProtocol.DisconnectResponse,
         _args: DebugProtocol.DisconnectArguments
     ): Promise<void> {
         try {
             if (this.targetType === 'remote') {
-                if (this.gdb.getAsyncMode() && this.isRunning) {
-                    // See #295 - this use of "await" is to try to slightly delay the
-                    // call to disconnect. A proper solution that waits for the
-                    // interrupt to be successful is needed to avoid future
-                    // "Cannot execute this command while the target is running"
-                    // errors
-                    await this.gdb.sendCommand('interrupt');
-                }
-
+                // Need to pause first, then disconnect and exit
+                await this.pauseIfNeeded(true);
                 await this.gdb.sendCommand('disconnect');
             }