[lldb] [gdb-remote] Ensure that all threads stop in non-stop mode

[mgorny]

Summary

Issue a vCont;t request after getting an asynchronous stop
notification in non-stop mode in order to ensure that all threads are
stopped. This is needed for servers implementing thread-level non-stop
mode (e.g. gdbserver). Unlike the current version of LLGS, these
servers do not stop all threads whenever one of them stops.

Depending on whether any threads were running at the time, the vCont;t
packet may or may not yield an asynchronous stop notification. Rather
than attempting to wait on that, use a combination of ?
and qfThreadInfo packets to determine whether all threads are stopped.

Issue a ? request first in order to obtain stop reasons for all
the stopped threads. As a bonus, this request clears the asynchronous
notification queue for any notification vCont;t may have produced.
Afterwards, obtain the complete thread list using qfThreadInfo.
If all debugged threads had stop reasons, it means that they all stopped
already and we can proceed. Otherwise, try again.

This logic should also be handle to handle the corner case of a new
thread being started after the vCont;t request.

We limit the stop attempts to 25 to avoid hanging when communicating
with a broken server.

Sponsored by: The FreeBSD Foundation

[mgorny]

Remove leftover debug hack.

lldb/packages/Python/lldbsuite/test/gdbclientutils.py

def respond(self, packet):
         if packet.startswith("QNonStop:"):
             return self.QNonStop(int(packet.split(":", 1)[1]))
         if packet == "vStdio":
             return self.vStdio()
         if packet == "vStopped":
             return self.vStopped()
         if packet == "vCtrlC":
             return self.vCtrlC()
+        if packet == "vCont;t":
+            return self.vContT()
         if packet == "k":
             return self.k()
 
         return self.other(packet)
 
     def qsProcessInfo(self):
         return "E04"
 
def vStdio(self):
         return ""
 
     def vStopped(self):
         return ""
 
     def vCtrlC(self):
         return ""
 
+    def vContT(self):
+        return ""
+
     def k(self):
         return ["W01", self.RESPONSE_DISCONNECT]
 
     """
     Raised when we receive a packet for which there is no default action.
     Override the responder class to implement behavior suitable for the test at
     hand.
     """

lldb/source/Plugins/Process/gdb-remote/GDBRemoteClientBase.h

public:
   };
 
   GDBRemoteClientBase(const char *comm_name, const char *listener_name);
 
   bool SendAsyncSignal(int signo, std::chrono::seconds interrupt_timeout);
 
   bool Interrupt(std::chrono::seconds interrupt_timeout);
 
+  std::vector<std::pair<lldb::pid_t, lldb::tid_t>>
+  GetCurrentProcessAndThreadIDsNoLock();
+
   lldb::StateType SendContinuePacketAndWaitForResponse(
       ContinueDelegate &delegate, const UnixSignals &signals,
       llvm::StringRef payload, std::chrono::seconds interrupt_timeout,
       StringExtractorGDBRemote &response);
 
   // If interrupt_timeout == 0 seconds, don't interrupt the target.
   // Only send the packet if the target is stopped.
   // If you want to use this mode, use the fact that the timeout is defaulted

lldb/source/Plugins/Process/gdb-remote/GDBRemoteClientBase.cpp

 //===-- GDBRemoteClientBase.cpp -------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "GDBRemoteClientBase.h"
 
+#include <chrono>
+#include <thread>
+#include <unordered_set>
+
 #include "llvm/ADT/StringExtras.h"
 
 #include "lldb/Target/UnixSignals.h"
 #include "lldb/Utility/LLDBAssert.h"
 
 #include "ProcessGDBRemoteLog.h"
 
 using namespace lldb;

 
 GDBRemoteClientBase::ContinueDelegate::~ContinueDelegate() = default;
 
 GDBRemoteClientBase::GDBRemoteClientBase(const char *comm_name,
                                          const char *listener_name)
     : GDBRemoteCommunication(comm_name, listener_name), m_async_count(0),
       m_is_running(false), m_should_stop(false) {}
 
+static llvm::Optional<std::pair<lldb::pid_t, lldb::tid_t>>
+GetThreadIDFromStopPacket(StringExtractor& packet) {
+  // Only stops are of interest to us, and only T stops carry thread id
+  if (packet.GetChar() != 'T')
+    return llvm::None;
+  // Skip signo
+  packet.GetHexU8();
+
+  llvm::StringRef key, value;
+  while (packet.GetNameColonValue(key, value)) {
+    if (key != "thread")
+      continue;
+    return StringExtractorGDBRemote(value).GetPidTid(LLDB_INVALID_PROCESS_ID);
+  }
+
+  return llvm::None;
+}
+
+std::vector<std::pair<lldb::pid_t, lldb::tid_t>>
+GDBRemoteClientBase::GetCurrentProcessAndThreadIDsNoLock() {
+  std::vector<std::pair<lldb::pid_t, lldb::tid_t>> ids;
+  StringExtractorGDBRemote response;
+
+  PacketResult packet_result;
+  for (packet_result =
+           SendPacketAndWaitForResponseNoLock("qfThreadInfo", response);
+       packet_result == PacketResult::Success && response.IsNormalResponse();
+       packet_result =
+           SendPacketAndWaitForResponseNoLock("qsThreadInfo", response)) {
+    char ch = response.GetChar();
+    if (ch == 'l')
+      break;
+    if (ch == 'm') {
+      do {
+        auto pid_tid = response.GetPidTid(LLDB_INVALID_PROCESS_ID);
+        // If we get an invalid response, break out of the loop.
+        // If there are valid tids, they have been added to ids.
+        // If there are no valid tids, we'll fall through to the
+        // bare-iron target handling below.
+        if (!pid_tid)
+          break;
+
+        ids.push_back(*pid_tid);
+        ch = response.GetChar(); // Skip the command separator
+      } while (ch == ',');       // Make sure we got a comma separator
+    }
+  }
+
+  /*
+   * Connected bare-iron target (like YAMON gdb-stub) may not have support for
+   * qProcessInfo, qC and qfThreadInfo packets. The reply from '?' packet
+   * could
+   * be as simple as 'S05'. There is no packet which can give us pid and/or
+   * tid.
+   * Assume pid=tid=1 in such cases.
+   */
+  if ((response.IsUnsupportedResponse() || response.IsNormalResponse()) &&
+      ids.size() == 0 && IsConnected()) {
+    ids.emplace_back(1, 1);
+  }
+
+  return ids;
+}
+
 StateType GDBRemoteClientBase::SendContinuePacketAndWaitForResponse(
     ContinueDelegate &delegate, const UnixSignals &signals,
     llvm::StringRef payload, std::chrono::seconds interrupt_timeout,
     StringExtractorGDBRemote &response) {
   Log *log = GetLog(GDBRLog::Process);
   response.Clear();
 
   {
for (;;) {
     if (response.Empty())
       return eStateInvalid;
 
     LLDB_LOG(log, "got result {0}, packet: {1}", read_result,
              response.GetStringRef());
 
     // We do not currently support full asynchronous communication.  Instead,
     // when in non-stop mode, we wait for the asynchronous %Stop notification
-    // and then drain the notification queue
-    // TODO: issue vCont;t to ensure that all threads have actually stopped
-    // (this is not needed for LLGS but for gdbserver)
+    // and then drain the notification queue.  Additionally, we issue
+    // a "vCont;t" request to stop all threads in case we were dealing
+    // with a true non-stop server.
 
     if (read_result == PacketResult::Notify &&
         response.GetStringRef().startswith("Stdio:")) {
       std::string inferior_stdout;
       response.SetFilePos(6);
       while (!response.IsOKResponse()) {
         if (response.GetChar() == 'O') {
           response.GetHexByteString(inferior_stdout);
for (;;) {
     }
 
     if (read_result == PacketResult::Notify &&
         response.GetStringRef().startswith("Stop:")) {
       response.Reset(response.GetStringRef().substr(5));
 
       if (!DrainNotificationQueue("vStopped"))
         return eStateInvalid;
+
+      if (response.GetStringRef()[0] == 'T') {
+        auto main_pidtid = GetThreadIDFromStopPacket(response);
+        if (!main_pidtid.hasValue()) {
+          LLDB_LOG(log, "T stop reply without thread-id in non-stop mode");
+          return eStateInvalid;
+        }
+        response.SetFilePos(0);
+
+        for (int attempt = 0; attempt < 25; ++attempt) {
+          // Attempt to stop all remaining threads via vCont;t (if any).
+          StringExtractorGDBRemote stop_response;
+          if (SendPacketAndWaitForResponseNoLock("vCont;t", stop_response) !=
+              PacketResult::Success || !stop_response.IsOKResponse()) {
+            LLDB_LOG(log, "vCont;t failed");
+            return eStateInvalid;
+          }
+
+          std::unordered_set<lldb::tid_t> all_threads;
+          std::unordered_set<lldb::tid_t> stopped_threads;
+
+          // Issue a '?' command to get a list of all threads that have
+          // stopped.
+          // Note that '?' clears the vStopped notification queue.
+          const char *cmd = "?";
+          for (;;) {
+            if (SendPacketAndWaitForResponseNoLock(cmd, stop_response) !=
+                PacketResult::Success) {
+              LLDB_LOG(log, "vCont;t failed");
+              return eStateInvalid;
+            }
+
+            // We iterate until we get an "OK" response.
+            if (stop_response.IsOKResponse())
+              break;
+
+            auto stopped_pidtid = GetThreadIDFromStopPacket(stop_response);
+            if (stopped_pidtid.hasValue() &&
+                stopped_pidtid->first == main_pidtid->first)
+              stopped_threads.insert(stopped_pidtid->second);
+
+            cmd = "vStopped";
+          }
+
+          // Collect the list of all threads.
+          for (auto pidtid : GetCurrentProcessAndThreadIDsNoLock()) {
+            if (pidtid.first == main_pidtid->first)
+              all_threads.insert(pidtid.second);
+          }
+
+          for (auto x : stopped_threads)
+            all_threads.erase(x);
+          // If all threads were stopped, we're done.  Otherwise, try another
+          // iteration.
+          if (all_threads.empty())
+            break;
+
+          // Wait a little not to spam the server too fast.
+          std::this_thread::sleep_for(std::chrono::milliseconds(200));
+        }
+
+        // TODO: do we fail if all attempts failed?
+      }
     }
 
     const char stop_type = response.GetChar();
 
     switch (stop_type) {
     case 'W':
     case 'X':
       return eStateExited;

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp

uint8_t GDBRemoteCommunicationClient::SendGDBStoppointTypePacket(
   }
   // Signal generic failure
   return UINT8_MAX;
 }
 
 std::vector<std::pair<lldb::pid_t, lldb::tid_t>>
 GDBRemoteCommunicationClient::GetCurrentProcessAndThreadIDs(
     bool &sequence_mutex_unavailable) {
-  std::vector<std::pair<lldb::pid_t, lldb::tid_t>> ids;
-
   Lock lock(*this);
   if (lock) {
     sequence_mutex_unavailable = false;
-    StringExtractorGDBRemote response;
-
-    PacketResult packet_result;
-    for (packet_result =
-             SendPacketAndWaitForResponseNoLock("qfThreadInfo", response);
-         packet_result == PacketResult::Success && response.IsNormalResponse();
-         packet_result =
-             SendPacketAndWaitForResponseNoLock("qsThreadInfo", response)) {
-      char ch = response.GetChar();
-      if (ch == 'l')
-        break;
-      if (ch == 'm') {
-        do {
-          auto pid_tid = response.GetPidTid(LLDB_INVALID_PROCESS_ID);
-          // If we get an invalid response, break out of the loop.
-          // If there are valid tids, they have been added to ids.
-          // If there are no valid tids, we'll fall through to the
-          // bare-iron target handling below.
-          if (!pid_tid)
-            break;
-
-          ids.push_back(*pid_tid);
-          ch = response.GetChar(); // Skip the command separator
-        } while (ch == ',');       // Make sure we got a comma separator
-      }
-    }
+    return GetCurrentProcessAndThreadIDsNoLock();
 
-    /*
-     * Connected bare-iron target (like YAMON gdb-stub) may not have support for
-     * qProcessInfo, qC and qfThreadInfo packets. The reply from '?' packet
-     * could
-     * be as simple as 'S05'. There is no packet which can give us pid and/or
-     * tid.
-     * Assume pid=tid=1 in such cases.
-     */
-    if ((response.IsUnsupportedResponse() || response.IsNormalResponse()) &&
-        ids.size() == 0 && IsConnected()) {
-      ids.emplace_back(1, 1);
-    }
-  } else {
-    Log *log(GetLog(GDBRLog::Process | GDBRLog::Packets));
-    LLDB_LOG(log, "error: failed to get packet sequence mutex, not sending "
-                  "packet 'qfThreadInfo'");
-    sequence_mutex_unavailable = true;
-  }
-
-  return ids;
+  return {};
 }
 
 size_t GDBRemoteCommunicationClient::GetCurrentThreadIDs(
     std::vector<lldb::tid_t> &thread_ids, bool &sequence_mutex_unavailable) {
   lldb::pid_t pid = GetCurrentProcessID();
   thread_ids.clear();
 
   auto ids = GetCurrentProcessAndThreadIDs(sequence_mutex_unavailable);

lldb/test/API/functionalities/gdb_remote_client/TestNonStop.py

def test_run(self):
                 self.vStopped_counter += 1
                 return ("OK" if self.vStopped_counter > 1
                         else "T00thread:p10.10;")
 
             def cont(self):
                 self.vStopped_counter = 0
                 return ["OK", "%Stop:T02thread:p10.12;"]
 
+            def vContT(self):
+                return "OK"
+
         self.dbg.HandleCommand(
             "settings set plugin.process.gdb-remote.use-non-stop-protocol true")
         self.addTearDownHook(lambda:
             self.runCmd(
                 "settings set plugin.process.gdb-remote.use-non-stop-protocol "
                 "false"))
         self.server.responder = MyResponder()
         target = self.dbg.CreateTarget("")
def test_stdio(self):
                         else "T00thread:p10.10;")
 
             def cont(self):
                 self.vStopped_counter = 0
                 self.vStdio_counter = 0
                 return ["OK",
                         "%Stdio:O6669727374206c696e650d0a",]
 
+            def vContT(self):
+                return "OK"
+
         self.dbg.HandleCommand(
             "settings set plugin.process.gdb-remote.use-non-stop-protocol true")
         self.addTearDownHook(lambda:
             self.runCmd(
                 "settings set plugin.process.gdb-remote.use-non-stop-protocol "
                 "false"))
         self.server.responder = MyResponder()
         target = self.dbg.CreateTarget("")
def test_vCtrlC(self):
                         else "T00thread:p10.10;")
 
             def cont(self):
                 return ["OK"]
 
             def vCtrlC(self):
                 return ["OK", "%Stop:T00thread:p10.10;"]
 
+            def vContT(self):
+                return "OK"
+
         self.dbg.HandleCommand(
             "settings set plugin.process.gdb-remote.use-non-stop-protocol true")
         self.addTearDownHook(lambda:
             self.runCmd(
                 "settings set plugin.process.gdb-remote.use-non-stop-protocol "
                 "false"))
         self.server.responder = MyResponder()
         target = self.dbg.CreateTarget("")
         process = self.connect(target)
         self.assertPacketLogContains(["QNonStop:1"])
 
         self.dbg.SetAsync(True)
         lldbutil.expect_state_changes(self, self.dbg.GetListener(), process,
                                       [lldb.eStateStopped])
         process.Continue()
         lldbutil.expect_state_changes(self, self.dbg.GetListener(), process,
                                       [lldb.eStateRunning])
         process.Stop()
         lldbutil.expect_state_changes(self, self.dbg.GetListener(), process,
                                       [lldb.eStateStopped])
         self.assertPacketLogContains(["vStopped"])
         self.assertEqual(process.GetSelectedThread().GetStopReason(),
                          lldb.eStopReasonNone)
+
+    def test_nonstop_server(self):
+        class MyResponder(NonStopResponder):
+            vCont_t_status = 0
+
+            def vStopped(self):
+                if self.vCont_t_status == 2:
+                    self.vCont_t_status += 1
+                    return "T02thread:p10.12;"
+                return "OK"
+
+            def cont(self):
+                return ["OK", "%Stop:T02thread:p10.12;"]
+
+            def vContT(self):
+                self.vCont_t_status = 1
+                return ["OK", "%Stop:T02thread:p10.10;"]
+
+            def haltReason(self):
+                if self.vCont_t_status == 1:
+                    self.vCont_t_status += 1
+                    return "T02thread:p10.10;"
+                return "S02"
+
+        self.dbg.HandleCommand(
+            "settings set plugin.process.gdb-remote.use-non-stop-protocol true")
+        self.addTearDownHook(lambda:
+            self.runCmd(
+                "settings set plugin.process.gdb-remote.use-non-stop-protocol "
+                "false"))
+        self.server.responder = MyResponder()
+        target = self.dbg.CreateTarget("")
+        process = self.connect(target)
+        self.assertPacketLogContains(["QNonStop:1"])
+
+        process.Continue()
+        self.assertPacketLogContains(["vStopped", "vCont;t"])
+        self.assertEqual(process.GetSelectedThread().GetStopReason(),
+                         lldb.eStopReasonSignal)
+        self.assertEqual(process.GetSelectedThread().GetStopDescription(100),
+                         "signal SIGINT")
+        self.assertEqual(self.server.responder.vCont_t_status, 3)
+
+    def test_nonstop_server_non_immediate(self):
+        class MyResponder(NonStopResponder):
+            vCont_t_status = 0
+
+            def vStopped(self):
+                if self.vCont_t_status == 2:
+                    return ["OK", "%Stop:T02thread:p10.10;"]
+                if self.vCont_t_status == 3:
+                    self.vCont_t_status += 1
+                    return "T02thread:p10.10;"
+                return "OK"
+
+            def cont(self):
+                return ["OK", "%Stop:T02thread:p10.12;"]
+
+            def vContT(self):
+                if self.vCont_t_status == 0:
+                    self.vCont_t_status = 1
+                return "OK"
+
+            def haltReason(self):
+                if self.vCont_t_status in (1, 2):
+                    self.vCont_t_status += 1
+                    return "T02thread:p10.12;"
+                return "S02"
+
+        self.dbg.HandleCommand(
+            "settings set plugin.process.gdb-remote.use-non-stop-protocol true")
+        self.addTearDownHook(lambda:
+            self.runCmd(
+                "settings set plugin.process.gdb-remote.use-non-stop-protocol "
+                "false"))
+        self.server.responder = MyResponder()
+        target = self.dbg.CreateTarget("")
+        process = self.connect(target)
+        self.assertPacketLogContains(["QNonStop:1"])
+
+        process.Continue()
+        self.assertPacketLogContains(["vStopped", "vCont;t"])
+        self.assertEqual(process.GetSelectedThread().GetStopReason(),
+                         lldb.eStopReasonSignal)
+        self.assertEqual(process.GetSelectedThread().GetStopDescription(100),
+                         "signal SIGINT")
+        self.assertEqual(self.server.responder.vCont_t_status, 4)