Using detach command for disconnection

[datphan9798]

In remote debugging, both terminate and disconnect actions currently result in the target being halted.
This change proposes using the detach command on disconnect, allowing the target to continue running after disconnection by properly handling the detach message in the GDB server.

[datphan9798]

Hi @jonahgraham , @asimgunes ,
Could you please help me review this PR?
Thank you!

[jonahgraham]

I feel out of my depth reviewing terminate/disconnect code. Can you see my review comments which are mostly just trying to understand the state of play.

In addition @thorstendb-ARM and @cwalther have done some work in this area too and I think both of them should review this if they are interested.

[cwalther]

I have tried it and found that it does not affect Indel: we don’t reach this part of the code because this.targetType is undefined because we use connectCommands. Therefore I do not have a strong opinion, as long as that stays that way.

Like in #496, this seems like a case of not clearly distinguishing between connection (which is between GDB and stub) and attachment (which is between stub and debuggee). How do we map these to DAP concepts? Probably these two issues should be looked at together.

I am not sure whether making the choice between disconnect and detach dependent on something related to termination makes sense, as the documentation of neither of those two GDB commands implies termination. Their only difference in the effect on the debuggee, as far as I understand, is whether it is resumed or not. In the DAP specification of the disconnect request, I see no guidance on which would be more appropriate. Personally I think I would prefer it not to additionally resume (after all, I can do that myself before disconnecting if that’s what I want – whereas the other way around, I would have no way to suppress the resumption), i.e. use disconnect. Edit: I forgot about the pauseIfNeeded. In that case neither behavior is what I want (I want the debuggee to stay in whatever state I left it). But I don’t care as I’m not hitting this code.

Also, if we are repurposing the sendTerminate argument of doDisconnectRequest to mean “does this come from a request asking for the debuggee to be terminated (Terminate request or Disconnect request with terminateDebuggee, as opposed to Disconnect request without terminateDebuggee)”, I think it should be renamed, because that’s not what it previously meant and what its name says. (Terminate Request and Terminated Event are not about the same thing! The former is about terminating the debuggee (hard to define what that even means when dealing with a bare-metal embedded system) and the latter about termination of the debug session.) It is not immediately clear to me why sending the TerminatedEvent is even conditional here, I see nothing in the DAP spec indicating that it would be optional (even if it may be in practice with VS Code). As far as the relation to the setExitSessionRequest/setSessionState machinery is concerned, I defer to the ARM folks, because I have never understood that code.

I concur with Jonah on terminateDebuggee not being expected to be set at all.

Edit 3: Having terminateRequestcall doDisconnectRequest seems questionable to begin with. According to the DAP spec, a Terminate request should only ask the debuggee to terminate, and not automatically end the debug session. If the debuggee does not terminate (e.g. because it is a bare-metal embedded system that has no concept of termination), it should be a no-op.

[datphan9798]

Hi @cwalther ,
Thank you for your comment!
I've changed sendTerminate argument to terminateDebuggee following to your concern.
About current implementation of terminateRequest does not match to DAP spec, I think this is quite an important topic and out of scope for this PR. Could we continue the discussion in another thread?

[cwalther]

As you can see I don’t have much helpful guidance for you (I hope someone else does – if nothing else, it would be good to have confirmation from the Arm guys (@thorstendb-ARM? @jreineckearm?) that this isn’t breaking anything for them), but I am just wondering (and don’t have time right now to test it for myself):

By the line response.body.supportTerminateDebuggee = true;, you are adding a claim that terminating the debuggee at disconnection is supported, but you don’t seem to be adding any code to fulfill that claim. Does that mean that it has worked that way before? That the debuggee was (wrongly) terminated even when that wasn’t asked for? (When working in a context that has a concept of termination, such as debugging a Unix process with gdbserver.)

And yes, we can discuss the terminateRequest (and TerminatedEvent) matter elsewhere (or ignore it). I just mentioned it to express my feeling of “Since this whole stuff appears to be wrong to begin with (and your request doesn’t affect me), you can do whatever you want as far as I’m concerned, in the worst case you make it differently wrong, which probably isn’t any worse.” 🙂

[jreineckearm]

Let me give it a try when I have time. We currently use two GDB servers. pyOCD should not be affected, behaves the same for detach and disconnect. I need to check J-Link GDB server.

The new usage makes somewhat sense since if you consider gdb server + target hardware as the "target" in GDB world.
GDB detach: "Detaching from the target normally resumes its execution, but the results will depend on your particular remote stub."
GDB disconnect: "The disconnect command closes the connection to the target, and the target is generally not resumed."

And if you consider gdb server + target hardware as the "debugee" in DAP world.
DAP Disconnect: "Indicates whether the debuggee should be terminated when the debugger is disconnected."

There will always be a bit of gray-area here given we are combining things which aren't all designed for embedded debug and/or have simple 1-2-1 mapping. Hence, would recommend to get guided by real-world usage. Which may become a bit of a trial-and-error exercise.

Yes, I skipped that part: GDB disconnect: "It will wait for GDB (this instance or another one) to connect and continue debugging. After the disconnect command, GDB is again free to connect to another target." But then you often also have CLI args to a gdb server to steer the behavior after all connections are gone....

[jreineckearm]

@datphan9798 , the one thing I'd like you to double-check is the unconditional setting of supportTerminateDebuggee = true. It becomes valid for both gdb and gdbtarget. While only gdbtarget (GDBTargetDebugSession.ts) really handles the argument. You could do something similar like in this PR to limit the support to gdbtarget for starters: #521

Also, there are web and desktop variants of GDBTargetDebugSession.ts. But we don't make use of that, so I cannot foresee impact if just adding it.

[cwalther]

And if you consider gdb server + target hardware as the "debugee" in DAP world.

Just a quick note that that’s not what I do when I have used the word debugge above and elsewhere. By debuggee I mean only the process or target hardware. The server/stub is not part of that, but part of the debugger.

[datphan9798]

By the line response.body.supportTerminateDebuggee = true;, you are adding a claim that terminating the debuggee at disconnection is supported, but you don’t seem to be adding any code to fulfill that claim. Does that mean that it has worked that way before? That the debuggee was (wrongly) terminated even when that wasn’t asked for? (When working in a context that has a concept of termination, such as debugging a Unix process with gdbserver.)

Hi @cwalther ,
Yes, disconnect request with terminateDebuggee = true will work the same as current terminate request. Sorry, I only know 1 case that leads to disconnect request is called with terminateDebuggee = true. It is terminating in attach mode. And I think it should behave similarly to terminating in launch mode (terminate request). If you know any other case that need to be handled when terminateDebuggee = true, please let me know.

@datphan9798 , the one thing I'd like you to double-check is the unconditional setting of supportTerminateDebuggee = true. It becomes valid for both gdb and gdbtarget. While only gdbtarget (GDBTargetDebugSession.ts) really handles the argument. You could do something similar like in this PR to limit the support to gdbtarget for starters: #521

Also, there are web and desktop variants of GDBTargetDebugSession.ts. But we don't make use of that, so I cannot foresee impact if just adding it.

Hi @jreineckearm ,
Thanks for your comment!
Please help me check if there is any issue on your side. I've updated to limit the support to gdbtarget only. Regarding the web variant, sorry I haven’t used it yet, so I’m not sure whether it’s necessary or what the impact would be. That’s why I didn’t make any changes for it.

[jreineckearm]

Thanks for updating the capability, @datphan9798 .

I am afraid I have to delay this PR a little further. I just made as series tests and it does change in the states the CPUs are left in. In my case a dual-core system, where first CPU is connected by a launch and the second by an attach to the previously launched. Need to understand first what I am seeing here....

What's more problematic is that closing the VS Code connection leaves GDB clients behind if target was "attached" and while the CPU is running.

What happens is

• we receive disconnect request with terminateDebuggee=false. It was an attach, hence no terminate request.
• detach while running throws Cannot execute this command while the target is running. Use the "interrupt" command to stop the target and then try again.
• that skips sendGDBExit commands, hence GDB client is not ended.

So, in our case we can't detach while in non-paused state.

Other connection parameters are at their defaults, effectively meaning

• gdbAsync=true
• gdbNonStop=false

Let me see if keeping pauseIfNeeded for both detach and disconnect fixes this...and maybe even the other observations.

I have created #525 to cover the overall review of connection-close (and possibly connection) strategies. I don't think any of us has bandwidth to sort this right away, and it has potential to go on for a bit. Let's get this PR here to a point that it can be safely merged. And address #525 separately. I think we have more topics like this in the backlog. We should discuss during next monthly meeting how and in which order to address (also based on individuals availability for such work).

[datphan9798]

Hi @jreineckearm ,
Thanks for your checking!
I see that gdbNonStop=false, so you are using all-stop mode, right? I think the error message Cannot execute this command while the target is running. Use the "interrupt" command to stop the target and then try again. is throwed when executing detach while target is running because of all-stop mode. Sorry, I forgot to check this point earlier! In my case, I am using non-stop mode, so I haven't seen this issue. I've added a condition to pauseIfNeeded if it is not non-stop mode. Could you please help me check if it works well to you?
Thank you!

[cwalther]

I think the error message Cannot execute this command while the target is running. Use the "interrupt" command to stop the target and then try again. is throwed when executing detach while target is running because of all-stop mode.

Yes. The exact condition for that error message can be seen here. See also #441 (comment).

[jreineckearm]

Apologies it took that long.
No more leftover GDB processes with your latest update.
Also, no regressions in disconnect behavior seen on our end yet.
Going to merge based on no other open feedback except the general desire to review today's situation/