[lldb] [llgs] Fix multi-resume bugs with nonstop mode

[mgorny]

Summary

Improve handling of multiple successive continue packets in non-stop
mode. More specifically:

1. Explicitly send error response (instead of crashing on assertion) if the user attempts to resume the same process twice. Since we do not support thread-level non-stop mode, one needs to always stop the process explicitly before resuming another thread set.

2. Actually stop the process if "vCont;t" is delivered to a running process. Similarly, we only support stopping all the running threads simultaneously and return an error if the action would result in a subset of threads still running.

With this patch, running multiple processes simultaneously is still
unsupported. The patch also employs a hack to avoid enabling stdio
forwarding on "vCont;t" packet. Both of these issues will be addressed
by followup patches.

Sponsored by: The FreeBSD Foundation

[mgorny]

Rebase on top of m_debugged_processes changes.

[labath]

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

1827–1830

I think this is going to be racy if new threads appear before we are able to stop everything. I mean that in the sense that if the client has listed all (known) threads of the process explicitly, and a new thread appears before we're able to act on it, then (I guess) the expected behavior would be to keep that thread running (but we won't do that).

Do we need to support this kind of stopping right now? Maybe we could only support the pid.-1 syntax for stopping all threads within a process? (which should also make the ResumeActionListStopsAllThreads logic simpler)

3002–3009

Could we factor even more of these functions into a common place? If we e.g. moved this block right before the m_continue_process->Resume, then we could put it, and into a helper function.

[mgorny]

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

1827–1830

No, I don't think we need to support it. I'll remove it then and simplify the logic.

3002–3009

Makes sense.

[mgorny]

Remove support for t with individual thread numbers and simplify code. Move resuming the process (and checking whether it can be resumed) into a common ResumeProcess() method.

[labath]

flake here: https://lab.llvm.org/buildbot/#/builders/68/builds/35932

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

protected:
   PacketResult Handle_QSetWorkingDir(StringExtractorGDBRemote &packet);
 
   PacketResult Handle_qGetWorkingDir(StringExtractorGDBRemote &packet);
 
   PacketResult Handle_QThreadSuffixSupported(StringExtractorGDBRemote &packet);
 
   PacketResult Handle_QListThreadsInStopReply(StringExtractorGDBRemote &packet);
 
+  PacketResult ResumeProcess(NativeProcessProtocol &process,
+                             const ResumeActionList &actions);
+
   PacketResult Handle_C(StringExtractorGDBRemote &packet);
 
   PacketResult Handle_c(StringExtractorGDBRemote &packet);
 
   PacketResult Handle_vCont(StringExtractorGDBRemote &packet);
 
   PacketResult Handle_vCont_actions(StringExtractorGDBRemote &packet);
 

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

 GDBRemoteCommunication::PacketResult
 GDBRemoteCommunicationServerLLGS::Handle_QListThreadsInStopReply(
     StringExtractorGDBRemote &packet) {
   m_list_threads_in_stop_reply = true;
   return SendOKResponse();
 }
 
 GDBRemoteCommunication::PacketResult
+GDBRemoteCommunicationServerLLGS::ResumeProcess(
+    NativeProcessProtocol &process, const ResumeActionList &actions) {
+  Log *log = GetLog(LLDBLog::Process | LLDBLog::Thread);
+
+  // In non-stop protocol mode, the process could be running already.
+  // We do not support resuming threads independently, so just error out.
+  if (!process.CanResume()) {
+    LLDB_LOG(log, "process {0} cannot be resumed (state={1})", process.GetID(),
+             process.GetState());
+    return SendErrorResponse(0x37);
+  }
+
+  Status error = process.Resume(actions);
+  if (error.Fail()) {
+    LLDB_LOG(log, "process {0} failed to resume: {1}", process.GetID(), error);
+    return SendErrorResponse(GDBRemoteServerError::eErrorResume);
+  }
+
+  LLDB_LOG(log, "process {0} resumed", process.GetID());
+
+  return PacketResult::Success;
+}
+
+GDBRemoteCommunication::PacketResult
 GDBRemoteCommunicationServerLLGS::Handle_C(StringExtractorGDBRemote &packet) {
   Log *log = GetLog(LLDBLog::Process | LLDBLog::Thread);
   LLDB_LOGF(log, "GDBRemoteCommunicationServerLLGS::%s called", __FUNCTION__);
 
   // Ensure we have a native process.
   if (!m_continue_process) {
     LLDB_LOGF(log,
               "GDBRemoteCommunicationServerLLGS::%s no debugged process "
if (packet.GetBytesLeft() > 0) {
     // FIXME add continue at address support for $C{signo}[;{continue-address}].
     if (*packet.Peek() == ';')
       return SendUnimplementedResponse(packet.GetStringRef().data());
     else
       return SendIllFormedResponse(
           packet, "unexpected content after $C{signal-number}");
   }
 
+  // In non-stop protocol mode, the process could be running already.
+  // We do not support resuming threads independently, so just error out.
+  if (!m_continue_process->CanResume()) {
+    LLDB_LOG(log, "process cannot be resumed (state={0})",
+             m_continue_process->GetState());
+    return SendErrorResponse(0x37);
+  }
+
   ResumeActionList resume_actions(StateType::eStateRunning,
                                   LLDB_INVALID_SIGNAL_NUMBER);
   Status error;
 
   // We have two branches: what to do if a continue thread is specified (in
   // which case we target sending the signal to that thread), or when we don't
   // have a continue thread set (in which case we send a signal to the
   // process).
if (signal_tid != LLDB_INVALID_THREAD_ID) {
     if (error.Fail()) {
       LLDB_LOG(log, "failed to send signal for process {0}: {1}",
                m_continue_process->GetID(), error);
 
       return SendErrorResponse(0x52);
     }
   }
 
-  // Resume the threads.
-  error = m_continue_process->Resume(resume_actions);
-  if (error.Fail()) {
-    LLDB_LOG(log, "failed to resume threads for process {0}: {1}",
-             m_continue_process->GetID(), error);
-
-    return SendErrorResponse(0x38);
-  }
+  // NB: this checks CanResume() twice but using a single code path for
+  // resuming still seems worth it.
+  PacketResult resume_res = ResumeProcess(*m_continue_process, resume_actions);
+  if (resume_res != PacketResult::Success)
+    return resume_res;
 
   // Don't send an "OK" packet, except in non-stop mode;
   // otherwise, the response is the stopped/exited message.
   return SendContinueSuccessResponse();
 }
 
 GDBRemoteCommunication::PacketResult
 GDBRemoteCommunicationServerLLGS::Handle_c(StringExtractorGDBRemote &packet) {
LLDB_LOGF(log,
               __FUNCTION__);
     return SendErrorResponse(0x36);
   }
 
   // Build the ResumeActionList
   ResumeActionList actions(StateType::eStateRunning,
                            LLDB_INVALID_SIGNAL_NUMBER);
 
-  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);
-  }
-
-  LLDB_LOG(log, "continued process {0}", m_continue_process->GetID());
+  PacketResult resume_res = ResumeProcess(*m_continue_process, actions);
+  if (resume_res != PacketResult::Success)
+    return resume_res;
 
   return SendContinueSuccessResponse();
 }
 
 GDBRemoteCommunication::PacketResult
 GDBRemoteCommunicationServerLLGS::Handle_vCont_actions(
     StringExtractorGDBRemote &packet) {
   StreamString response;
   response.Printf("vCont;c;C;s;S;t");
 
   return SendPacketNoLock(response.GetString());
 }
 
+static bool ResumeActionListStopsAllThreads(ResumeActionList &actions) {
+  // We're doing a stop-all if and only if our only action is a "t" for all
+  // threads.
+  if (const ResumeAction *default_action =
+          actions.GetActionForThread(LLDB_INVALID_THREAD_ID, false)) {
+    if (default_action->state == eStateSuspended && actions.GetSize() == 1)
+      return true;
+  }
+
+  return false;
+}
+
 GDBRemoteCommunication::PacketResult
 GDBRemoteCommunicationServerLLGS::Handle_vCont(
     StringExtractorGDBRemote &packet) {
   Log *log = GetLog(LLDBLog::Process);
   LLDB_LOGF(log, "GDBRemoteCommunicationServerLLGS::%s handling vCont packet",
             __FUNCTION__);
 
   packet.SetFilePos(::strlen("vCont"));
 
   if (packet.GetBytesLeft() == 0) {
     LLDB_LOGF(log,
               "GDBRemoteCommunicationServerLLGS::%s missing action from "
               "vCont package",
               __FUNCTION__);
     return SendIllFormedResponse(packet, "Missing action from vCont package");
   }
 
   if (::strcmp(packet.Peek(), ";s") == 0) {
     // Move past the ';', then do a simple 's'.
     packet.SetFilePos(packet.GetFilePos() + 1);
     return Handle_s(packet);
-  } else if (m_non_stop && ::strcmp(packet.Peek(), ";t") == 0) {
-    // TODO: add full support for "t" action
-    return SendOKResponse();
   }
 
   std::unordered_map<lldb::pid_t, ResumeActionList> thread_actions;
 
   while (packet.GetBytesLeft() && *packet.Peek() == ';') {
     // Skip the semi-colon.
     packet.GetChar();
 
if (packet.GetBytesLeft() && (*packet.Peek() == ':')) {
       auto pid_tid = packet.GetPidTid(StringExtractorGDBRemote::AllProcesses);
       if (!pid_tid)
         return SendIllFormedResponse(packet, "Malformed thread-id");
 
       pid = pid_tid->first;
       tid = pid_tid->second;
     }
 
+    if (thread_action.state == eStateSuspended &&
+        tid != StringExtractorGDBRemote::AllThreads) {
+      return SendIllFormedResponse(
+          packet, "'t' action not supported for individual threads");
+    }
+
     if (pid == StringExtractorGDBRemote::AllProcesses) {
       if (m_debugged_processes.size() > 1)
         return SendIllFormedResponse(
             packet, "Resuming multiple processes not supported yet");
       if (!m_continue_process) {
         LLDB_LOG(log, "no debugged process");
         return SendErrorResponse(0x36);
       }
GDBRemoteCommunicationServerLLGS::Handle_vCont(
   for (std::pair<lldb::pid_t, ResumeActionList> x : thread_actions) {
     auto process_it = m_debugged_processes.find(x.first);
     if (process_it == m_debugged_processes.end()) {
       LLDB_LOG(log, "vCont failed for process {0}: process not debugged",
                x.first);
       return SendErrorResponse(GDBRemoteServerError::eErrorResume);
     }
 
-    Status error = process_it->second.process_up->Resume(x.second);
+    // There are four possible scenarios here.  These are:
+    // 1. vCont on a stopped process that resumes at least one thread.
+    //    In this case, we call Resume().
+    // 2. vCont on a stopped process that leaves all threads suspended.
+    //    A no-op.
+    // 3. vCont on a running process that requests suspending all
+    //    running threads.  In this case, we call Interrupt().
+    // 4. vCont on a running process that requests suspending a subset
+    //    of running threads or resuming a subset of suspended threads.
+    //    Since we do not support full nonstop mode, this is unsupported
+    //    and we return an error.
+
+    assert(process_it->second.process_up);
+    if (ResumeActionListStopsAllThreads(x.second)) {
+      if (process_it->second.process_up->IsRunning()) {
+        assert(m_non_stop);
+
+        Status error = process_it->second.process_up->Interrupt();
         if (error.Fail()) {
-      LLDB_LOG(log, "vCont failed for process {0}: {1}", x.first, error);
+          LLDB_LOG(log, "vCont failed to halt process {0}: {1}", x.first,
+                   error);
           return SendErrorResponse(GDBRemoteServerError::eErrorResume);
         }
 
-    LLDB_LOG(log, "continued process {0}", x.first);
+        LLDB_LOG(log, "halted process {0}", x.first);
+
+        // hack to avoid enabling stdio forwarding after stop
+        // TODO: remove this when we improve stdio forwarding for nonstop
+        assert(thread_actions.size() == 1);
+        return SendOKResponse();
+      }
+    } else {
+      PacketResult resume_res =
+          ResumeProcess(*process_it->second.process_up, x.second);
+      if (resume_res != PacketResult::Success)
+        return resume_res;
+    }
   }
 
   return SendContinueSuccessResponse();
 }
 
 void GDBRemoteCommunicationServerLLGS::SetCurrentThreadID(lldb::tid_t tid) {
   Log *log = GetLog(LLDBLog::Thread);
   LLDB_LOG(log, "setting current thread id to {0}", tid);
if (!m_continue_process ||
       (m_continue_process->GetID() == LLDB_INVALID_PROCESS_ID)) {
     LLDB_LOGF(
         log,
         "GDBRemoteCommunicationServerLLGS::%s failed, no process available",
         __FUNCTION__);
     return SendErrorResponse(0x32);
   }
 
   // We first try to use a continue thread id.  If any one or any all set, use
   // the current thread. Bail out if we don't have a thread id.
   lldb::tid_t tid = GetContinueThreadID();
   if (tid == 0 || tid == LLDB_INVALID_THREAD_ID)
     tid = GetCurrentThreadID();
   if (tid == LLDB_INVALID_THREAD_ID)
     return SendErrorResponse(0x33);
 
   // Double check that we have such a thread.
   // TODO investigate: on MacOSX we might need to do an UpdateThreads () here.
   NativeThreadProtocol *thread = m_continue_process->GetThreadByID(tid);
   if (!thread)
     return SendErrorResponse(0x33);
 
   // Create the step action for the given thread.
   ResumeAction action = {tid, eStateStepping, LLDB_INVALID_SIGNAL_NUMBER};
 
   // Setup the actions list.
   ResumeActionList actions;
   actions.Append(action);
 
   // All other threads stop while we're single stepping a thread.
   actions.SetDefaultThreadActionIfNeeded(eStateStopped, 0);
-  Status error = m_continue_process->Resume(actions);
-  if (error.Fail()) {
-    LLDB_LOGF(log,
-              "GDBRemoteCommunicationServerLLGS::%s pid %" PRIu64
-              " tid %" PRIu64 " Resume() failed with error: %s",
-              __FUNCTION__, m_continue_process->GetID(), tid,
-              error.AsCString());
-    return SendErrorResponse(0x49);
-  }
+
+  PacketResult resume_res = ResumeProcess(*m_continue_process, actions);
+  if (resume_res != PacketResult::Success)
+    return resume_res;
 
   // No response here, unless in non-stop mode.
   // Otherwise, the stop or exit will come from the resulting action.
   return SendContinueSuccessResponse();
 }
 
 llvm::Expected<std::unique_ptr<llvm::MemoryBuffer>>
 GDBRemoteCommunicationServerLLGS::BuildTargetXml() {

lldb/test/API/tools/lldb-server/TestNonStop.py

def test_exit_query(self):
              "send packet: $OK#00",
              "send packet: %Stop:W00#00",
              "read packet: $?#00",
              "send packet: $W00#00",
              "read packet: $vStopped#00",
              "send packet: $OK#00",
              ], True)
         self.expect_gdbremote_sequence()
+
+    def multiple_resume_test(self, second_command):
+        self.build()
+        self.set_inferior_startup_launch()
+        procs = self.prep_debug_monitor_and_inferior(
+                inferior_args=["sleep:15"])
+        self.test_sequence.add_log_lines(
+            ["read packet: $QNonStop:1#00",
+             "send packet: $OK#00",
+             "read packet: $c#63",
+             "send packet: $OK#00",
+             "read packet: ${}#00".format(second_command),
+             "send packet: $E37#00",
+             ], True)
+        self.expect_gdbremote_sequence()
+
+    @add_test_categories(["llgs"])
+    def test_multiple_C(self):
+        self.multiple_resume_test("C05")
+
+    @add_test_categories(["llgs"])
+    def test_multiple_c(self):
+        self.multiple_resume_test("c")
+
+    @add_test_categories(["llgs"])
+    def test_multiple_s(self):
+        self.multiple_resume_test("s")
+
+    @expectedFailureAll(archs=["arm"])  # TODO
+    @expectedFailureAll(archs=["aarch64"],
+                        bugnumber="https://github.com/llvm/llvm-project/issues/56268")
+    @add_test_categories(["llgs"])
+    def test_multiple_vCont(self):
+        self.build()
+        self.set_inferior_startup_launch()
+        procs = self.prep_debug_monitor_and_inferior(
+                inferior_args=["thread:new", "trap", "sleep:15"])
+        self.test_sequence.add_log_lines(
+            ["read packet: $QNonStop:1#00",
+             "send packet: $OK#00",
+             "read packet: $c#63",
+             "send packet: $OK#00",
+             {"direction": "send",
+              "regex": r"^%Stop:T[0-9a-fA-F]{2}thread:([0-9a-fA-F]+);",
+              "capture": {1: "tid1"},
+              },
+             "read packet: $vStopped#63",
+             {"direction": "send",
+              "regex": r"^[$]T[0-9a-fA-F]{2}thread:([0-9a-fA-F]+);",
+              "capture": {1: "tid2"},
+              },
+             "read packet: $vStopped#63",
+             "send packet: $OK#00",
+             ], True)
+        ret = self.expect_gdbremote_sequence()
+
+        self.reset_test_sequence()
+        self.test_sequence.add_log_lines(
+            ["read packet: $vCont;c:{}#00".format(ret["tid1"]),
+             "send packet: $OK#00",
+             "read packet: $vCont;c:{}#00".format(ret["tid2"]),
+             "send packet: $E37#00",
+             ], True)
+        self.expect_gdbremote_sequence()
+
+    @add_test_categories(["llgs"])
+    def test_vCont_then_stop(self):
+        self.build()
+        self.set_inferior_startup_launch()
+        procs = self.prep_debug_monitor_and_inferior(
+                inferior_args=["sleep:15"])
+        self.test_sequence.add_log_lines(
+            ["read packet: $QNonStop:1#00",
+             "send packet: $OK#00",
+             "read packet: $c#63",
+             "send packet: $OK#00",
+             "read packet: $vCont;t#00",
+             "send packet: $OK#00",
+             ], True)
+        self.expect_gdbremote_sequence()
+
+    def vCont_then_partial_stop_test(self, run_both):
+        self.build()
+        self.set_inferior_startup_launch()
+        procs = self.prep_debug_monitor_and_inferior(
+                inferior_args=["thread:new", "trap", "sleep:15"])
+        self.test_sequence.add_log_lines(
+            ["read packet: $QNonStop:1#00",
+             "send packet: $OK#00",
+             "read packet: $c#63",
+             "send packet: $OK#00",
+             {"direction": "send",
+              "regex": r"^%Stop:T[0-9a-fA-F]{2}thread:([0-9a-fA-F]+);",
+              "capture": {1: "tid1"},
+              },
+             "read packet: $vStopped#63",
+             {"direction": "send",
+              "regex": r"^[$]T[0-9a-fA-F]{2}thread:([0-9a-fA-F]+);",
+              "capture": {1: "tid2"},
+              },
+             "read packet: $vStopped#63",
+             "send packet: $OK#00",
+             ], True)
+        ret = self.expect_gdbremote_sequence()
+
+        self.reset_test_sequence()
+        if run_both:
+            self.test_sequence.add_log_lines(
+                ["read packet: $vCont;c#00",
+                 ], True)
+        else:
+            self.test_sequence.add_log_lines(
+                ["read packet: $vCont;c:{}#00".format(ret["tid1"]),
+                 ], True)
+        self.test_sequence.add_log_lines(
+            ["send packet: $OK#00",
+             "read packet: $vCont;t:{}#00".format(ret["tid2"]),
+             "send packet: $E03#00",
+             ], True)
+        self.expect_gdbremote_sequence()
+
+    @expectedFailureAll(archs=["arm"])  # TODO
+    @expectedFailureAll(archs=["aarch64"],
+                        bugnumber="https://github.com/llvm/llvm-project/issues/56268")
+    @add_test_categories(["llgs"])
+    def test_vCont_then_partial_stop(self):
+        self.vCont_then_partial_stop_test(False)
+
+    @expectedFailureAll(archs=["arm"])  # TODO
+    @expectedFailureAll(archs=["aarch64"],
+                        bugnumber="https://github.com/llvm/llvm-project/issues/56268")
+    @add_test_categories(["llgs"])
+    def test_vCont_then_partial_stop_run_both(self):
+        self.vCont_then_partial_stop_test(True)

lldb/test/API/tools/lldb-server/vCont-threads/TestPartialResume.py

def test_vCont_cxcxt(self):
         main_thread, all_subthreads_list = (
             self.start_vCont_run_subset_of_threads_test())
         # resume two threads explicitly, stop others explicitly
         self.assertEqual(
             self.continue_and_get_threads_running(
                 main_thread,
                 "c:{:x};c:{:x};t".format(*all_subthreads_list[:2])),
             set(all_subthreads_list[:2]))
-
-    @skipIfWindows
-    @add_test_categories(["llgs"])
-    def test_vCont_txc(self):
-        main_thread, all_subthreads_list = (
-            self.start_vCont_run_subset_of_threads_test())
-        # stop one thread explicitly, resume others
-        self.assertEqual(
-            self.continue_and_get_threads_running(
-                main_thread,
-                "t:{:x};c".format(all_subthreads_list[-1])),
-            set(all_subthreads_list[:2]))
-
-    @skipIfWindows
-    @add_test_categories(["llgs"])
-    def test_vCont_cxtxc(self):
-        main_thread, all_subthreads_list = (
-            self.start_vCont_run_subset_of_threads_test())
-        # resume one thread explicitly, stop one explicitly,
-        # resume others
-        self.assertEqual(
-            self.continue_and_get_threads_running(
-                main_thread,
-                "c:{:x};t:{:x};c".format(*all_subthreads_list[-2:])),
-            set(all_subthreads_list[:2]))
-
-    @skipIfWindows
-    @add_test_categories(["llgs"])
-    def test_vCont_txcx(self):
-        main_thread, all_subthreads_list = (
-            self.start_vCont_run_subset_of_threads_test())
-        # resume one thread explicitly, stop one explicitly,
-        # stop others implicitly
-        self.assertEqual(
-            self.continue_and_get_threads_running(
-                main_thread,
-                "t:{:x};c:{:x}".format(*all_subthreads_list[:2])),
-            set(all_subthreads_list[1:2]))
-
-    @skipIfWindows
-    @add_test_categories(["llgs"])
-    def test_vCont_txcxt(self):
-        main_thread, all_subthreads_list = (
-            self.start_vCont_run_subset_of_threads_test())
-        # resume one thread explicitly, stop one explicitly,
-        # stop others explicitly
-        self.assertEqual(
-            self.continue_and_get_threads_running(
-                main_thread,
-                "t:{:x};c:{:x};t".format(*all_subthreads_list[:2])),
-            set(all_subthreads_list[1:2]))