[lldb] [llgs] Support resuming one process with PID!=current via vCont

Extend vCont function to support resuming a process with an arbitrary
PID, that could be different than the one selected via Hc (or no process
at all may be selected).  Resuming more than one process simultaneously
is not supported yet.

Remove the ReadTid() method that was only used by Handle_vCont(),
and furthermore it was wrongly using m_current_process rather than
m_continue_process.

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

Author: mgorny

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

@@ -1674,12 +1674,7 @@ GDBRemoteCommunicationServerLLGS::Handle_vCont(
     return SendIllFormedResponse(packet, "Missing action from vCont package");
   }
 
-  // Check if this is all continue (no options or ";c").
-  if (::strcmp(packet.Peek(), ";c") == 0) {
-    // Move past the ';', then do a simple 'c'.
-    packet.SetFilePos(packet.GetFilePos() + 1);
-    return Handle_c(packet);
-  } else if (::strcmp(packet.Peek(), ";s") == 0) {
+  if (::strcmp(packet.Peek(), ";s") == 0) {
     // Move past the ';', then do a simple 's'.
     packet.SetFilePos(packet.GetFilePos() + 1);
     return Handle_s(packet);
@@ -1688,13 +1683,7 @@ GDBRemoteCommunicationServerLLGS::Handle_vCont(
     return SendOKResponse();
   }
 
-  // Ensure we have a native process.
-  if (!m_continue_process) {
-    LLDB_LOG(log, "no debugged process");
-    return SendErrorResponse(0x36);
-  }
-
-  ResumeActionList thread_actions;
+  std::unordered_map<lldb::pid_t, ResumeActionList> thread_actions;
 
   while (packet.GetBytesLeft() && *packet.Peek() == ';') {
     // Skip the semi-colon.
@@ -1737,32 +1726,62 @@ GDBRemoteCommunicationServerLLGS::Handle_vCont(
       break;
     }
 
+    lldb::pid_t pid = StringExtractorGDBRemote::AllProcesses;
+    lldb::tid_t tid = StringExtractorGDBRemote::AllThreads;
+
     // Parse out optional :{thread-id} value.
     if (packet.GetBytesLeft() && (*packet.Peek() == ':')) {
       // Consume the separator.
       packet.GetChar();
 
-      llvm::Expected<lldb::tid_t> tid_ret =
-          ReadTid(packet, /*allow_all=*/true, m_continue_process->GetID());
-      if (!tid_ret)
-        return SendErrorResponse(tid_ret.takeError());
+      auto pid_tid = packet.GetPidTid(StringExtractorGDBRemote::AllProcesses);
+      if (!pid_tid)
+        return SendIllFormedResponse(packet, "Malformed thread-id");
 
-      thread_action.tid = tid_ret.get();
-      if (thread_action.tid == StringExtractorGDBRemote::AllThreads)
-        thread_action.tid = LLDB_INVALID_THREAD_ID;
+      pid = pid_tid->first;
+      tid = pid_tid->second;
     }
 
-    thread_actions.Append(thread_action);
-  }
+    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);
+      }
+      pid = m_continue_process->GetID();
+    }
 
-  Status error = m_continue_process->Resume(thread_actions);
-  if (error.Fail()) {
-    LLDB_LOG(log, "vCont failed for process {0}: {1}",
-             m_continue_process->GetID(), error);
-    return SendErrorResponse(GDBRemoteServerError::eErrorResume);
+    if (tid == StringExtractorGDBRemote::AllThreads)
+      tid = LLDB_INVALID_THREAD_ID;
+
+    thread_action.tid = tid;
+
+    thread_actions[pid].Append(thread_action);
   }
 
-  LLDB_LOG(log, "continued process {0}", m_continue_process->GetID());
+  assert(thread_actions.size() >= 1);
+  if (thread_actions.size() > 1)
+    return SendIllFormedResponse(
+        packet, "Resuming multiple processes not supported yet");
+
+  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->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();
 }
@@ -4011,38 +4030,6 @@ std::string GDBRemoteCommunicationServerLLGS::XMLEncodeAttributeValue(
   return result;
 }
 
-llvm::Expected<lldb::tid_t> GDBRemoteCommunicationServerLLGS::ReadTid(
-    StringExtractorGDBRemote &packet, bool allow_all, lldb::pid_t default_pid) {
-  assert(m_current_process);
-  assert(m_current_process->GetID() != LLDB_INVALID_PROCESS_ID);
-
-  auto pid_tid = packet.GetPidTid(default_pid);
-  if (!pid_tid)
-    return llvm::make_error<StringError>(inconvertibleErrorCode(),
-                                         "Malformed thread-id");
-
-  lldb::pid_t pid = pid_tid->first;
-  lldb::tid_t tid = pid_tid->second;
-
-  if (!allow_all && pid == StringExtractorGDBRemote::AllProcesses)
-    return llvm::make_error<StringError>(
-        inconvertibleErrorCode(),
-        llvm::formatv("PID value {0} not allowed", pid == 0 ? 0 : -1));
-
-  if (!allow_all && tid == StringExtractorGDBRemote::AllThreads)
-    return llvm::make_error<StringError>(
-        inconvertibleErrorCode(),
-        llvm::formatv("TID value {0} not allowed", tid == 0 ? 0 : -1));
-
-  if (pid != StringExtractorGDBRemote::AllProcesses) {
-    if (pid != m_current_process->GetID())
-      return llvm::make_error<StringError>(
-          inconvertibleErrorCode(), llvm::formatv("PID {0} not debugged", pid));
-  }
-
-  return tid;
-}
-
 std::vector<std::string> GDBRemoteCommunicationServerLLGS::HandleFeatures(
     const llvm::ArrayRef<llvm::StringRef> client_features) {
   std::vector<std::string> ret =

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

@@ -293,15 +293,6 @@ class GDBRemoteCommunicationServerLLGS
 
   void StopSTDIOForwarding();
 
-  // Read thread-id from packet.  If the thread-id is correct, returns it.
-  // Otherwise, returns the error.
-  //
-  // If allow_all is true, then the pid/tid value of -1 ('all') will be allowed.
-  // In any case, the function assumes that exactly one inferior is being
-  // debugged and rejects pid values that do no match that inferior.
-  llvm::Expected<lldb::tid_t> ReadTid(StringExtractorGDBRemote &packet,
-                                      bool allow_all, lldb::pid_t default_pid);
-
   // Call SetEnabledExtensions() with appropriate flags on the process.
   void SetEnabledExtensions(NativeProcessProtocol &process);
 

lldb/source/Utility/StringExtractorGDBRemote.cpp

@@ -639,7 +639,7 @@ llvm::Optional<std::pair<lldb::pid_t, lldb::tid_t>>
 StringExtractorGDBRemote::GetPidTid(lldb::pid_t default_pid) {
   llvm::StringRef view = llvm::StringRef(m_packet).substr(m_index);
   size_t initial_length = view.size();
-  lldb::pid_t pid = default_pid;
+  lldb::pid_t pid = LLDB_INVALID_PROCESS_ID;
   lldb::tid_t tid;
 
   if (view.consume_front("p")) {
@@ -675,5 +675,5 @@ StringExtractorGDBRemote::GetPidTid(lldb::pid_t default_pid) {
   // update m_index
   m_index += initial_length - view.size();
 
-  return {{pid, tid}};
+  return {{pid != LLDB_INVALID_PROCESS_ID ? pid : default_pid, tid}};
 }

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

@@ -354,7 +354,7 @@ def test_vkill_parent(self):
     def test_vkill_both(self):
         self.vkill_test(kill_parent=True, kill_child=True)
 
-    def resume_one_test(self, run_order):
+    def resume_one_test(self, run_order, use_vCont=False):
         self.build()
         self.prep_debug_monitor_and_inferior(inferior_args=["fork", "trap"])
         self.add_qSupported_packets(["multiprocess+",
@@ -395,11 +395,19 @@ def resume_one_test(self, run_order):
             else:
                 assert False, "unexpected x={}".format(x)
 
+            if use_vCont:
+                self.test_sequence.add_log_lines([
+                    # continue the selected process
+                    "read packet: $vCont;c:p{}.{}#00".format(*pidtid),
+                ], True)
+            else:
+                self.test_sequence.add_log_lines([
+                    # continue the selected process
+                    "read packet: $Hcp{}.{}#00".format(*pidtid),
+                    "send packet: $OK#00",
+                    "read packet: $c#00",
+                ], True)
             self.test_sequence.add_log_lines([
-                # continue the selected process
-                "read packet: $Hcp{}.{}#00".format(*pidtid),
-                "send packet: $OK#00",
-                "read packet: $c#00",
                 {"direction": "send", "regex": expect},
             ], True)
             # if at least one process remained, check both PIDs
@@ -431,3 +439,117 @@ def test_c_child_then_parent(self):
     @add_test_categories(["fork"])
     def test_c_interspersed(self):
         self.resume_one_test(run_order=["parent", "child", "parent", "child"])
+
+    @add_test_categories(["fork"])
+    def test_vCont_parent(self):
+        self.resume_one_test(run_order=["parent", "parent"], use_vCont=True)
+
+    @add_test_categories(["fork"])
+    def test_vCont_child(self):
+        self.resume_one_test(run_order=["child", "child"], use_vCont=True)
+
+    @add_test_categories(["fork"])
+    def test_vCont_parent_then_child(self):
+        self.resume_one_test(run_order=["parent", "parent", "child", "child"],
+                             use_vCont=True)
+
+    @add_test_categories(["fork"])
+    def test_vCont_child_then_parent(self):
+        self.resume_one_test(run_order=["child", "child", "parent", "parent"],
+                             use_vCont=True)
+
+    @add_test_categories(["fork"])
+    def test_vCont_interspersed(self):
+        self.resume_one_test(run_order=["parent", "child", "parent", "child"],
+                             use_vCont=True)
+
+    @add_test_categories(["fork"])
+    def test_vCont_two_processes(self):
+        self.build()
+        self.prep_debug_monitor_and_inferior(inferior_args=["fork", "trap"])
+        self.add_qSupported_packets(["multiprocess+",
+                                     "fork-events+"])
+        ret = self.expect_gdbremote_sequence()
+        self.assertIn("fork-events+", ret["qSupported_response"])
+        self.reset_test_sequence()
+
+        # continue and expect fork
+        self.test_sequence.add_log_lines([
+            "read packet: $c#00",
+            {"direction": "send", "regex": self.fork_regex.format("fork"),
+             "capture": self.fork_capture},
+        ], True)
+        ret = self.expect_gdbremote_sequence()
+        parent_pid = ret["parent_pid"]
+        parent_tid = ret["parent_tid"]
+        child_pid = ret["child_pid"]
+        child_tid = ret["child_tid"]
+        self.reset_test_sequence()
+
+        self.test_sequence.add_log_lines([
+            # try to resume both processes
+            "read packet: $vCont;c:p{}.{};c:p{}.{}#00".format(
+                parent_pid, parent_tid, child_pid, child_tid),
+            "send packet: $E03#00",
+        ], True)
+        self.expect_gdbremote_sequence()
+
+    @add_test_categories(["fork"])
+    def test_vCont_all_processes_explicit(self):
+        self.build()
+        self.prep_debug_monitor_and_inferior(inferior_args=["fork", "trap"])
+        self.add_qSupported_packets(["multiprocess+",
+                                     "fork-events+"])
+        ret = self.expect_gdbremote_sequence()
+        self.assertIn("fork-events+", ret["qSupported_response"])
+        self.reset_test_sequence()
+
+        # continue and expect fork
+        self.test_sequence.add_log_lines([
+            "read packet: $c#00",
+            {"direction": "send", "regex": self.fork_regex.format("fork"),
+             "capture": self.fork_capture},
+        ], True)
+        ret = self.expect_gdbremote_sequence()
+        parent_pid = ret["parent_pid"]
+        parent_tid = ret["parent_tid"]
+        child_pid = ret["child_pid"]
+        child_tid = ret["child_tid"]
+        self.reset_test_sequence()
+
+        self.test_sequence.add_log_lines([
+            # try to resume all processes implicitly
+            "read packet: $vCont;c:p-1.-1#00",
+            "send packet: $E03#00",
+        ], True)
+        self.expect_gdbremote_sequence()
+
+    @add_test_categories(["fork"])
+    def test_vCont_all_processes_implicit(self):
+        self.build()
+        self.prep_debug_monitor_and_inferior(inferior_args=["fork", "trap"])
+        self.add_qSupported_packets(["multiprocess+",
+                                     "fork-events+"])
+        ret = self.expect_gdbremote_sequence()
+        self.assertIn("fork-events+", ret["qSupported_response"])
+        self.reset_test_sequence()
+
+        # continue and expect fork
+        self.test_sequence.add_log_lines([
+            "read packet: $c#00",
+            {"direction": "send", "regex": self.fork_regex.format("fork"),
+             "capture": self.fork_capture},
+        ], True)
+        ret = self.expect_gdbremote_sequence()
+        parent_pid = ret["parent_pid"]
+        parent_tid = ret["parent_tid"]
+        child_pid = ret["child_pid"]
+        child_tid = ret["child_tid"]
+        self.reset_test_sequence()
+
+        self.test_sequence.add_log_lines([
+            # try to resume all processes implicitly
+            "read packet: $vCont;c#00",
+            "send packet: $E03#00",
+        ], True)
+        self.expect_gdbremote_sequence()