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

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 (via -1) and return an error in any other case.

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 are addressed
by followup patches.

Sponsored by: The FreeBSD Foundation
Differential Revision: https://reviews.llvm.org/D128710

4 files changed, 235 insertions, 85 deletions

diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
index a2ea53b0e8e1..8edb49f06a33 100644
--- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
+++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
@@ -1513,6 +1513,30 @@ GDBRemoteCommunicationServerLLGS::Handle_QListThreadsInStopReply(
 }
 
 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__);
@@ -1547,6 +1571,14 @@ GDBRemoteCommunicationServerLLGS::Handle_C(StringExtractorGDBRemote &packet) {
           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;
@@ -1580,14 +1612,11 @@ GDBRemoteCommunicationServerLLGS::Handle_C(StringExtractorGDBRemote &packet) {
     }
   }
 
-  // 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.
@@ -1622,14 +1651,9 @@ GDBRemoteCommunicationServerLLGS::Handle_c(StringExtractorGDBRemote &packet) {
   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();
 }
@@ -1643,6 +1667,18 @@ GDBRemoteCommunicationServerLLGS::Handle_vCont_actions(
   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) {
@@ -1664,9 +1700,6 @@ GDBRemoteCommunicationServerLLGS::Handle_vCont(
     // 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;
@@ -1733,6 +1766,12 @@ GDBRemoteCommunicationServerLLGS::Handle_vCont(
       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(
@@ -1765,13 +1804,43 @@ GDBRemoteCommunicationServerLLGS::Handle_vCont(
       return SendErrorResponse(GDBRemoteServerError::eErrorResume);
     }
 
-    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);
-    }
+    // 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 to halt process {0}: {1}", x.first,
+                   error);
+          return SendErrorResponse(GDBRemoteServerError::eErrorResume);
+        }
+
+        LLDB_LOG(log, "halted process {0}", x.first);
 
-    LLDB_LOG(log, "continued 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();
@@ -2940,15 +3009,10 @@ GDBRemoteCommunicationServerLLGS::Handle_s(StringExtractorGDBRemote &packet) {
 
   // 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.
diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h
index ebd656687da9..47e8a783603d 100644
--- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h
+++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h
@@ -155,6 +155,9 @@ protected:
 
   PacketResult Handle_QListThreadsInStopReply(StringExtractorGDBRemote &packet);
 
+  PacketResult ResumeProcess(NativeProcessProtocol &process,
+                             const ResumeActionList &actions);
+
   PacketResult Handle_C(StringExtractorGDBRemote &packet);
 
   PacketResult Handle_c(StringExtractorGDBRemote &packet);
diff --git a/lldb/test/API/tools/lldb-server/TestNonStop.py b/lldb/test/API/tools/lldb-server/TestNonStop.py
index 83772ada78e3..d544cdd45c48 100644
--- a/lldb/test/API/tools/lldb-server/TestNonStop.py
+++ b/lldb/test/API/tools/lldb-server/TestNonStop.py
@@ -168,3 +168,137 @@ class LldbGdbServerTestCase(gdbremote_testcase.GdbRemoteTestCaseBase):
              "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)
diff --git a/lldb/test/API/tools/lldb-server/vCont-threads/TestPartialResume.py b/lldb/test/API/tools/lldb-server/vCont-threads/TestPartialResume.py
index 2cc60d3d0a5c..b98b63ea22e0 100644
--- a/lldb/test/API/tools/lldb-server/vCont-threads/TestPartialResume.py
+++ b/lldb/test/API/tools/lldb-server/vCont-threads/TestPartialResume.py
@@ -75,54 +75,3 @@ class TestPartialResume(gdbremote_testcase.GdbRemoteTestCaseBase):
                 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]))