[Lldb-commits] [PATCH] D127500: [lldb] [llgs] Make `k` kill all processes, and fix multiple exits

[Michał Górny via Phabricator via lldb-commits]

mgorny added inline comments.


================
Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:1196
+  if (!bool(m_extensions_supported &
+            NativeProcessProtocol::Extension::multiprocess))
+    assert(!m_stdio_handle_up);
----------------
labath wrote:
> mgorny wrote:
> > labath wrote:
> > > I don't think this is right. I believe the important distinction is whether we have non-stop mode enabled or not, because in all-stop mode we are not able to send stdio while stopped, regardless of how many processes we're debugging.
> > Well, the code exploded in all-stop mode as well because in order to kill multiple processes, we effectively resume them all. I suppose there could be a way around it (synchronously killing one process after another and waiting for everything to happen) but I'm not convinced this is really worth the added complexity.
> I don't think this needs to be complex at all. What we need to basically do, is call StartSTDIOForwarding whenever the protocol allows us to send stdio, and StopSTDIOForwarding when we cannot. When we supported just a single process, the easiest way to achieve this was to hook into the started/stopped events of that process. Now that's no longer true, so maybe we just need to hook it up elsewhere.
> 
> I think starting could be done directly from the packet handlers (c/s/vCont/...). And those handlers already have to check for nonstop mode, so any deviations could be handled there:
> ```
> if (all_stop) {
>   StartSTDIOForwarding(); // Can forward from now until the stop-reply packet is send
>   return Success;
> } else {
>   SendOKResponse();
>   // Can we forward now? Or maybe it can be sent all the time?
> }
> ```
> 
> Stopping could probably stay coupled with the sending of the stop-reply packet (i.e., handling of the process event), since it's the stop reply which determines whether the stdio packet can be sent.
Thanks, this makes sense. Good news is that it seems that I can just wire it into our current `SendContinueSuccessResponse()`, unless I've missed some other case (I've grepped for everything calling `Resume()` or `Kill()`). So far I didn't special-case non-stop mode, as I still need to rework `O` packets for it.

That said, do we want to enable forwarding for kill actions at all? I suppose this was kinda implicit due to the whole Linux `PTRACE_EVENT_EXIT` ping-pong but I honestly doubt any output can happen there.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D127500/new/

https://reviews.llvm.org/D127500