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

[Pavel Labath via Phabricator via lldb-commits]

labath added a reviewer: jingham.
labath added a comment.

(I think Jim would be interested in following this.)

I'm not sure I understand what this does. Is it that, when the setting is enabled, lldb uses non-stop communication at the protocol level, but then forces an all-stop whenever one thread stops (so that the upper layers are happy)?

> API tests required changing the Process::Halt() API to inspect process'
> private state rather than the public state, that remains eStateUnloaded
> in gdb_remote_client tests.

I don't think that checking the private state is correct there. Doesn't that just mean that there's a bug in the tests that it's not simulating a real gdbserver sufficiently well? Or maybe (if the simulated responses are reasonable) lldb should be made to work (properly set the state)  for the simulated responses?



================
Comment at: lldb/packages/Python/lldbsuite/test/gdbclientutils.py:26
 
     Framing includes surrounding the message between $ and #, and appending
     a two character hex checksum.
----------------
I guess this comment needs updating. I've been also thinking whether this is a sufficiently obvious way to send notifications, but I kinda like the brevity of it. 


================
Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteClientBase.cpp:128
+            PacketResult::Success) {
+          LLDB_LOGF(log, "GDBRemoteClientBase::%s () vCtrlC failed",
+                    __FUNCTION__);
----------------
not `vCont;t` ? Also, I think these log lines are unnecessarily verbose, and are obscuring the real code. I think a single log line (indicating failure) should be more than enough as more information (the kind of failure, for instance) can be determined by looking at the packet log.


================
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) {
----------------
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?


================
Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteClientBase.cpp:450
+      if (m_comm.GetNonStopProtocol())
+        success = m_comm.SendPacketNoLock("vCtrlC") == PacketResult::Success;
+      else {
----------------
Isn't the vCtrlC packet supposed to have a response (OK or Exx according to <https://sourceware.org/gdb/onlinedocs/gdb/Packets.html#Packets>) ?


================
Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteClientBase.h:62
 
+  bool GetNonStopProtocol() { return m_non_stop_protocol; }
+
----------------
Maybe call this `GetNonStopEnabled` or `GetNonStopInUse` ?


================
Comment at: lldb/test/API/functionalities/gdb_remote_client/TestGDBRemoteClient.py:686
+            def QNonStop(self, val):
+                assert val == 1
+                return "OK"
----------------
This is not a good assertion. Maybe you could replace check that after-the-fact with `assertPacketLogContains`?


================
Comment at: lldb/test/API/functionalities/gdb_remote_client/TestGDBRemoteClient.py:717
+        process.Stop()
+        self.assertPacketLogContains(["vStopped", "vCont;t"])
+        self.assertEqual(process.GetSelectedThread().GetStopReason(),
----------------
I think here you'll need to manually extract (and verify) the running/stopped events from the appropriate listener. In async mode, Continue and Stop return immediately, so there's no guarantee that anything has actually happened at this point.


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

https://reviews.llvm.org/D126614