[lldb] [llgs] Convert m_debugged_processes into a map of structs

[mgorny]

Summary

Convert the m_debugged_processes map from NativeProcessProtocol pointers
to structs, and combine the additional set(s) holding the additional
process properties into a flag field inside this struct. This is
desirable since there are more properties to come and having a single
structure with all information should be cleaner and more efficient than
using multiple sets for that.

Suggested by Pavel Labath in D128893.

[labath]

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

1058–1060

Btw, is this actually correct. E.g. I don't think we ought to terminate the connection if a process dies "naturally", and there are still other processes around...

Isn't it more like the opposite, that if k is used, *then* we close the connection (after all processes exit)? And maybe also when the process exits, and multiprocess extensions are disabled?

[mgorny]

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

1058–1060

Well, the comment describes some of the cases when we don't terminate it, i.e. it skips the kinda-obvious case when there are additional processes running, i.e. we stop it only if all of the following apply: a. all processes have exited, b. we are not in non-stop mode (in non-stop mode we wait for vStopped) and c. the last process wasn't killed by vKill (to match the behavior of GDB).

[labath]

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

1058–1060

Ah, yes, that makes sense. I should have looked at the code as well. Thanks for explaining.

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

public:
   void DidExec(NativeProcessProtocol *process) override;
 
   void
   NewSubprocess(NativeProcessProtocol *parent_process,
                 std::unique_ptr<NativeProcessProtocol> child_process) override;
 
   Status InitializeConnection(std::unique_ptr<Connection> connection);
 
+  struct DebuggedProcess {
+    enum class Flag {
+      vkilled = (1u << 0),
+
+      LLVM_MARK_AS_BITMASK_ENUM(vkilled)
+    };
+
+    std::unique_ptr<NativeProcessProtocol> process_up;
+    Flag flags;
+  };
+
 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;
   NativeProcessProtocol *m_current_process;
   NativeProcessProtocol *m_continue_process;
   std::recursive_mutex m_debugged_process_mutex;
-  std::unordered_map<lldb::pid_t, std::unique_ptr<NativeProcessProtocol>>
-      m_debugged_processes;
-  std::unordered_set<lldb::pid_t> m_vkilled_processes;
+  std::unordered_map<lldb::pid_t, DebuggedProcess> m_debugged_processes;
 
   Communication m_stdio_communication;
   MainLoop::ReadHandleUP m_stdio_handle_up;
 
   lldb::StateType m_inferior_prev_state = lldb::StateType::eStateInvalid;
   llvm::StringMap<std::unique_ptr<llvm::MemoryBuffer>> m_xfer_buffer_map;
   std::mutex m_saved_registers_mutex;
   std::unordered_map<uint32_t, lldb::DataBufferSP> m_saved_registers_map;

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

#endif
     std::lock_guard<std::recursive_mutex> guard(m_debugged_process_mutex);
     assert(m_debugged_processes.empty() && "lldb-server creating debugged "
                                            "process but one already exists");
     auto process_or =
         m_process_factory.Launch(m_process_launch_info, *this, m_mainloop);
     if (!process_or)
       return Status(process_or.takeError());
     m_continue_process = m_current_process = process_or->get();
-    m_debugged_processes[m_current_process->GetID()] = std::move(*process_or);
+    m_debugged_processes.emplace(
+        m_current_process->GetID(),
+        DebuggedProcess{std::move(*process_or), DebuggedProcess::Flag{}});
   }
 
   SetEnabledExtensions(*m_current_process);
 
   // Handle mirroring of inferior stdout/stderr over the gdb-remote protocol as
   // needed. llgs local-process debugging may specify PTY paths, which will
   // make these file actions non-null process launch -i/e/o will also make
   // these file actions non-null nullptr means that the traffic is expected to
Status GDBRemoteCommunicationServerLLGS::AttachToProcess(lldb::pid_t pid) {
   auto process_or = m_process_factory.Attach(pid, *this, m_mainloop);
   if (!process_or) {
     Status status(process_or.takeError());
     llvm::errs() << llvm::formatv("failed to attach to process {0}: {1}\n", pid,
                                   status);
     return status;
   }
   m_continue_process = m_current_process = process_or->get();
-  m_debugged_processes[m_current_process->GetID()] = std::move(*process_or);
+  m_debugged_processes.emplace(
+      m_current_process->GetID(),
+      DebuggedProcess{std::move(*process_or), DebuggedProcess::Flag{}});
   SetEnabledExtensions(*m_current_process);
 
   // Setup stdout/stderr mapping from inferior.
   auto terminal_fd = m_current_process->GetTerminalFileDescriptor();
   if (terminal_fd >= 0) {
     LLDB_LOGF(log,
               "ProcessGDBRemoteCommunicationServerLLGS::%s setting "
               "inferior STDIO fd to %d",
if (!wait_status) {
     response.PutHex8(GDBRemoteServerError::eErrorExitStatus);
     return SendPacketNoLock(response.GetString());
   }
 
   LLDB_LOG(log, "pid = {0}, returning exit type {1}", process->GetID(),
            *wait_status);
 
   // If the process was killed through vKill, return "OK".
-  if (m_vkilled_processes.find(process->GetID()) != m_vkilled_processes.end())
+  if (bool(m_debugged_processes.at(process->GetID()).flags &
+           DebuggedProcess::Flag::vkilled))
     return SendOKResponse();
 
   StreamGDBRemote response;
   response.Format("{0:g}", *wait_status);
-  if (bool(m_extensions_supported & NativeProcessProtocol::Extension::multiprocess))
+  if (bool(m_extensions_supported &
+           NativeProcessProtocol::Extension::multiprocess))
     response.Format(";process:{0:x-}", process->GetID());
   if (m_non_stop)
     return SendNotificationPacketNoLock("Stop", m_stop_notification_queue,
                                         response.GetString());
   return SendPacketNoLock(response.GetString());
 }
 
 static void AppendHexValue(StreamString &response, const uint8_t *buf,
void GDBRemoteCommunicationServerLLGS::HandleInferiorState_Exited(
 
   if (m_current_process == process)
     m_current_process = nullptr;
   if (m_continue_process == process)
     m_continue_process = nullptr;
 
   lldb::pid_t pid = process->GetID();
   m_mainloop.AddPendingCallback([this, pid](MainLoopBase &loop) {
-    m_debugged_processes.erase(pid);
-    auto vkill_it = m_vkilled_processes.find(pid);
-    if (vkill_it != m_vkilled_processes.end())
-      m_vkilled_processes.erase(vkill_it);
+    auto find_it = m_debugged_processes.find(pid);
+    assert(find_it != m_debugged_processes.end());
+    bool vkilled = bool(find_it->second.flags & DebuggedProcess::Flag::vkilled);
+    m_debugged_processes.erase(find_it);
     // Terminate the main loop only if vKill has not been used.
     // When running in non-stop mode, wait for the vStopped to clear
     // the notification queue.
-    else if (m_debugged_processes.empty() && !m_non_stop) {
+    if (m_debugged_processes.empty() && !m_non_stop && !vkilled) {
       // Close the pipe to the inferior terminal i/o if we launched it and set
       // one up.
       MaybeCloseInferiorTerminalConnection();
 
       // We are ready to exit the debug monitor.
       m_exit_now = true;
       loop.RequestTermination();
     }

 }
 
 void GDBRemoteCommunicationServerLLGS::NewSubprocess(
     NativeProcessProtocol *parent_process,
     std::unique_ptr<NativeProcessProtocol> child_process) {
   lldb::pid_t child_pid = child_process->GetID();
   assert(child_pid != LLDB_INVALID_PROCESS_ID);
   assert(m_debugged_processes.find(child_pid) == m_debugged_processes.end());
-  m_debugged_processes[child_pid] = std::move(child_process);
+  m_debugged_processes.emplace(
+      child_pid,
+      DebuggedProcess{std::move(child_process), DebuggedProcess::Flag{}});
 }
 
 void GDBRemoteCommunicationServerLLGS::DataAvailableCallback() {
   Log *log = GetLog(GDBRLog::Comm);
 
   bool interrupt = false;
   bool done = false;
   Status error;
GDBRemoteCommunicationServerLLGS::Handle_k(StringExtractorGDBRemote &packet) {
   if (m_debugged_processes.empty()) {
     LLDB_LOG(log, "No debugged process found.");
     return PacketResult::Success;
   }
 
   for (auto it = m_debugged_processes.begin(); it != m_debugged_processes.end();
        ++it) {
     LLDB_LOG(log, "Killing process {0}", it->first);
-    Status error = it->second->Kill();
+    Status error = it->second.process_up->Kill();
     if (error.Fail())
       LLDB_LOG(log, "Failed to kill debugged process {0}: {1}", it->first,
                error);
   }
 
   // The response to kill packet is undefined per the spec.  LLDB
   // follows the same rules as for continue packets, i.e. no response
   // in all-stop mode, and "OK" in non-stop mode; in both cases this
GDBRemoteCommunicationServerLLGS::Handle_vKill(
   if (pid == LLDB_INVALID_PROCESS_ID)
     return SendIllFormedResponse(packet,
                                  "vKill failed to parse the process id");
 
   auto it = m_debugged_processes.find(pid);
   if (it == m_debugged_processes.end())
     return SendErrorResponse(42);
 
-  Status error = it->second->Kill();
+  Status error = it->second.process_up->Kill();
   if (error.Fail())
     return SendErrorResponse(error.ToError());
 
   // OK response is sent when the process dies.
-  m_vkilled_processes.insert(pid);
+  it->second.flags |= DebuggedProcess::Flag::vkilled;
   return PacketResult::Success;
 }
 
 GDBRemoteCommunication::PacketResult
 GDBRemoteCommunicationServerLLGS::Handle_QSetDisableASLR(
     StringExtractorGDBRemote &packet) {
   packet.SetFilePos(::strlen("QSetDisableASLR:"));
   if (packet.GetU32(0))
GDBRemoteCommunicationServerLLGS::Handle_vCont(
   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);
+    Status error = process_it->second.process_up->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);
   }
 
GDBRemoteCommunicationServerLLGS::Handle_qfThreadInfo(
   assert(m_debugged_processes.size() == 1 ||
          bool(m_extensions_supported &
               NativeProcessProtocol::Extension::multiprocess));
 
   bool had_any = false;
   StreamGDBRemote response;
 
   for (auto &pid_ptr : m_debugged_processes)
-    AddProcessThreads(response, *pid_ptr.second, had_any);
+    AddProcessThreads(response, *pid_ptr.second.process_up, had_any);
 
   if (!had_any)
     return SendOKResponse();
   return SendPacketNoLock(response.GetString());
 }
 
 GDBRemoteCommunication::PacketResult
 GDBRemoteCommunicationServerLLGS::Handle_qsThreadInfo(
GDBRemoteCommunicationServerLLGS::Handle_H(StringExtractorGDBRemote &packet) {
   if (new_process_it == m_debugged_processes.end())
     return SendErrorResponse(llvm::make_error<StringError>(
         inconvertibleErrorCode(),
         llvm::formatv("No process with PID {0} debugged", pid)));
 
   // 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 = new_process_it->second->GetThreadByID(tid);
+    NativeThreadProtocol *thread =
+        new_process_it->second.process_up->GetThreadByID(tid);
     if (!thread) {
       LLDB_LOGF(log,
                 "GDBRemoteCommunicationServerLLGS::%s failed, tid %" PRIu64
                 " not found",
                 __FUNCTION__, tid);
       return SendErrorResponse(0x15);
     }
   }
 
   // Now switch the given process and thread type.
   switch (h_variant) {
   case 'g':
-    m_current_process = new_process_it->second.get();
+    m_current_process = new_process_it->second.process_up.get();
     SetCurrentThreadID(tid);
     break;
 
   case 'c':
-    m_continue_process = new_process_it->second.get();
+    m_continue_process = new_process_it->second.process_up.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");
   }
GDBRemoteCommunicationServerLLGS::Handle_D(StringExtractorGDBRemote &packet) {
   llvm::Error detach_error = llvm::Error::success();
   bool detached = false;
   for (auto it = m_debugged_processes.begin();
        it != m_debugged_processes.end();) {
     if (pid == LLDB_INVALID_PROCESS_ID || pid == it->first) {
       LLDB_LOGF(log,
                 "GDBRemoteCommunicationServerLLGS::%s detaching %" PRId64,
                 __FUNCTION__, it->first);
-      if (llvm::Error e = it->second->Detach().ToError())
+      if (llvm::Error e = it->second.process_up->Detach().ToError())
         detach_error = llvm::joinErrors(std::move(detach_error), std::move(e));
       else {
-        if (it->second.get() == m_current_process)
+        if (it->second.process_up.get() == m_current_process)
           m_current_process = nullptr;
-        if (it->second.get() == m_continue_process)
+        if (it->second.process_up.get() == m_continue_process)
           m_continue_process = nullptr;
         it = m_debugged_processes.erase(it);
         detached = true;
         continue;
       }
     }
     ++it;
   }
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(1);
 
   // Check the thread ID
-  if (!new_process_it->second->GetThreadByID(tid))
+  if (!new_process_it->second.process_up->GetThreadByID(tid))
     return SendErrorResponse(2);
 
   return SendOKResponse();
 }
 
 void GDBRemoteCommunicationServerLLGS::MaybeCloseInferiorTerminalConnection() {
   Log *log = GetLog(LLDBLog::Process);
 
std::vector<std::string> GDBRemoteCommunicationServerLLGS::HandleFeatures(
   if (bool(m_extensions_supported & Extension::multiprocess))
     ret.push_back("multiprocess+");
   if (bool(m_extensions_supported & Extension::fork))
     ret.push_back("fork-events+");
   if (bool(m_extensions_supported & Extension::vfork))
     ret.push_back("vfork-events+");
 
   for (auto &x : m_debugged_processes)
-    SetEnabledExtensions(*x.second);
+    SetEnabledExtensions(*x.second.process_up);
   return ret;
 }
 
 void GDBRemoteCommunicationServerLLGS::SetEnabledExtensions(
     NativeProcessProtocol &process) {
   NativeProcessProtocol::Extension flags = m_extensions_supported;
   assert(!bool(flags & ~m_process_factory.GetSupportedExtensions()));
   process.SetEnabledExtensions(flags);