[lldb] [gdb-remote server] Support selecting process via Hg

[mgorny]

Summary

Support using the extended thread-id syntax with Hg packet to select
a subprocess. This makes it possible to start providing support for
running some of the debugger packets against another subprocesses.

[mgorny]

Rebase.

[labath]

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

2132

Does it ever make sense to have more than one (process or thread) selected? If not, an error response may be more appropriate..

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

234 ↗

(On Diff #337241)

revert

[mgorny]

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

2132

Good question.

As for PID, I don't think that the client would do something like that within foreseeable future.

However, sending all-TIDs are technically supported in our LLGS client code. I don't know whether it's actually used anywhere, though.

[mgorny]

Revert accidental whitespace change.

[mgorny]

Add tests.

[rovka]

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

2135

Just a drive-by comment: The previous behaviour was to send 0x15 when the current process ID was invalid. Should we do that here?

[mgorny]

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

2135

I've originally did that but it was kinda hackish and @labath didn't like it.

[JDevlieghere]

I'm probably not the most qualified to sign off on this, but the review has stalled and I don't see anything obviously wrong here so LGTM.

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

2126

Would it be useful to say "Malformed thread-id for process-id {}"?

[mgorny]

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

2126

In this context, 'thread-id' is GDB protocol jargon for 'PID and/or TID'. So if it's malformed, then we don't really have a process ID here. I suppose I could try replacing 'thread-id' with something more verbose but I'm not sure if this is the kind of error end users will be dealing with.

[JDevlieghere]

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

2126

Fair enough. Thanks for the explanation!

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

GDBRemoteCommunicationServerLLGS::Handle_P(StringExtractorGDBRemote &packet) {
 
   return SendOKResponse();
 }
 
 GDBRemoteCommunication::PacketResult
 GDBRemoteCommunicationServerLLGS::Handle_H(StringExtractorGDBRemote &packet) {
   Log *log(GetLogIfAnyCategoriesSet(LIBLLDB_LOG_THREAD));
 
-  // Fail if we don't have a current process.
-  if (!m_current_process ||
-      (m_current_process->GetID() == LLDB_INVALID_PROCESS_ID)) {
-    LLDB_LOGF(
-        log,
-        "GDBRemoteCommunicationServerLLGS::%s failed, no process available",
-        __FUNCTION__);
-    return SendErrorResponse(0x15);
-  }
-
   // Parse out which variant of $H is requested.
   packet.SetFilePos(strlen("H"));
   if (packet.GetBytesLeft() < 1) {
     LLDB_LOGF(log,
               "GDBRemoteCommunicationServerLLGS::%s failed, H command "
               "missing {g,c} variant",
               __FUNCTION__);
     return SendIllFormedResponse(packet, "H command missing {g,c} variant");
   }
 
   const char h_variant = packet.GetChar();
-  lldb::pid_t default_pid;
+  NativeProcessProtocol *default_process;
   switch (h_variant) {
   case 'g':
-    default_pid = m_current_process->GetID();
+    default_process = m_current_process;
     break;
 
   case 'c':
-    default_pid = m_continue_process->GetID();
+    default_process = m_continue_process;
     break;
 
   default:
     LLDB_LOGF(
         log,
         "GDBRemoteCommunicationServerLLGS::%s failed, invalid $H variant %c",
         __FUNCTION__, h_variant);
     return SendIllFormedResponse(packet,
                                  "H variant unsupported, should be c or g");
   }
 
   // Parse out the thread number.
-  llvm::Expected<lldb::tid_t> tid_ret =
-      ReadTid(packet, /*allow_all=*/true, default_pid);
-  if (!tid_ret)
-    return SendErrorResponse(tid_ret.takeError());
+  auto pid_tid = packet.GetPidTid(default_process ? default_process->GetID()
+                                                  : LLDB_INVALID_PROCESS_ID);
+  if (!pid_tid)
+    return SendErrorResponse(llvm::make_error<StringError>(
+        inconvertibleErrorCode(), "Malformed thread-id"));
+
+  lldb::pid_t pid = pid_tid->first;
+  lldb::tid_t tid = pid_tid->second;
+
+  if (pid == StringExtractorGDBRemote::AllProcesses)
+    return SendUnimplementedResponse("Selecting all processes not supported");
+  if (pid == LLDB_INVALID_PROCESS_ID)
+    return SendErrorResponse(llvm::make_error<StringError>(
+        inconvertibleErrorCode(), "No current process and no PID provided"));
+
+  // Check the process ID and find respective process instance.
+  auto new_process_it = m_debugged_processes.find(pid);
+  if (new_process_it == m_debugged_processes.end())
+    return SendErrorResponse(llvm::make_error<StringError>(
+        inconvertibleErrorCode(),
+        llvm::formatv("No process with PID {0} debugged", pid)));
 
-  lldb::tid_t tid = tid_ret.get();
   // Ensure we have the given thread when not specifying -1 (all threads) or 0
   // (any thread).
   if (tid != LLDB_INVALID_THREAD_ID && tid != 0) {
-    NativeThreadProtocol *thread = m_current_process->GetThreadByID(tid);
+    NativeThreadProtocol *thread = new_process_it->second->GetThreadByID(tid);
     if (!thread) {
       LLDB_LOGF(log,
                 "GDBRemoteCommunicationServerLLGS::%s failed, tid %" PRIu64
                 " not found",
                 __FUNCTION__, tid);
       return SendErrorResponse(0x15);
     }
   }
 
-  // Now switch the given thread type.
+  // Now switch the given process and thread type.
   switch (h_variant) {
   case 'g':
+    m_current_process = new_process_it->second.get();
     SetCurrentThreadID(tid);
     break;
 
   case 'c':
+    m_continue_process = new_process_it->second.get();
     SetContinueThreadID(tid);
     break;
 
   default:
     assert(false && "unsupported $H variant - shouldn't get here");
     return SendIllFormedResponse(packet,
                                  "H variant unsupported, should be c or g");
   }

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

def fork_and_detach_test(self, variant):
         self.prep_debug_monitor_and_inferior(inferior_args=[variant])
         self.add_qSupported_packets(["multiprocess+",
                                      "{}-events+".format(variant)])
         ret = self.expect_gdbremote_sequence()
         self.assertIn("{}-events+".format(variant), ret["qSupported_response"])
         self.reset_test_sequence()
 
         # continue and expect fork
-        fork_regex = "[$]T.*;{}:p([0-9a-f]*)[.]([0-9a-f]*).*".format(variant)
+        fork_regex = "[$]T.*;{}:p([0-9a-f]+)[.]([0-9a-f]+).*".format(variant)
         self.test_sequence.add_log_lines([
             "read packet: $c#00",
             {"direction": "send", "regex": fork_regex,
              "capture": {1: "pid", 2: "tid"}},
         ], True)
         ret = self.expect_gdbremote_sequence()
         pid = int(ret["pid"], 16)
         self.reset_test_sequence()
def test_vfork(self):
         # resume the parent
         self.test_sequence.add_log_lines([
             "read packet: $c#00",
             {"direction": "send", "regex": r"[$]T.*vforkdone.*"},
             "read packet: $c#00",
             {"direction": "send", "regex": r"[$]W00#.*"},
         ], True)
         self.expect_gdbremote_sequence()
+
+    def fork_and_follow_test(self, variant):
+        self.build()
+        self.prep_debug_monitor_and_inferior(inferior_args=[variant])
+        self.add_qSupported_packets(["multiprocess+",
+                                     "{}-events+".format(variant)])
+        ret = self.expect_gdbremote_sequence()
+        self.assertIn("{}-events+".format(variant), ret["qSupported_response"])
+        self.reset_test_sequence()
+
+        # continue and expect fork
+        procinfo_regex = "[$]pid:([0-9a-f]+);.*"
+        fork_regex = "[$]T.*;{}:p([0-9a-f]+)[.]([0-9a-f]+).*".format(variant)
+        self.test_sequence.add_log_lines([
+            "read packet: $qProcessInfo#00",
+            {"direction": "send", "regex": procinfo_regex,
+             "capture": {1: "parent_pid"}},
+            "read packet: $c#00",
+            {"direction": "send", "regex": fork_regex,
+             "capture": {1: "pid", 2: "tid"}},
+        ], True)
+        ret = self.expect_gdbremote_sequence()
+        parent_pid, pid, tid = (int(ret[x], 16) for x
+                                in ("parent_pid", "pid", "tid"))
+        self.reset_test_sequence()
+
+        # switch to the forked child
+        self.test_sequence.add_log_lines([
+            "read packet: $Hgp{:x}.{:x}#00".format(pid, tid),
+            {"direction": "send", "regex": r"[$]OK#.*"},
+            "read packet: $Hcp{:x}.{:x}#00".format(pid, tid),
+            {"direction": "send", "regex": r"[$]OK#.*"},
+        ], True)
+
+        # detach the parent
+        self.test_sequence.add_log_lines([
+            "read packet: $D;{:x}#00".format(parent_pid),
+            {"direction": "send", "regex": r"[$]OK#.*"},
+        ], True)
+        ret = self.expect_gdbremote_sequence()
+        self.reset_test_sequence()
+
+        # resume the child
+        self.test_sequence.add_log_lines([
+            "read packet: $c#00",
+            {"direction": "send", "regex": r"[$]W00#.*"},
+        ], True)
+        self.expect_gdbremote_sequence()
+
+    @add_test_categories(["fork"])
+    def test_fork_follow(self):
+        self.fork_and_follow_test("fork")
+
+    @add_test_categories(["fork"])
+    def test_vfork_follow(self):
+        self.fork_and_follow_test("vfork")
+
+    @add_test_categories(["fork"])
+    def test_select_wrong_pid(self):
+        self.build()
+        self.prep_debug_monitor_and_inferior()
+        self.add_qSupported_packets(["multiprocess+"])
+        ret = self.expect_gdbremote_sequence()
+        self.assertIn("multiprocess+", ret["qSupported_response"])
+        self.reset_test_sequence()
+
+        # get process pid
+        procinfo_regex = "[$]pid:([0-9a-f]+);.*"
+        self.test_sequence.add_log_lines([
+            "read packet: $qProcessInfo#00",
+            {"direction": "send", "regex": procinfo_regex,
+             "capture": {1: "pid"}},
+            "read packet: $qC#00",
+            {"direction": "send", "regex": "[$]QC([0-9a-f]+)#.*",
+             "capture": {1: "tid"}},
+        ], True)
+        ret = self.expect_gdbremote_sequence()
+        pid, tid = (int(ret[x], 16) for x in ("pid", "tid"))
+        self.reset_test_sequence()
+
+        # try switching to correct pid
+        self.test_sequence.add_log_lines([
+            "read packet: $Hgp{:x}.{:x}#00".format(pid, tid),
+            {"direction": "send", "regex": r"[$]OK#.*"},
+            "read packet: $Hcp{:x}.{:x}#00".format(pid, tid),
+            {"direction": "send", "regex": r"[$]OK#.*"},
+        ], True)
+        ret = self.expect_gdbremote_sequence()
+
+        # try switching to invalid tid
+        self.test_sequence.add_log_lines([
+            "read packet: $Hgp{:x}.{:x}#00".format(pid, tid+1),
+            {"direction": "send", "regex": r"[$]E15#.*"},
+            "read packet: $Hcp{:x}.{:x}#00".format(pid, tid+1),
+            {"direction": "send", "regex": r"[$]E15#.*"},
+        ], True)
+        ret = self.expect_gdbremote_sequence()
+
+        # try switching to invalid pid
+        self.test_sequence.add_log_lines([
+            "read packet: $Hgp{:x}.{:x}#00".format(pid+1, tid),
+            {"direction": "send", "regex": r"[$]Eff#.*"},
+            "read packet: $Hcp{:x}.{:x}#00".format(pid+1, tid),
+            {"direction": "send", "regex": r"[$]Eff#.*"},
+        ], True)
+        ret = self.expect_gdbremote_sequence()
+
+    def test_detach_current(self):
+        self.build()
+        self.prep_debug_monitor_and_inferior()
+        self.add_qSupported_packets(["multiprocess+"])
+        ret = self.expect_gdbremote_sequence()
+        self.assertIn("multiprocess+", ret["qSupported_response"])
+        self.reset_test_sequence()
+
+        # get process pid
+        procinfo_regex = "[$]pid:([0-9a-f]+);.*"
+        self.test_sequence.add_log_lines([
+            "read packet: $qProcessInfo#00",
+            {"direction": "send", "regex": procinfo_regex,
+             "capture": {1: "pid"}},
+        ], True)
+        ret = self.expect_gdbremote_sequence()
+        pid = int(ret["pid"], 16)
+        self.reset_test_sequence()
+
+        # detach the process
+        self.test_sequence.add_log_lines([
+            "read packet: $D;{:x}#00".format(pid),
+            {"direction": "send", "regex": r"[$]OK#.*"},
+            "read packet: $qC#00",
+            {"direction": "send", "regex": r"[$]E44#.*"},
+        ], True)
+        ret = self.expect_gdbremote_sequence()