-
Notifications
You must be signed in to change notification settings - Fork 40
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
Send interrupt before disconnecting from remote #287
Send interrupt before disconnecting from remote #287
Conversation
… disconnect request to ensure we only disconnect from a halted target
src/GDBTargetDebugSession.ts
Outdated
@@ -535,6 +535,8 @@ export class GDBTargetDebugSession extends GDBDebugSession { | |||
this.serialPort.close(); | |||
|
|||
if (this.targetType === 'remote') { | |||
// Ensure target is halted before disconnecting. | |||
await this.gdb.sendCommand('interrupt'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will only work if in async mode. Have a look at the implementation of pause - I think you should pause it first and then do the disconnect.
Is this change testable using normal gdbserver, or does it require special hardware kit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We encountered this issue across multiple remote targets, so I think normal gdbserver should be fine to test with.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see what you mean - let me change this (I'll call pause instead of sending the interrupt myself)
The shutdown / termination / disconnect code in cdt-gdb-adapter is undertested. The reason I would like to see a test for your change is so that we try not to regress here. However I think we can forgo the test as this is just a "fixup" of #273. If you are able to write some tests to verify disconnect is working as expected it would be most welcome - but I recommend doing that independently of this PR. |
@jonahgraham I updated the code, but I was having to introduce some sort of tiny timeout to ensure it actually has time to pause - this.gdb.pause is a sync function it seems to an await will have no effect. If you know of a better way to make use of this, I'd really appreciate it, but this was how I managed to get it to work. |
Hmm. That is problematic to get this right. This snippet of code is how other places handle it in the code that need to ensure target is paused before operating on it. The snippet ensures that the the target actually stops before proceeding. I think we need to do something similar here, but we probably need to refactor it to make it more (re)usable cdt-gdb-adapter/src/GDBDebugSession.ts Lines 474 to 492 in 250881c
But first, lets separate these two issues - for that I am going to take a step back to your original patchset. What happens in your original patchset (i.e. just sending I think we can document (in the code) as only supporting this shutdown sequence when using async mode and only sending the interrupt if |
The test case I've been using is running an infinite loop at the end (by design) when I am trying to stop - Pausing and then stopping doesn't cause any issues, so no error message is received - it just doesn't respond with the SIGTRAP like the case when it's actually running when I attempt to stop. Sending just "interrupt" generally hasn't caused issues as far as I can see. Would the following be a good solution for this:
This sequence seems to work properly, and when the target is paused, doesn't attempt to send the interrupt signal. |
It looks right, I'll double-check once you submit it as a new commit. |
Replaced with #289 |
Ensure that for a remote target, an interrupt signal is sent before a disconnect request to ensure we only disconnect from a halted target.
This resolves an issue that, on remote targets, can be reproduced as follows:
The disconnect request errors out, returning the following error message:
"Cannot execute this command while the target is running. Use the 'interrupt' command to stop the target and then try again."
This results in the GDB Server not being killed, which can cause issues for future runs.
Note that this only happens when we continue after hitting a breakpoint and try to stop without pausing. If we never set a breakpoint to begin with, we can disconnect without pausing just fine.
@jonahgraham Please let me know if there needs to be a test written for this, and if so, how: I tried to look for a test case that deals with the disconnect request, but I couldn't find one.