From 75d56b24a4786c33728c41ade40b65973e0eea5a Mon Sep 17 00:00:00 2001 From: Luke Zhang Date: Thu, 7 Nov 2024 13:38:21 -0500 Subject: [PATCH 1/8] Move pause-and-wait/continue code into class methods Signed-off-by: Luke Zhang --- src/gdb/GDBDebugSessionBase.ts | 79 ++++++++++++++++------------------ 1 file changed, 38 insertions(+), 41 deletions(-) diff --git a/src/gdb/GDBDebugSessionBase.ts b/src/gdb/GDBDebugSessionBase.ts index f3283777..712cbc85 100644 --- a/src/gdb/GDBDebugSessionBase.ts +++ b/src/gdb/GDBDebugSessionBase.ts @@ -328,28 +328,47 @@ export abstract class GDBDebugSessionBase extends LoggingDebugSession { return this.gdb?.spawn(args); } + /** + * Sends a pause command to GDBBackend, and resolves when the debugger is + * actually paused + */ + protected async pause(): Promise { + const waitPromise = new Promise((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(); + } + + // 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 continue(): Promise { + 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 { this.waitPausedNeeded = this.isRunning; if (this.waitPausedNeeded) { - // Need to pause first - const waitPromise = new Promise((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.pause(); } try { @@ -519,11 +538,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.continue(); } } @@ -534,21 +549,7 @@ export abstract class GDBDebugSessionBase extends LoggingDebugSession { this.waitPausedNeeded = this.isRunning; if (this.waitPausedNeeded) { // Need to pause first - const waitPromise = new Promise((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.pause(); } try { @@ -646,11 +647,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.continue(); } } From b092c129ff59a4feef390939deaad33ccb82e604 Mon Sep 17 00:00:00 2001 From: Luke Zhang Date: Thu, 7 Nov 2024 13:58:00 -0500 Subject: [PATCH 2/8] Ensure debugger is suspended before disconnecting This solution is similar to #287, except the pause method is not based on an arbitrary timeout, but an emitted event from GDB that indicates the target is actually suspended. Signed-off-by: Luke Zhang --- src/desktop/GDBTargetDebugSession.ts | 13 ++++++------- src/web/GDBTargetDebugSession.ts | 13 ++++++------- 2 files changed, 12 insertions(+), 14 deletions(-) diff --git a/src/desktop/GDBTargetDebugSession.ts b/src/desktop/GDBTargetDebugSession.ts index 169ffb19..b3338e65 100644 --- a/src/desktop/GDBTargetDebugSession.ts +++ b/src/desktop/GDBTargetDebugSession.ts @@ -471,13 +471,12 @@ 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'); + this.waitPausedNeeded = + this.gdb.getAsyncMode() && this.isRunning; + + if (this.waitPausedNeeded) { + // Need to pause first, then disconnect and exit + await this.pause(); } await this.gdb.sendCommand('disconnect'); diff --git a/src/web/GDBTargetDebugSession.ts b/src/web/GDBTargetDebugSession.ts index 4049b395..4c6d3794 100644 --- a/src/web/GDBTargetDebugSession.ts +++ b/src/web/GDBTargetDebugSession.ts @@ -326,13 +326,12 @@ export class GDBTargetDebugSession extends GDBDebugSession { ): Promise { 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'); + this.waitPausedNeeded = + this.gdb.getAsyncMode() && this.isRunning; + + if (this.waitPausedNeeded) { + // Need to pause first, then disconnect and exit + await this.pause(); } await this.gdb.sendCommand('disconnect'); From 2e694da2d8a1ad2892c1abf395c3bfba1a68d0f5 Mon Sep 17 00:00:00 2001 From: Luke Zhang Date: Fri, 8 Nov 2024 15:50:14 -0500 Subject: [PATCH 3/8] rename `continue` GDBDebugSessionBase to `continuePausedThread` continue is a reserved keyword Signed-off-by: Luke Zhang --- src/gdb/GDBDebugSessionBase.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/gdb/GDBDebugSessionBase.ts b/src/gdb/GDBDebugSessionBase.ts index 712cbc85..7632bdc8 100644 --- a/src/gdb/GDBDebugSessionBase.ts +++ b/src/gdb/GDBDebugSessionBase.ts @@ -354,7 +354,7 @@ export abstract class GDBDebugSessionBase extends LoggingDebugSession { await waitPromise; } - protected async continue(): Promise { + protected async continuePausedThread(): Promise { if (this.gdb.isNonStopMode()) { await mi.sendExecContinue(this.gdb, this.waitPausedThreadId); } else { @@ -538,7 +538,7 @@ export abstract class GDBDebugSessionBase extends LoggingDebugSession { } if (this.waitPausedNeeded) { - await this.continue(); + await this.continuePausedThread(); } } @@ -647,7 +647,7 @@ export abstract class GDBDebugSessionBase extends LoggingDebugSession { } if (this.waitPausedNeeded) { - await this.continue(); + await this.continuePausedThread(); } } From a59ad98b64a951c99ff58109ac1dfccfdd9dc5dd Mon Sep 17 00:00:00 2001 From: Luke Zhang Date: Fri, 8 Nov 2024 15:54:34 -0500 Subject: [PATCH 4/8] move `waitPauseNeeded` logic into pause and continue methods Signed-off-by: Luke Zhang --- src/desktop/GDBTargetDebugSession.ts | 10 +--- src/gdb/GDBDebugSessionBase.ts | 73 +++++++++++++--------------- src/web/GDBTargetDebugSession.ts | 10 +--- 3 files changed, 38 insertions(+), 55 deletions(-) diff --git a/src/desktop/GDBTargetDebugSession.ts b/src/desktop/GDBTargetDebugSession.ts index b3338e65..64e26dcd 100644 --- a/src/desktop/GDBTargetDebugSession.ts +++ b/src/desktop/GDBTargetDebugSession.ts @@ -471,14 +471,8 @@ export class GDBTargetDebugSession extends GDBDebugSession { this.serialPort.close(); if (this.targetType === 'remote') { - this.waitPausedNeeded = - this.gdb.getAsyncMode() && this.isRunning; - - if (this.waitPausedNeeded) { - // Need to pause first, then disconnect and exit - await this.pause(); - } - + // Need to pause first, then disconnect and exit + await this.pauseIfNeeded(); await this.gdb.sendCommand('disconnect'); } diff --git a/src/gdb/GDBDebugSessionBase.ts b/src/gdb/GDBDebugSessionBase.ts index 7632bdc8..04c6dae2 100644 --- a/src/gdb/GDBDebugSessionBase.ts +++ b/src/gdb/GDBDebugSessionBase.ts @@ -330,35 +330,41 @@ export abstract class GDBDebugSessionBase extends LoggingDebugSession { /** * Sends a pause command to GDBBackend, and resolves when the debugger is - * actually paused + * actually paused. The paused thread ID is saved to `this.waitPausedThreadId`. */ - protected async pause(): Promise { - const waitPromise = new Promise((resolve) => { - this.waitPaused = resolve; - }); - if (this.gdb.isNonStopMode()) { - const threadInfo = await mi.sendThreadInfoRequest(this.gdb, {}); + protected async pauseIfNeeded(): Promise { + this.waitPausedNeeded = this.isRunning && this.gdb.getAsyncMode(); - this.waitPausedThreadId = parseInt( - threadInfo['current-thread-id'], - 10 - ); - this.gdb.pause(this.waitPausedThreadId); - } else { - this.gdb.pause(); - } + if (this.waitPausedNeeded) { + const waitPromise = new Promise((resolve) => { + this.waitPaused = resolve; + }); + if (this.gdb.isNonStopMode()) { + const threadInfo = await mi.sendThreadInfoRequest(this.gdb, {}); - // This promise resolves when handling GDBAsync for the "stopped" - // result class, which indicates that the call to `GDBBackend::pause` - // is actually finished. - await waitPromise; + this.waitPausedThreadId = parseInt( + threadInfo['current-thread-id'], + 10 + ); + this.gdb.pause(this.waitPausedThreadId); + } 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 continuePausedThread(): Promise { - if (this.gdb.isNonStopMode()) { - await mi.sendExecContinue(this.gdb, this.waitPausedThreadId); - } else { - await mi.sendExecContinue(this.gdb); + protected async continueIfNeeded(): Promise { + if (this.waitPausedNeeded) { + if (this.gdb.isNonStopMode()) { + await mi.sendExecContinue(this.gdb, this.waitPausedThreadId); + } else { + await mi.sendExecContinue(this.gdb); + } } } @@ -366,10 +372,7 @@ export abstract class GDBDebugSessionBase extends LoggingDebugSession { response: DebugProtocol.SetBreakpointsResponse, args: DebugProtocol.SetBreakpointsArguments ): Promise { - this.waitPausedNeeded = this.isRunning; - if (this.waitPausedNeeded) { - await this.pause(); - } + await this.pauseIfNeeded(); try { // Need to get the list of current breakpoints in the file and then make sure @@ -537,20 +540,14 @@ export abstract class GDBDebugSessionBase extends LoggingDebugSession { ); } - if (this.waitPausedNeeded) { - await this.continuePausedThread(); - } + await this.continueIfNeeded(); } protected async setFunctionBreakPointsRequest( response: DebugProtocol.SetFunctionBreakpointsResponse, args: DebugProtocol.SetFunctionBreakpointsArguments ) { - this.waitPausedNeeded = this.isRunning; - if (this.waitPausedNeeded) { - // Need to pause first - await this.pause(); - } + await this.pauseIfNeeded(); try { const result = await mi.sendBreakList(this.gdb); @@ -646,9 +643,7 @@ export abstract class GDBDebugSessionBase extends LoggingDebugSession { ); } - if (this.waitPausedNeeded) { - await this.continuePausedThread(); - } + await this.continueIfNeeded(); } /** diff --git a/src/web/GDBTargetDebugSession.ts b/src/web/GDBTargetDebugSession.ts index 4c6d3794..27425348 100644 --- a/src/web/GDBTargetDebugSession.ts +++ b/src/web/GDBTargetDebugSession.ts @@ -326,14 +326,8 @@ export class GDBTargetDebugSession extends GDBDebugSession { ): Promise { try { if (this.targetType === 'remote') { - this.waitPausedNeeded = - this.gdb.getAsyncMode() && this.isRunning; - - if (this.waitPausedNeeded) { - // Need to pause first, then disconnect and exit - await this.pause(); - } - + // Need to pause first, then disconnect and exit + await this.pauseIfNeeded(); await this.gdb.sendCommand('disconnect'); } From eef8427f5c502cbeac6baa8f181c91ca4eee11f8 Mon Sep 17 00:00:00 2001 From: Luke Zhang Date: Fri, 8 Nov 2024 15:56:20 -0500 Subject: [PATCH 5/8] do not require wait pause if GDB async mode is false Signed-off-by: Luke Zhang --- src/gdb/GDBDebugSessionBase.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/gdb/GDBDebugSessionBase.ts b/src/gdb/GDBDebugSessionBase.ts index 04c6dae2..48608d23 100644 --- a/src/gdb/GDBDebugSessionBase.ts +++ b/src/gdb/GDBDebugSessionBase.ts @@ -333,7 +333,7 @@ export abstract class GDBDebugSessionBase extends LoggingDebugSession { * actually paused. The paused thread ID is saved to `this.waitPausedThreadId`. */ protected async pauseIfNeeded(): Promise { - this.waitPausedNeeded = this.isRunning && this.gdb.getAsyncMode(); + this.waitPausedNeeded = this.isRunning; if (this.waitPausedNeeded) { const waitPromise = new Promise((resolve) => { From 8b3628ec62e1e3cf851fc189b5f5db4cdae4f643 Mon Sep 17 00:00:00 2001 From: Luke-zhang-04 Date: Mon, 11 Nov 2024 20:02:34 -0500 Subject: [PATCH 6/8] require async mode for pausing before exiting Signed-off-by: Luke-zhang-04 --- src/desktop/GDBTargetDebugSession.ts | 2 +- src/gdb/GDBDebugSessionBase.ts | 7 +++++-- src/web/GDBTargetDebugSession.ts | 2 +- 3 files changed, 7 insertions(+), 4 deletions(-) diff --git a/src/desktop/GDBTargetDebugSession.ts b/src/desktop/GDBTargetDebugSession.ts index 64e26dcd..de15a303 100644 --- a/src/desktop/GDBTargetDebugSession.ts +++ b/src/desktop/GDBTargetDebugSession.ts @@ -472,7 +472,7 @@ export class GDBTargetDebugSession extends GDBDebugSession { if (this.targetType === 'remote') { // Need to pause first, then disconnect and exit - await this.pauseIfNeeded(); + await this.pauseIfNeeded(true); await this.gdb.sendCommand('disconnect'); } diff --git a/src/gdb/GDBDebugSessionBase.ts b/src/gdb/GDBDebugSessionBase.ts index 48608d23..32853319 100644 --- a/src/gdb/GDBDebugSessionBase.ts +++ b/src/gdb/GDBDebugSessionBase.ts @@ -331,9 +331,12 @@ export abstract class GDBDebugSessionBase extends LoggingDebugSession { /** * 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(): Promise { - this.waitPausedNeeded = this.isRunning; + protected async pauseIfNeeded(requireAsync = false): Promise { + this.waitPausedNeeded = + this.isRunning && (!requireAsync || this.gdb.getAsyncMode()); if (this.waitPausedNeeded) { const waitPromise = new Promise((resolve) => { diff --git a/src/web/GDBTargetDebugSession.ts b/src/web/GDBTargetDebugSession.ts index 27425348..2cc72bd3 100644 --- a/src/web/GDBTargetDebugSession.ts +++ b/src/web/GDBTargetDebugSession.ts @@ -327,7 +327,7 @@ export class GDBTargetDebugSession extends GDBDebugSession { try { if (this.targetType === 'remote') { // Need to pause first, then disconnect and exit - await this.pauseIfNeeded(); + await this.pauseIfNeeded(true); await this.gdb.sendCommand('disconnect'); } From b3c22ff946f117c361a7d442551b207de2d5183e Mon Sep 17 00:00:00 2001 From: Luke Zhang Date: Wed, 13 Nov 2024 19:39:35 -0500 Subject: [PATCH 7/8] add warning comments for sync mode Unreliable behaviour due to race conditions is a known issue in sync mode. See https://github.com/eclipse-cdt-cloud/cdt-gdb-adapter/pull/339 for more details. --- src/gdb/GDBDebugSessionBase.ts | 12 ++++++++++++ src/web/GDBTargetDebugSession.ts | 4 ++++ 2 files changed, 16 insertions(+) diff --git a/src/gdb/GDBDebugSessionBase.ts b/src/gdb/GDBDebugSessionBase.ts index 32853319..97cfa45a 100644 --- a/src/gdb/GDBDebugSessionBase.ts +++ b/src/gdb/GDBDebugSessionBase.ts @@ -328,12 +328,24 @@ export abstract class GDBDebugSessionBase extends LoggingDebugSession { return this.gdb?.spawn(args); } + /** + * 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; + /** * 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; + protected async pauseIfNeeded(requireAsync = false): Promise { this.waitPausedNeeded = this.isRunning && (!requireAsync || this.gdb.getAsyncMode()); diff --git a/src/web/GDBTargetDebugSession.ts b/src/web/GDBTargetDebugSession.ts index 2cc72bd3..3f9ef181 100644 --- a/src/web/GDBTargetDebugSession.ts +++ b/src/web/GDBTargetDebugSession.ts @@ -320,6 +320,10 @@ 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 From 78cf06a25b1d76eaf9733d92fe476df3a3071157 Mon Sep 17 00:00:00 2001 From: Luke Zhang Date: Wed, 13 Nov 2024 19:43:19 -0500 Subject: [PATCH 8/8] add missing commentn in desktop GDBTargetDebugSession missing from b3c22ff946f117c361a7d442551b207de2d5183e --- src/desktop/GDBTargetDebugSession.ts | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/desktop/GDBTargetDebugSession.ts b/src/desktop/GDBTargetDebugSession.ts index de15a303..f532c83a 100644 --- 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