[Lldb-commits] [PATCH] D126614: [lldb] [gdb-remote] Client support for using the non-stop protocol

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

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