[lldb] [llgs] Make `k` kill all processes, and fix multiple exits

[mgorny]

Summary

Modify the behavior of the k packet to kill all inferiors rather than
just the current one. The specification leaves the exact behavior
of this packet up to the implementation but since vKill is specifically
meant to be used to kill a single process, it seems logical to use k
to provide the alternate function of killing all of them.

Move starting stdio forwarding from the "running" response
to the packet handlers that trigger the process to start. This avoids
attempting to start it multiple times when multiple processes are killed
on Linux which implicitly causes LLGS to receive "started" events
for all of them. This is probably also more correct as the ability
to send "O" packets is implied by the continue-like command being issued
(and therefore the client waiting for responses) rather than the start
notification.

Sponsored by: The FreeBSD Foundation

[mgorny]

I've gotten so absorbed by fixing server code that I've forgotten to actually finish the test case ;-).

[labath]

I've ran into the "dynamic uninitialization order fiasco" problem as well. I'm wondering if, instead of the trick (which is pretty neat, but still a trick) with the limbo member variable, we could add something like the asyncio.loop.call_soon function in python to our MainLoop class. So, instead of immediately calling the exited callback (and possibly others as well), the process class would schedule the call to be made upon return to the top-level loop.

What do you think about that?

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

1232

I don't think this is right. I believe the important distinction is whether we have non-stop mode enabled or not, because in all-stop mode we are not able to send stdio while stopped, regardless of how many processes we're debugging.

[mgorny]

In D127500#3595605, @labath wrote:

I've ran into the "dynamic uninitialization order fiasco" problem as well. I'm wondering if, instead of the trick (which is pretty neat, but still a trick) with the limbo member variable, we could add something like the asyncio.loop.call_soon function in python to our MainLoop class. So, instead of immediately calling the exited callback (and possibly others as well), the process class would schedule the call to be made upon return to the top-level loop.

What do you think about that?

Not sure if I follow you. If you mean delaying the whole callback logic, I don't think that would change anything — the problem is that the object gets destroyed in the middle of it. If you mean delaying just the part where it's removed from m_debugged_processes, then I suppose that could work.

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

1232

Well, the code exploded in all-stop mode as well because in order to kill multiple processes, we effectively resume them all. I suppose there could be a way around it (synchronously killing one process after another and waiting for everything to happen) but I'm not convinced this is really worth the added complexity.

[mgorny]

Updated to delay removing the process (and therefore stopping lldb-server) until the last process exits.

[mgorny]

This is now pending rebase due to the nonstop patch landing.

[mgorny]

Rebase and fix for nonstop.

[labath]

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

1050

The universal reference capture is dangerous for callbacks that outlive the enclosing function. I think that [this,pid] would suffice.

1232

I don't think this needs to be complex at all. What we need to basically do, is call StartSTDIOForwarding whenever the protocol allows us to send stdio, and StopSTDIOForwarding when we cannot. When we supported just a single process, the easiest way to achieve this was to hook into the started/stopped events of that process. Now that's no longer true, so maybe we just need to hook it up elsewhere.

I think starting could be done directly from the packet handlers (c/s/vCont/...). And those handlers already have to check for nonstop mode, so any deviations could be handled there:

if (all_stop) {
  StartSTDIOForwarding(); // Can forward from now until the stop-reply packet is send
  return Success;
} else {
  SendOKResponse();
  // Can we forward now? Or maybe it can be sent all the time?
}

Stopping could probably stay coupled with the sending of the stop-reply packet (i.e., handling of the process event), since it's the stop reply which determines whether the stdio packet can be sent.

[mgorny]

Limited variables passed into the lambda. Reworked starting stdio forwarding per the suggestion.

TODO: rework stopping.

[mgorny]

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

1232

Thanks, this makes sense. Good news is that it seems that I can just wire it into our current SendContinueSuccessResponse(), unless I've missed some other case (I've grepped for everything calling Resume() or Kill()). So far I didn't special-case non-stop mode, as I still need to rework O packets for it.

That said, do we want to enable forwarding for kill actions at all? I suppose this was kinda implicit due to the whole Linux PTRACE_EVENT_EXIT ping-pong but I honestly doubt any output can happen there.

[mgorny]

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

1232

Oh, and regarding non-stop mode, I've left it as TODO for now. The whole stdio forwarding needs to be fixed for non-stop mode anyway, and since we don't expect to have two processes running simultaneously yet, I'm going to look into it separately.

That said, this is probably going to be a "LLDB extension". FWICS gdb pretty much uses O packets only to deliver debugger-specific messages and doesn't forward stdio at all, nor special-cases O in non-stop mode. My initial idea is to always send O as asynchronous notifications. I suppose we could then enable stdio forwarding as soon as non-stop mode is enabled, and keep it enabled until the very end.

[mgorny]

Remove the extra check from StopSTDIOForwarding() — we currently don't have to account for multiple processes actually running simultaneously (only the weird Linux PTRACE_EVENT_EXIT handling), so we can just stop it when the first of killed processes exits.

[labath]

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

1232

Using async notifications sounds reasonable. Also, ew, I did not realize that's what O packets were meant for.

I don't think we need to enable forwarding for kill packets. In fact, I would be surprised if lldb was prepared to handle them here.

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

PacketResult result = SendStopReasonForState(
       *process, StateType::eStateExited, /*force_synchronous=*/false);
   if (result != PacketResult::Success) {
     LLDB_LOGF(log,
               "GDBRemoteCommunicationServerLLGS::%s failed to send stop "
               "notification for PID %" PRIu64 ", state: eStateExited",
               __FUNCTION__, process->GetID());
   }
 
-  // Close the pipe to the inferior terminal i/o if we launched it and set one
-  // up.
-  MaybeCloseInferiorTerminalConnection();
+  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);
     // When running in non-stop mode, wait for the vStopped to clear
     // the notification queue.
-  if (!m_non_stop) {
+    if (m_debugged_processes.empty() && !m_non_stop) {
+      // 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;
-    m_mainloop.RequestTermination();
+      loop.RequestTermination();
     }
+  });
 }
 
 void GDBRemoteCommunicationServerLLGS::HandleInferiorState_Stopped(
     NativeProcessProtocol *process) {
   assert(process && "process cannot be NULL");
 
   Log *log = GetLog(LLDBLog::Process);
   LLDB_LOGF(log, "GDBRemoteCommunicationServerLLGS::%s called", __FUNCTION__);
if (log) {
     LLDB_LOGF(log,
               "GDBRemoteCommunicationServerLLGS::%s called with "
               "NativeProcessProtocol pid %" PRIu64 ", state: %s",
               __FUNCTION__, process->GetID(), StateAsCString(state));
   }
 
   switch (state) {
   case StateType::eStateRunning:
-    StartSTDIOForwarding();
     break;
 
   case StateType::eStateStopped:
     // Make sure we get all of the pending stdout/stderr from the inferior and
     // send it to the lldb host before we send the state change notification
     SendProcessOutput();
     // Then stop the forwarding, so that any late output (see llvm.org/pr25652)
     // does not interfere with our protocol.

 }
 
 void GDBRemoteCommunicationServerLLGS::StartSTDIOForwarding() {
   // Don't forward if not connected (e.g. when attaching).
   if (!m_stdio_communication.IsConnected())
     return;
 
   Status error;
-  lldbassert(!m_stdio_handle_up);
+  assert(!m_stdio_handle_up);
   m_stdio_handle_up = m_mainloop.RegisterReadObject(
       m_stdio_communication.GetConnection()->GetReadObject(),
       [this](MainLoopBase &) { SendProcessOutput(); }, error);
 
   if (!m_stdio_handle_up) {
     // Not much we can do about the failure. Log it and continue without
     // forwarding.
     if (Log *log = GetLog(LLDBLog::Process))
-      LLDB_LOGF(log,
-                "GDBRemoteCommunicationServerLLGS::%s Failed to set up stdio "
-                "forwarding: %s",
-                __FUNCTION__, error.AsCString());
+      LLDB_LOG(log, "Failed to set up stdio forwarding: {0}", error);
   }
 }
 
 void GDBRemoteCommunicationServerLLGS::StopSTDIOForwarding() {
   m_stdio_handle_up.reset();
 }
 
 void GDBRemoteCommunicationServerLLGS::SendProcessOutput() {

 }
 
 GDBRemoteCommunication::PacketResult
 GDBRemoteCommunicationServerLLGS::Handle_k(StringExtractorGDBRemote &packet) {
   Log *log = GetLog(LLDBLog::Process);
 
   StopSTDIOForwarding();
 
-  if (!m_current_process) {
+  if (m_debugged_processes.empty()) {
     LLDB_LOG(log, "No debugged process found.");
     return PacketResult::Success;
   }
 
-  Status error = m_current_process->Kill();
+  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();
     if (error.Fail())
-    LLDB_LOG(log, "Failed to kill debugged process {0}: {1}",
-             m_current_process->GetID(), error);
+      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
   // is followed by the actual stop reason.
   return SendContinueSuccessResponse();
 }
 
void GDBRemoteCommunicationServerLLGS::SetContinueThreadID(lldb::tid_t tid) {
   m_continue_tid = tid;
 }
 
 GDBRemoteCommunication::PacketResult
 GDBRemoteCommunicationServerLLGS::Handle_stop_reason(
     StringExtractorGDBRemote &packet) {
   // Handle the $? gdbremote command.
 
-  // If no process, indicate error
-  if (!m_current_process)
-    return SendErrorResponse(02);
-
   if (m_non_stop) {
     // Clear the notification queue first, except for pending exit
     // notifications.
     llvm::erase_if(m_stop_notification_queue, [](const std::string &x) {
       return x.front() != 'W' && x.front() != 'X';
     });
 
-    // Queue stop reply packets for all active threads.  Start with the current
-    // thread (for clients that don't actually support multiple stop reasons).
+    if (m_current_process) {
+      // Queue stop reply packets for all active threads.  Start with
+      // the current thread (for clients that don't actually support multiple
+      // stop reasons).
       NativeThreadProtocol *thread = m_current_process->GetCurrentThread();
       if (thread)
         m_stop_notification_queue.push_back(
             PrepareStopReplyPacketForThread(*thread).GetString().str());
-    EnqueueStopReplyPackets(thread ? thread->GetID() : LLDB_INVALID_THREAD_ID);
+      EnqueueStopReplyPackets(thread ? thread->GetID()
+                                     : LLDB_INVALID_THREAD_ID);
+    }
 
     // If the notification queue is empty (i.e. everything is running), send OK.
     if (m_stop_notification_queue.empty())
       return SendOKResponse();
 
     // Send the first item from the new notification queue synchronously.
     return SendPacketNoLock(m_stop_notification_queue.front());
   }
 
+  // If no process, indicate error
+  if (!m_current_process)
+    return SendErrorResponse(02);
+
   return SendStopReasonForState(*m_current_process,
                                 m_current_process->GetState(),
                                 /*force_synchronous=*/true);
 }
 
 GDBRemoteCommunication::PacketResult
 GDBRemoteCommunicationServerLLGS::SendStopReasonForState(
     NativeProcessProtocol &process, lldb::StateType process_state,
void GDBRemoteCommunicationServerLLGS::SetEnabledExtensions(
     NativeProcessProtocol &process) {
   NativeProcessProtocol::Extension flags = m_extensions_supported;
   assert(!bool(flags & ~m_process_factory.GetSupportedExtensions()));
   process.SetEnabledExtensions(flags);
 }
 
 GDBRemoteCommunication::PacketResult
 GDBRemoteCommunicationServerLLGS::SendContinueSuccessResponse() {
+  // TODO: how to handle forwarding in non-stop mode?
+  StartSTDIOForwarding();
   return m_non_stop ? SendOKResponse() : PacketResult::Success;
 }
 
 std::string
 lldb_private::process_gdb_remote::LLGSArgToURL(llvm::StringRef url_arg,
                                                bool reverse_connect) {
   // Try parsing the argument as URL.
   if (llvm::Optional<URI> url = URI::Parse(url_arg)) {

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

def test_detach_all(self):
             "send packet: $OK#00",
             # verify that both PIDs are invalid now
             "read packet: $Hgp{}.{}#00".format(parent_pid, parent_tid),
             "send packet: $Eff#00",
             "read packet: $Hgp{}.{}#00".format(child_pid, child_tid),
             "send packet: $Eff#00",
         ], True)
         self.expect_gdbremote_sequence()
+
+    @add_test_categories(["fork"])
+    def test_kill_all(self):
+        self.build()
+        self.prep_debug_monitor_and_inferior(inferior_args=["fork"])
+        self.add_qSupported_packets(["multiprocess+",
+                                     "fork-events+"])
+        ret = self.expect_gdbremote_sequence()
+        self.assertIn("fork-events+", ret["qSupported_response"])
+        self.reset_test_sequence()
+
+        # continue and expect fork
+        self.test_sequence.add_log_lines([
+            "read packet: $c#00",
+            {"direction": "send", "regex": self.fork_regex.format("fork"),
+             "capture": self.fork_capture},
+        ], True)
+        ret = self.expect_gdbremote_sequence()
+        parent_pid = ret["parent_pid"]
+        child_pid = ret["child_pid"]
+        self.reset_test_sequence()
+
+        exit_regex = "[$]X09;process:([0-9a-f]+)#.*"
+        self.test_sequence.add_log_lines([
+            # kill all processes
+            "read packet: $k#00",
+            {"direction": "send", "regex": exit_regex,
+             "capture": {1: "pid1"}},
+            {"direction": "send", "regex": exit_regex,
+             "capture": {1: "pid2"}},
+        ], True)
+        ret = self.expect_gdbremote_sequence()
+        self.assertEqual(set([ret["pid1"], ret["pid2"]]),
+                         set([parent_pid, child_pid]))