[Lldb-commits] [PATCH] D125575: [lldb] [llgs] Implement non-stop style stop notification packets

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I think this is a pretty good start. The main thing bugging me is the two bool flags on the stop reply packet functions. I wonder if it would be more readable to split this into two functions. Something like:

  SendStopReplyForThread(bool force_synchronous /*seems better than allow_async, but that could just be me*/, bool enqueue /*enqueues just this thread*/);
  EnqueueStopReplyPackets(NativeThreadProtocol &thread_to_skip);

perhaps?



================
Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:3749
+    return SendErrorResponse(Status("No pending notification to ack"));
+  m_stop_notification_queue.pop_front();
+  if (!m_stop_notification_queue.empty())
----------------
Is this correct? I would expect to this to first read (`front()`) the packet before popping it.


================
Comment at: lldb/test/API/tools/lldb-server/TestLldbGdbServer.py:1414
+
+    def test_QNonStop(self):
+        self.build()
----------------
Could you put these into a new file? TestLldbGdbServer is already one of the longest running tests...


================
Comment at: lldb/test/API/tools/lldb-server/TestLldbGdbServer.py:1443
+            m = self.expect_gdbremote_sequence()
+            del m["O_content"]
+            threads = [m]
----------------
Is this necessary? (I get that it's not useful, but it seems weird...)


================
Comment at: lldb/test/API/tools/lldb-server/TestLldbGdbServer.py:1525
+        procs = self.prep_debug_monitor_and_inferior(
+                inferior_args=["thread:new"])
+        self.test_sequence.add_log_lines(
----------------
Are you sure this is not racy (like, can the inferior terminate before we get a chance to interrupt it)?


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

https://reviews.llvm.org/D125575