[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

2097

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

239

revert

[mgorny]

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

2097

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

2100

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

2100

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

2091

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

[mgorny]

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

2091

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

2091

Fair enough. Thanks for the explanation!

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

public:
   Status InitializeConnection(std::unique_ptr<Connection> connection);
 
 protected:
   MainLoop &m_mainloop;
   MainLoop::ReadHandleUP m_network_handle_up;
   const NativeProcessProtocol::Factory &m_process_factory;
   lldb::tid_t m_current_tid = LLDB_INVALID_THREAD_ID;
   lldb::tid_t m_continue_tid = LLDB_INVALID_THREAD_ID;
+  lldb::pid_t m_current_pid = LLDB_INVALID_PROCESS_ID;
   std::recursive_mutex m_debugged_process_mutex;
   std::unique_ptr<NativeProcessProtocol> m_debugged_process_up;
   std::unordered_map<lldb::pid_t, std::unique_ptr<NativeProcessProtocol>>
       m_additional_processes;
 
   Communication m_stdio_communication;
   MainLoop::ReadHandleUP m_stdio_handle_up;
 
protected:
 
   static std::string XMLEncodeAttributeValue(llvm::StringRef value);
 
   virtual std::vector<std::string>
   HandleFeatures(const llvm::ArrayRef<llvm::StringRef> client_features) override;
 
   // Verify and get the process instance selected by the client.
   // If run is true, the process selected by "Hc" packet will be returned,
-  // otherwise the one selected by "Hg" will be used.  Returns nullptr if there
-  // is no process debugged.
-  NativeProcessProtocol *GetCurrentProcess(bool run) const;
+  // otherwise the one selected by "Hg" will be used.
+  //
+  // If allow_subprocess is true, a subprocess instance may be returned.
+  // Otherwise, only the main process can be returned and the function will
+  // return nullptr if a subprocess is selected.
+  //
+  // Returns nullptr if there is no process debugged.
+  NativeProcessProtocol *GetCurrentProcess(bool run,
+                                           bool allow_subprocess = false) const;
+
+  void SetCurrentProcessID(lldb::pid_t pid, lldb::tid_t tid);
 
 private:
   llvm::Expected<std::unique_ptr<llvm::MemoryBuffer>> BuildTargetXml();
 
   void HandleInferiorState_Exited(NativeProcessProtocol *process);
 
   void HandleInferiorState_Stopped(NativeProcessProtocol *process);
 
   NativeThreadProtocol *GetThreadFromSuffix(StringExtractorGDBRemote &packet);

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

 
 void GDBRemoteCommunicationServerLLGS::SetCurrentThreadID(lldb::tid_t tid) {
   Log *log(GetLogIfAnyCategoriesSet(LIBLLDB_LOG_THREAD));
   LLDB_LOG(log, "setting current thread id to {0}", tid);
 
   m_current_tid = tid;
   if (m_debugged_process_up)
     m_debugged_process_up->SetCurrentThreadID(m_current_tid);
+
+  // Reset subprocess support
+  m_current_pid = LLDB_INVALID_PROCESS_ID;
 }
 
 void GDBRemoteCommunicationServerLLGS::SetContinueThreadID(lldb::tid_t tid) {
   Log *log(GetLogIfAnyCategoriesSet(LIBLLDB_LOG_THREAD));
   LLDB_LOG(log, "setting continue thread id to {0}", tid);
 
   m_continue_tid = tid;
 }
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, /*run=*/h_variant == 'c', /*allow_all=*/true);
-  if (!tid_ret)
-    return SendErrorResponse(tid_ret.takeError());
+  auto *process = GetCurrentProcess(h_variant == 'c', true);
+  auto pid_tid =
+      packet.GetPidTid(process ? 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 != m_debugged_process_up->GetID()) {
+    // Is it a subprocess?
+    if (m_additional_processes.find(pid) == m_additional_processes.end())
+      return SendErrorResponse(llvm::make_error<StringError>(
+          inconvertibleErrorCode(),
+          llvm::formatv("PID {0} not debugged", pid)));
+    if (h_variant != 'g')
+      return SendUnimplementedResponse(
+          "Hc packet for subprocess not supported");
+
+    SetCurrentProcessID(pid, tid);
+    return SendOKResponse();
+  }
 
-  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_debugged_process_up->GetThreadByID(tid);
     if (!thread) {
       LLDB_LOGF(log,
                 "GDBRemoteCommunicationServerLLGS::%s failed, tid %" PRIu64
                 " not found",
LLDB_LOG(log, "pid {0} failed to set watchpoint: {1}", process->GetID(),
              error);
     return SendErrorResponse(0x09);
   }
 }
 
 GDBRemoteCommunication::PacketResult
 GDBRemoteCommunicationServerLLGS::Handle_z(StringExtractorGDBRemote &packet) {
   // Ensure we have a process.
-  auto *process = GetCurrentProcess(false);
+  auto *process = GetCurrentProcess(false, true);
   if (!process) {
     Log *log(GetLogIfAnyCategoriesSet(LIBLLDB_LOG_PROCESS));
     LLDB_LOG(log, "failed, no process available");
     return SendErrorResponse(0x15);
   }
 
   // Parse out software or hardware breakpoint or watchpoint requested.
   packet.SetFilePos(strlen("z"));
if (m_vfork_events_supported)
     flags |= NativeProcessProtocol::Extension::vfork;
 
   llvm::Error error = process.SetEnabledExtensions(flags);
   if (error)
     LLDB_LOG_ERROR(log, std::move(error),
                    "Enabling protocol extensions failed: {0}");
 }
 
-NativeProcessProtocol *
-GDBRemoteCommunicationServerLLGS::GetCurrentProcess(bool run) const {
+NativeProcessProtocol *GDBRemoteCommunicationServerLLGS::GetCurrentProcess(
+    bool run, bool allow_subprocess) const {
   // Fail if we don't have a current process.
   if (!m_debugged_process_up ||
       (m_debugged_process_up->GetID() == LLDB_INVALID_PROCESS_ID))
     return nullptr;
 
+  // Check if another process was selected via Hg.
+  if (!run && m_current_pid != LLDB_INVALID_PROCESS_ID &&
+      m_current_pid != m_debugged_process_up->GetID()) {
+    if (!allow_subprocess)
+      return nullptr;
+
+    auto it = m_additional_processes.find(m_current_pid);
+    if (it == m_additional_processes.end())
+      return nullptr;
+    return it->second.get();
+  }
+
   return m_debugged_process_up.get();
 }
+
+void GDBRemoteCommunicationServerLLGS::SetCurrentProcessID(lldb::pid_t pid,
+                                                           lldb::tid_t tid) {
+  Log *log(GetLogIfAnyCategoriesSet(LIBLLDB_LOG_PROCESS));
+  LLDB_LOG(log, "setting current subprocess to {0}, thread id {1}", pid, tid);
+
+  m_current_pid = pid;
+  m_current_tid = tid;
+}