Support kill() process when GDB does not exit

[bangdang1411]

Following the discussion in PR #256 I would like to commit the new change about it. I think it is easy to review because the change is different from #256

Solution: Override disconnectRequest() with option exitGDBServer() if this request does not use for restart Request. It has some benefits:

1. disconnectRequest does not wait over 2 seconds for response. exitGDBServer() will handle separately with the request.
2. restartRequest will be implemented in RenesasGDBTargetDebugSession, that mean it is not depended on timeout of others Request.
3. does not need to implement terminateRequest because it seems cannot update CallStack view when GDB exited.

[bangdang1411]

Hi @jonahgraham-san, I have read this discussion in #256 and #258 PR and would like to commit my solution. This commit can fix for Renesas devices, but it needs to be reviewed and confirmed for issue #258. The fix for issue #256 about "restartRequest" must change some things in RenesasGDBTargetDebugSession and override the restartRequest() function. Thank you.

[jonahgraham]

If you are going to override exitGDBServer in Renesas's backend, why not just override disconnectRequest entirely then? Then we can merge #258 for the general case and Renesas can do whatever it wants?

PS. If one of the reasons you want to change it here is for the isServerTerminated, then I suggest you poll this.gdbserver.exitCode, or better add an additional listener on gdbserver's 'exit' (and have it set a promise so you can remove polling altogether)

[bangdang1411]

Hi @jonahgraham-san,

If you are going to override exitGDBServer in Renesas's backend, why not just override disconnectRequest entirely then? Then we can merge https://github.com/eclipse-cdt-cloud/cdt-gdb-adapter/pull/258 for the general case and Renesas can do whatever it wants.

My solution is not to override exitGDBServer() and disconnectRequest in Renesas's backend. I will override restartRequest, please see the commit in renesas-gdb-adapter that I comment in the Jira ticket. This commit in cdt-gdb-adapter will fix the issue of #258, It supports kill gdb process after 4 seconds timeout (it is not dependent on 2 seconds timeout of disconnectRequest). The bug restart Renesas devices will use this commit to implement restartRequest.

PS. If one of the reasons you want to change it here is for the isServerTerminated, then I suggest you poll this.gdbserver.exitCode, or better add an additional listener on gdbserver's 'exit' (and have it set a promise so you can remove polling altogether)

Thank you for your suggestion. Please help me review the latest commit and give me your idea. Do we need to write the integration test for it? Thank you.

[bangdang1411]

Hi @jonahgraham-san, The latest commit fix both issue Terminateand RestartGDB server. It does not need to implement in Renesas's backend. I see that cdt-gdb-adapter takes over 4 seconds to send disconnect response to vs code has no error returned. I think the node process is not killed at this time and we can wait for the exiting gdb process completed.

[07:45:05.525 UTC] From client: disconnect({"restart":false})
[07:45:05.527 UTC] GDB command: 49 -gdb-exit
[07:45:05.543 UTC] GDB result: 49 exit
[07:45:05.543 UTC] GDB notify async: thread-group-exited,id="i1"
[07:45:08.535 UTC] To client: {"seq":0,"type":"event","event":"output","body":{"category":"server","output":"Disconnected from the Target Debugger.\r\n"}}
[07:45:08.551 UTC] To client: {"seq":0,"type":"event","event":"output","body":{"category":"server","output":"e2-server-gdb has exited with code 0"}}
[07:45:08.551 UTC] To client: {"seq":0,"type":"response","request_seq":10,"command":"disconnect","success":true}

[bangdang1411]

I need to spin this up as I want to understand why the code in VSCode is not being observed. That will take me another day.

I try change the code that does not send the response fordisconnect request. In my environment, after 6 seconds VS Code will kill the node process (I assume the time is depended on the performance of PC). I think we need to extend the timeout on VS Code side. But this code can work for now and the change should be in VSCode repository.

Please check documentation again, on Linux SIGTERM is the default, that is why I was raising that as an issue as kill() on Windows is equivalent to SIGKILL, but kill() on Linux is SIGTERM.

Sorry, I misunderstood your question. SIGTERM is only supported in Linux but it is can be ignored and the Promise that I implemented will wait forever. Is it correct? I changed both case Windows and Linux to 'SIGKILL'. My understanding is 'SIGKILL' can work in both environments and cannot be ignored.

[jonahgraham]

In my environment, after 6 seconds VS Code will kill the node process (I assume the time is depended on the performance of PC).

I have tested this on a couple of computers and it seems that node is killed in about 2 seconds on each of them.

However, I have a new proposal on how to resolve this problem. Instead of running cdt-gdb-adapter as a separate node process, we can run it in the extension host process. That means that the node process won't be killed at all when disconnect completes and we can do whatever we want. However this moves the problem in part to VSCode extension. Please look up DebugAdapterInlineImplementation in https://code.visualstudio.com/api/extension-guides/debugger-extension

In parallel, please raise an issue with vscode, perhaps with a PR, to remove the 2 second kill.

[bangdang1411]

Hi @jonahgraham-san,

However, I have a new proposal on how to resolve this problem. Instead of running cdt-gdb-adapter as a separate node process, we can run it in the extension host process. That means that the node process won't be killed at all when disconnect completes and we can do whatever we want. However this moves the problem in part to VSCode extension. Please look up DebugAdapterInlineImplementation in https://code.visualstudio.com/api/extension-guides/debugger-extension

I understand your idea but it takes time to investigate and implement. Anyway, It should be implemented in Renesas vs code. Is that correct? Additionally, when it was implemented, this PR should be merged to fix the issue gdb does not exit normally after 4 seconds.

In parallel, please raise an issue with vscode, perhaps with a PR, to remove the 2 second kill.

I see the same requirement, but it is discussing in another issue

Could you please share your idea? Thank you.

[jonahgraham]

Hi @jonahgraham-san,

However, I have a new proposal on how to resolve this problem. Instead of running cdt-gdb-adapter as a separate node process, we can run it in the extension host process. That means that the node process won't be killed at all when disconnect completes and we can do whatever we want. However this moves the problem in part to VSCode extension. Please look up DebugAdapterInlineImplementation in https://code.visualstudio.com/api/extension-guides/debugger-extension

I understand your idea but it takes time to investigate and implement. Anyway, It should be implemented in Renesas vs code. Is that correct?

The main change in cdt-gdb-vscode (and any extensions that use cdt-gdb-adapter too, line renesas-gdb-vscode). It needs review work up and down the entire stack because, for example, cdt-gdb-adapter was written assuming that it would be in its own process. For the most part it probably will work, but the entry points and cleanup especially will need some investigating to be sure.

Additionally, when it was implemented, this PR should be merged to fix the issue gdb does not exit normally after 4 seconds.

I don't think so, at least not as is. Renesas can do a workaround for the issues that is affecting them in their own adapter. That is to say, this entire PR can just be done in Renesas's own code base as it is a workaround for Renesas's issue that does not work in the general case.

In parallel, please raise an issue with vscode, perhaps with a PR, to remove the 2 second kill.

I see the same requirement, but it is discussing in another issue

Could you please share your idea? Thank you.

I just read that and it is on point. Of course someone needs to provide a PR to improve the situation there based on what is discussed.

[bangdang1411]

The main change in cdt-gdb-vscode (and any extensions that use cdt-gdb-adapter too, line renesas-gdb-vscode). It needs review work up and down the entire stack because, for example, cdt-gdb-adapter was written assuming that it would be in its own process. For the most part it probably will work, but the entry points and cleanup especially will need some investigating to be sure.

I am investigating the VS Code Mock Debug and trying to know how it works. But it seems more difficult than my experience. Could you please help me take a look at it and give me some ideas on how to implement your solution? Thank you.

I don't think so, at least not as is. Renesas can do a workaround for the issues that is affecting them in their own adapter. That is to say, this entire PR can just be done in Renesas's own code base as it is a workaround for Renesas's issue that does not work in the general case.

I understood. But please consider if we have a short-term solution for fixing the restart bug in Renesas. It should be fixed in the next release (no more time). Thank you.

I just read that and it is on point. Of course, someone needs to provide a PR to improve the situation there based on what is discussed.

My understanding in that discussion is requested to improve the UX when the termination process is more than 2s. I think removing the 2-second kill is not acceptable.

[jonahgraham]

I think the solution properly belongs in the Renesas extension so I am going to close this for now. If there is a generic version please provide it.

Please also see https://github.com/eclipse-cdt-cloud/cdt-gdb-adapter/issues/253#issuecomment-1480059599 for the next time you update to newer cdt-gdb-adapter.