[lldb] [gdb-remote client] Support switching PID along with TID

[mgorny]

Summary

Extend the SetCurrentThread() method to support specifying an alternate
PID to switch to. This makes it possible to issue requests to forked
processes.

[mgorny]

Rebase on top of the refactor. Remove m_override_pid in favor of controlling m_curr_pid and m_curr_pid_run.

[JDevlieghere]

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

345–346

I generally prefer a struct with named fields over an unnamed pair. Do you think that would help readability here?

587–589

Maybe a comment how these two are different?

[mgorny]

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

345–346

While I was typing that I'm fine either way, I've noticed that the function takes tid+pid but returns pid+tid, so I suppose it could be confusing. I'll switch over to structs.

[mgorny]

Rebase. Use a struct for the PID+TID return value.

[JDevlieghere]

Thanks! LGTM

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

589–590

Can we please make these doxygen comments /// and put them above the variable? The trailing comments really don't work well with the 80-col limit.

[mgorny]

I'll do one more test run and push, thanks!

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

589–590

Sure, done.

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

struct QOffsets {
   /// Segment offsets have either one of two.
   std::vector<uint64_t> offsets;
 };
 inline bool operator==(const QOffsets &a, const QOffsets &b) {
   return a.segments == b.segments && a.offsets == b.offsets;
 }
 llvm::raw_ostream &operator<<(llvm::raw_ostream &os, const QOffsets &offsets);
 
+// A trivial struct used to return a pair of PID and TID.
+struct PidTid {
+  uint64_t pid;
+  uint64_t tid;
+};
+
 class GDBRemoteCommunicationClient : public GDBRemoteClientBase {
 public:
   GDBRemoteCommunicationClient();
 
   ~GDBRemoteCommunicationClient() override;
 
   // After connecting, send the handshake to the server to make sure
   // we are communicating with it.
public:
 
   // This packet is for testing the speed of the interface only. Both
   // the client and server need to support it, but this allows us to
   // measure the packet speed without any other work being done on the
   // other end and avoids any of that work affecting the packet send
   // and response times.
   bool SendSpeedTestPacket(uint32_t send_size, uint32_t recv_size);
 
-  llvm::Optional<uint64_t> SendSetCurrentThreadPacket(uint64_t tid, char op);
+  llvm::Optional<PidTid>
+  SendSetCurrentThreadPacket(uint64_t tid, uint64_t pid, char op);
 
-  bool SetCurrentThread(uint64_t tid);
+  bool SetCurrentThread(uint64_t tid,
+                        lldb::pid_t pid = LLDB_INVALID_PROCESS_ID);
 
-  bool SetCurrentThreadForRun(uint64_t tid);
+  bool SetCurrentThreadForRun(uint64_t tid,
+                              lldb::pid_t pid = LLDB_INVALID_PROCESS_ID);
 
   bool GetQXferAuxvReadSupported();
 
   void EnableErrorStringInPacket();
 
   bool GetQXferLibrariesReadSupported();
 
   bool GetQXferLibrariesSVR4ReadSupported();
protected:
   bool m_supports_qProcessInfoPID : 1, m_supports_qfProcessInfo : 1,
       m_supports_qUserName : 1, m_supports_qGroupName : 1,
       m_supports_qThreadStopInfo : 1, m_supports_z0 : 1, m_supports_z1 : 1,
       m_supports_z2 : 1, m_supports_z3 : 1, m_supports_z4 : 1,
       m_supports_QEnvironment : 1, m_supports_QEnvironmentHexEncoded : 1,
       m_supports_qSymbol : 1, m_qSymbol_requests_done : 1,
       m_supports_qModuleInfo : 1, m_supports_jThreadsInfo : 1,
       m_supports_jModulesInfo : 1;
 
+  /// Current gdb remote protocol process identifier for all other operations
   lldb::pid_t m_curr_pid = LLDB_INVALID_PROCESS_ID;
-  lldb::tid_t m_curr_tid =
-      LLDB_INVALID_THREAD_ID; // Current gdb remote protocol thread index for
-                              // all other operations
-  lldb::tid_t m_curr_tid_run =
-      LLDB_INVALID_THREAD_ID; // Current gdb remote protocol thread index for
-                              // continue, step, etc
+  /// Current gdb remote protocol process identifier for continue, step, etc
+  lldb::pid_t m_curr_pid_run = LLDB_INVALID_PROCESS_ID;
+  /// Current gdb remote protocol thread identifier for all other operations
+  lldb::tid_t m_curr_tid = LLDB_INVALID_THREAD_ID;
+  /// Current gdb remote protocol thread identifier for continue, step, etc
+  lldb::tid_t m_curr_tid_run = LLDB_INVALID_THREAD_ID;
 
   uint32_t m_num_supported_hardware_watchpoints = 0;
   uint32_t m_addressing_bits = 0;
 
   ArchSpec m_host_arch;
   ArchSpec m_process_arch;
   llvm::VersionTuple m_os_version;
   llvm::VersionTuple m_maccatalyst_version;

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

if (m_curr_pid_is_valid == eLazyBoolYes) {
     // lldb-platform stubs. The gdb remote protocol documents $qC as returning
     // the thread id, which newer debugserver and lldb-gdbserver stubs return
     // correctly.
     StringExtractorGDBRemote response;
     if (SendPacketAndWaitForResponse("qC", response, false) ==
         PacketResult::Success) {
       if (response.GetChar() == 'Q') {
         if (response.GetChar() == 'C') {
-          m_curr_pid = response.GetHexMaxU64(false, LLDB_INVALID_PROCESS_ID);
+          m_curr_pid_run = m_curr_pid =
+              response.GetHexMaxU64(false, LLDB_INVALID_PROCESS_ID);
           if (m_curr_pid != LLDB_INVALID_PROCESS_ID) {
             m_curr_pid_is_valid = eLazyBoolYes;
             return m_curr_pid;
           }
         }
       }
     }
 
     // If we don't get a response for $qC, check if $qfThreadID gives us a
     // result.
     if (m_curr_pid == LLDB_INVALID_PROCESS_ID) {
       bool sequence_mutex_unavailable;
       auto ids = GetCurrentProcessAndThreadIDs(sequence_mutex_unavailable);
       if (!ids.empty() && !sequence_mutex_unavailable) {
         // If server returned an explicit PID, use that.
-        m_curr_pid = ids.front().first;
+        m_curr_pid_run = m_curr_pid = ids.front().first;
         // Otherwise, use the TID of the first thread (Linux hack).
         if (m_curr_pid == LLDB_INVALID_PROCESS_ID)
-          m_curr_pid = ids.front().second;
+          m_curr_pid_run = m_curr_pid = ids.front().second;
         m_curr_pid_is_valid = eLazyBoolYes;
         return m_curr_pid;
       }
     }
   }
 
   return LLDB_INVALID_PROCESS_ID;
 }
if (response.GetChar() == 'Q' && response.GetChar() == 'C') {
 
     lldb::pid_t pid = pid_tid->first;
     // invalid
     if (pid == StringExtractorGDBRemote::AllProcesses)
       return false;
 
     // if we get pid as well, update m_curr_pid
     if (pid != 0) {
-      m_curr_pid = pid;
+      m_curr_pid_run = m_curr_pid = pid;
       m_curr_pid_is_valid = eLazyBoolYes;
     }
     tid = pid_tid->second;
   }
 
   return true;
 }
 
if (response.IsNormalResponse()) {
           elf_abi = std::string(value);
           ++num_keys_decoded;
         }
       }
       if (num_keys_decoded > 0)
         m_qProcessInfo_is_valid = eLazyBoolYes;
       if (pid != LLDB_INVALID_PROCESS_ID) {
         m_curr_pid_is_valid = eLazyBoolYes;
-        m_curr_pid = pid;
+        m_curr_pid_run = m_curr_pid = pid;
       }
 
       // Set the ArchSpec from the triple if we have it.
       if (!triple.empty()) {
         m_process_arch.SetTriple(triple.c_str());
         m_process_arch.SetFlags(elf_abi);
         if (pointer_byte_size) {
           assert(pointer_byte_size == m_process_arch.GetAddressByteSize());
bool GDBRemoteCommunicationClient::KillSpawnedProcess(lldb::pid_t pid) {
   if (SendPacketAndWaitForResponse(stream.GetString(), response, false) ==
       PacketResult::Success) {
     if (response.IsOKResponse())
       return true;
   }
   return false;
 }
 
-llvm::Optional<uint64_t>
+llvm::Optional<PidTid>
 GDBRemoteCommunicationClient::SendSetCurrentThreadPacket(uint64_t tid,
+                                                         uint64_t pid,
                                                          char op) {
   lldb_private::StreamString packet;
   packet.PutChar('H');
   packet.PutChar(op);
+
+  if (pid != LLDB_INVALID_PROCESS_ID) {
+    packet.PutChar('p');
+    packet.PutHex64(pid);
+    packet.PutChar('.');
+  }
+
   if (tid == UINT64_MAX)
     packet.PutCString("-1");
   else
     packet.PutHex64(tid);
+
   StringExtractorGDBRemote response;
   if (SendPacketAndWaitForResponse(packet.GetString(), response, false) ==
       PacketResult::Success) {
     if (response.IsOKResponse())
-      return tid;
+      return {{pid, tid}};
 
     /*
      * Connected bare-iron target (like YAMON gdb-stub) may not have support for
      * Hg packet.
      * The reply from '?' packet could be as simple as 'S05'. There is no packet
      * which can
      * give us pid and/or tid. Assume pid=tid=1 in such cases.
      */
     if (response.IsUnsupportedResponse() && IsConnected())
-      return 1;
+      return {{1, 1}};
   }
   return llvm::None;
 }
 
-bool GDBRemoteCommunicationClient::SetCurrentThread(uint64_t tid) {
-  if (m_curr_tid == tid)
+bool GDBRemoteCommunicationClient::SetCurrentThread(uint64_t tid,
+                                                    uint64_t pid) {
+  if (m_curr_tid == tid &&
+      (m_curr_pid == pid || LLDB_INVALID_PROCESS_ID == pid))
     return true;
 
-  llvm::Optional<uint64_t> ret = SendSetCurrentThreadPacket(tid, 'g');
-  if (ret.hasValue())
-    m_curr_tid = ret.getValue();
+  llvm::Optional<PidTid> ret = SendSetCurrentThreadPacket(tid, pid, 'g');
+  if (ret.hasValue()) {
+    if (ret->pid != LLDB_INVALID_PROCESS_ID)
+      m_curr_pid = ret->pid;
+    m_curr_tid = ret->tid;
+  }
   return ret.hasValue();
 }
 
-bool GDBRemoteCommunicationClient::SetCurrentThreadForRun(uint64_t tid) {
-  if (m_curr_tid_run == tid)
+bool GDBRemoteCommunicationClient::SetCurrentThreadForRun(uint64_t tid,
+                                                          uint64_t pid) {
+  if (m_curr_tid_run == tid &&
+      (m_curr_pid_run == pid || LLDB_INVALID_PROCESS_ID == pid))
     return true;
 
-  llvm::Optional<uint64_t> ret = SendSetCurrentThreadPacket(tid, 'c');
-  if (ret.hasValue())
-    m_curr_tid_run = ret.getValue();
+  llvm::Optional<PidTid> ret = SendSetCurrentThreadPacket(tid, pid, 'c');
+  if (ret.hasValue()) {
+    if (ret->pid != LLDB_INVALID_PROCESS_ID)
+      m_curr_pid_run = ret->pid;
+    m_curr_tid_run = ret->tid;
+  }
   return ret.hasValue();
 }
 
 bool GDBRemoteCommunicationClient::GetStopReply(
     StringExtractorGDBRemote &response) {
   if (SendPacketAndWaitForResponse("?", response, false) ==
       PacketResult::Success)
     return response.IsNormalResponse();