Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure 'interrupt' and 'disconnect' signals are sent before '-gdb-exit' #338

Merged
merged 1 commit into from
Nov 8, 2024

Conversation

Luke-zhang-mchp
Copy link
Contributor

@Luke-zhang-mchp Luke-zhang-mchp commented Nov 6, 2024

Previous PRs (i.e #292 and #289) do not include an await when sending interrupt/disconnect signals if the debugger is running. This causes the -gdb-exit command to be written and queued before the interrupt/disconnect signals.

An alternate, equivalent solution would look like:

await this.gdb
    .sendCommand('interrupt')
    .then(() => this.gdb.sendCommand('disconnect'));

I did read over #292 (comment) and #292 (comment). I'm not encountering any issues with using await instead of .then. I manually tested this on our IDE with yield operators, and it worked fine. After looking at GDBBackend::sendCommand, there doesn't seem to be any reason await doesn't work. The returned Promise does not resolve until the MI parser receives a response, which guarantees that the command was sent already (that tracks right?):

public sendCommand<T>(command: string): Promise<T> {
const token = this.nextToken();
logger.verbose(`GDB command: ${token} ${command}`);
return new Promise<T>((resolve, reject) => {
if (this.out) {
/* Set error to capture the stack where the request originated,
not the stack of reading the stream and parsing the message.
*/
const failure = new Error();
this.parser.queueCommand(token, (resultClass, resultData) => {
switch (resultClass) {
case 'done':
case 'running':
case 'connected':
case 'exit':
resolve(resultData);
break;
case 'error':
failure.message = resultData.msg;
reject(failure);
break;
default:
failure.message = `Unknown response ${resultClass}: ${JSON.stringify(
resultData
)}`;
reject(failure);
}
});
this.out.write(`${token}${command}\n`);
} else {
reject(new Error('gdb is not running.'));
}
});
}

If we decide to keep .then, there is still a missing await (as noted above in the equivalent solution).

As you noted in https://github.com/eclipse-cdt-cloud/cdt-gdb-adapter/pull/292/files#r1330234785, this does not fix #295, but that issue is causing us some problems too, so I'll spend some time on that as well.

@Luke-zhang-mchp Luke-zhang-mchp marked this pull request as draft November 6, 2024 23:20
@Luke-zhang-mchp
Copy link
Contributor Author

On second thought, I'm converting this to draft to address #295 as well.

Copy link
Contributor

@jonahgraham jonahgraham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Luke-zhang-mchp I appreciate the cleanup that this PR already includes. Do you want to land this PR and then work on #295 as a separate PR?

@Luke-zhang-mchp
Copy link
Contributor Author

Yeah sure, that works too

@Luke-zhang-mchp Luke-zhang-mchp marked this pull request as ready for review November 7, 2024 15:10
@jonahgraham jonahgraham merged commit afa6d13 into eclipse-cdt-cloud:main Nov 8, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add API to easily pause target and wait for it to be ready
2 participants