[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

337

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

574–575

Maybe a comment how these two are different?

[mgorny]

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

337

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

573–575

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

573–575

Sure, done.

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

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 type);
+  llvm::Optional<std::pair<uint64_t, uint64_t>>
+  SendSetCurrentThreadPacket(uint64_t tid, uint64_t pid, char type);
 
-  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;
 
   lldb::pid_t m_curr_pid;
+  lldb::pid_t m_curr_pid_run;
   lldb::tid_t m_curr_tid; // Current gdb remote protocol thread index for all
                           // other operations
   lldb::tid_t m_curr_tid_run; // Current gdb remote protocol thread index for
                               // continue, step, etc
 
   uint32_t m_num_supported_hardware_watchpoints;
 
   ArchSpec m_host_arch;

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

: GDBRemoteClientBase("gdb-remote.client", "gdb-remote.client.rx_packet"),
       m_supports_qProcessInfoPID(true), m_supports_qfProcessInfo(true),
       m_supports_qUserName(true), m_supports_qGroupName(true),
       m_supports_qThreadStopInfo(true), m_supports_z0(true),
       m_supports_z1(true), m_supports_z2(true), m_supports_z3(true),
       m_supports_z4(true), m_supports_QEnvironment(true),
       m_supports_QEnvironmentHexEncoded(true), m_supports_qSymbol(true),
       m_qSymbol_requests_done(false), m_supports_qModuleInfo(true),
       m_supports_jThreadsInfo(true), m_supports_jModulesInfo(true),
-      m_curr_pid(LLDB_INVALID_PROCESS_ID), m_curr_tid(LLDB_INVALID_THREAD_ID),
+      m_curr_pid(LLDB_INVALID_PROCESS_ID),
+      m_curr_pid_run(LLDB_INVALID_PROCESS_ID),
+      m_curr_tid(LLDB_INVALID_THREAD_ID),
       m_curr_tid_run(LLDB_INVALID_THREAD_ID),
       m_num_supported_hardware_watchpoints(0), m_host_arch(), m_process_arch(),
       m_os_build(), m_os_kernel(), m_hostname(), m_gdb_server_name(),
       m_gdb_server_version(UINT32_MAX), m_default_packet_timeout(0),
       m_max_packet_size(0), m_qSupported_response(),
       m_supported_async_json_packets_is_valid(false),
       m_supported_async_json_packets_sp(), m_qXfer_memory_map(),
       m_qXfer_memory_map_loaded(false) {}
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<std::pair<uint64_t, uint64_t>>
 GDBRemoteCommunicationClient::SendSetCurrentThreadPacket(uint64_t tid,
+                                                         uint64_t pid,
                                                          char type) {
   lldb_private::StreamString packet;
   packet.PutChar('H');
   packet.PutChar(type);
+
+  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<std::pair<uint64_t, uint64_t>> ret =
+      SendSetCurrentThreadPacket(tid, pid, 'g');
+  if (ret.hasValue()) {
+    if (ret->first != LLDB_INVALID_PROCESS_ID)
+      m_curr_pid = ret->first;
+    m_curr_tid = ret->second;
+  }
   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<std::pair<uint64_t, uint64_t>> ret =
+      SendSetCurrentThreadPacket(tid, pid, 'c');
+  if (ret.hasValue()) {
+    if (ret->first != LLDB_INVALID_PROCESS_ID)
+      m_curr_pid_run = ret->first;
+    m_curr_tid_run = ret->second;
+  }
   return ret.hasValue();
 }
 
 bool GDBRemoteCommunicationClient::GetStopReply(
     StringExtractorGDBRemote &response) {
   if (SendPacketAndWaitForResponse("?", response, false) ==
       PacketResult::Success)
     return response.IsNormalResponse();