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

[Pavel Labath via Phabricator via lldb-commits]

labath accepted this revision.
labath added a comment.
This revision is now accepted and ready to land.

Ok, looks good. Some extra suggestions inline.



================
Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:490
+                                        response.GetString());
+  else
+    return SendPacketNoLock(response.GetString());
----------------
no else after return


================
Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:1592-1602
   Status error = m_continue_process->Resume(actions);
   if (error.Fail()) {
     LLDB_LOG(log, "c failed for process {0}: {1}", m_continue_process->GetID(),
              error);
     return SendErrorResponse(GDBRemoteServerError::eErrorResume);
   }
 
----------------
Could we have a helper function for this, and make all the continue-like actions go through that?


================
Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:1753
+    // notifications.
+    for (auto it = m_stop_notification_queue.begin();
+         it != m_stop_notification_queue.end();) {
----------------
`llvm::erase_if(m_stop_notification_queue, []{ return !begins_with_W_or_X;});`


================
Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:3264
   // Notify we attached by sending a stop packet.
-  return SendStopReasonForState(m_current_process->GetState());
+  return SendStopReasonForState(m_current_process->GetState(), false);
 }
----------------
add `/*force_synchronous=*/` here and elsewhere.


================
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())
----------------
mgorny wrote:
> labath wrote:
> > Is this correct? I would expect to this to first read (`front()`) the packet before popping it.
> Yes, it is. I suppose it could be a bit misleading. The first message is sent as asynchronous notification but we keep it on the queue (and technically, the protocol assumes we may resend the asynchronous notification if we think client may have not gotten it). `vStopped` means client has processed it and is ready for the next one, so we pop the one that's been sent already and send the next one, wait for the next ACK and so on.
Yeah, it is confusing. Having a comment (with the explanation you just made) would help a lot. However, if you don't see any use case for the resending, we could also delete it, and use the standard queue pattern. Up to you...


================
Comment at: lldb/test/API/tools/lldb-server/TestLldbGdbServer.py:1443
+            m = self.expect_gdbremote_sequence()
+            del m["O_content"]
+            threads = [m]
----------------
mgorny wrote:
> labath wrote:
> > Is this necessary? (I get that it's not useful, but it seems weird...)
> It's convenient because it lets us compare the matches from `threads` and `threads_verify`.
Ok, I suppose I can live with that.


================
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(
----------------
mgorny wrote:
> labath wrote:
> > Are you sure this is not racy (like, can the inferior terminate before we get a chance to interrupt it)?
> Well, if I'm reading the code right, the new thread waits 60 seconds, so yes, technically it's racy ;-). Do you have a better idea? Some new argument that puts the process in an infinite sleep loop?
That's ok, a 60s wait is fine. I forgot there was a default sleep action -- I though the process would quit immediately...


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

https://reviews.llvm.org/D125575