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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
