[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

1818–1821

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)

3003–3010

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

1818–1821

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

3003–3010

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.cpp

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).
GDBRemoteCommunicationServerLLGS::Handle_c(StringExtractorGDBRemote &packet) {
   if (!m_continue_process) {
     LLDB_LOGF(log,
               "GDBRemoteCommunicationServerLLGS::%s no debugged process "
               "shared pointer",
               __FUNCTION__);
     return SendErrorResponse(0x36);
   }
 
+  // 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);
+  }
+
   // 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);

 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,
+                                            NativeProcessProtocol &process) {
+  for (NativeThreadProtocol &thread : process.Threads()) {
+    const ResumeAction *action =
+        actions.GetActionForThread(thread.GetID(), true);
+    // If there's at least one non-"t" action, we're not stopping them all.
+    if (action && action->state != eStateSuspended)
+      return false;
+    // If there's no action (i.e. no change) and the thread is running
+    // currently, it would remain running.
+    if (!action && thread.GetState() != eStateStopped &&
+        thread.GetState() != eStateSuspended)
+      return false;
+  }
+  return true;
+}
+
 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();
 
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);
     }
 
+    // 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,
+                                        *process_it->second.process_up)) {
+      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 to halt process {0}: {1}", x.first,
+                   error);
+          return SendErrorResponse(GDBRemoteServerError::eErrorResume);
+        }
+
+        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 {
+      if (!process_it->second.process_up->CanResume()) {
+        LLDB_LOG(log, "process cannot be resumed (pid={0}, state={1})",
+                 process_it->first, process_it->second.process_up->GetState());
+        return SendErrorResponse(0x37);
+      }
+
       Status error = process_it->second.process_up->Resume(x.second);
       if (error.Fail()) {
         LLDB_LOG(log, "vCont failed for process {0}: {1}", x.first, error);
         return SendErrorResponse(GDBRemoteServerError::eErrorResume);
       }
 
       LLDB_LOG(log, "continued process {0}", x.first);
     }
+  }
 
   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);
   }
 
+  // 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);
+  }
+
   // 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);
 

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: $E37#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)