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

[Pavel Labath via Phabricator via lldb-commits]

labath accepted this revision.
labath added inline comments.
This revision is now accepted and ready to land.


================
Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:1196
+  if (!bool(m_extensions_supported &
+            NativeProcessProtocol::Extension::multiprocess))
+    assert(!m_stdio_handle_up);
----------------
mgorny wrote:
> mgorny wrote:
> > 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.
> Oh, and regarding non-stop mode, I've left it as TODO for now. The whole stdio forwarding needs to be fixed for non-stop mode anyway, and since we don't expect to have two processes running simultaneously yet, I'm going to look into it separately.
> 
> That said, this is probably going to be a "LLDB extension". FWICS gdb pretty much uses `O` packets only to deliver debugger-specific messages and doesn't forward stdio at all, nor special-cases `O` in non-stop mode. My initial idea is to always send `O` as asynchronous notifications. I suppose we could then enable stdio forwarding as soon as non-stop mode is enabled, and keep it enabled until the very end.
Using async notifications sounds reasonable. Also, ew, I did not realize that's what O packets were meant for.

I don't think we need to enable forwarding for kill packets. In fact, I would be surprised if lldb was prepared to handle them here.


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

https://reviews.llvm.org/D127500