mgorny marked 5 inline comments as done.
mgorny added inline comments.
================
Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteClientBase.cpp:147
+ // vCtrlC may not do anything, so timeout if we don't get notification
+ if (ReadPacket(stop_response, milliseconds(500), false) ==
+ PacketResult::Success) {
----------------
mgorny wrote:
> labath wrote:
> > This is unfortunate. Isn't there a better way to do this? Why aren't you
> > using vCtrlC as all the comments say? Did you find any parallel to this in
> > gdb?
> Yes, it is unfortunate. I'm going to think more and try to figure out a
> better way. I was originally thinking of simply not waiting for the response
> and instead letting it come after and then taking care of any pending
> notifications before sending the next continue packet (this would require
> merging D126655 first) but I'm somewhat afraid of race conditions. Though
> maybe unnecessarily.
>
> Though OTOH I guess 500 ms may be insufficient for debugging over the
> Internet, and this would lead to even worse results.
>
> As for `vCtrlC`, I've changed the code to use `vCont;t` as the former didn't
> work well with gdbserver (and the latter is also more correct wrt the
> protocol). I've forgotten to update the comments.
Ok, I've been thinking and thinking about this and I have a better idea.
However, since this is not needed for LLGS and only for true gdbserver, I'm
going to split it into a separate patch.
My idea is basically to, in a loop:
1. issue `vCont;t` to request stopping all threads.
2. issue `?` to get the list of all stopped threads.
3. issue `qfThreadInfo` to get list of all threads.
and basically do this until both lists are the same. That should take care of
pretty much every corner case and race condition I can think of.
================
Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteClientBase.h:62
+ bool GetNonStopProtocol() { return m_non_stop_protocol; }
+
----------------
mgorny wrote:
> labath wrote:
> > Maybe call this `GetNonStopEnabled` or `GetNonStopInUse` ?
> I kinda wanted to emphasize 'protocol' here, to make it clear we aren't
> introducing full-scale non-stop support.
Thinking about it again, I suppose you're right.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D126614/new/
https://reviews.llvm.org/D126614
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
