[lldb] [gdb-remote] Use vKill packet if multiprocess & available

[mgorny]

Summary

Prefer the modern vKill packet over k if the server reports multiprocess
support. This ensures that when multiple processes are debugged over
a shared connection, only the correct one will be killed.

Sponsored by: The FreeBSD Foundation

[labath]

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

4268

This is a host signal number (and not really available on windows). Maybe we should just return zero or indicate that we don't know the "cause of death" in some other way? (Are we sure that there's no way to determine the cause of death?)

[mgorny]

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

4268

Either way's fine with me, I was treating it as pretty much "an arbitrary return value". The way I understand the protocol, we're supposed to assume that if vKill succeeded, then the process died because of our kill, and the client isn't really supposed to be interested in the technical details of how it died. FWICS if we issue a ? to gdbserver after a vKill, it returns W00 (also, it doesn't return it in non-stop mode at all).

lldb/packages/Python/lldbsuite/test/gdbclientutils.py

def respond(self, packet):
         if packet == "vStdio":
             return self.vStdio()
         if packet == "vStopped":
             return self.vStopped()
         if packet == "vCtrlC":
             return self.vCtrlC()
         if packet == "vCont;t":
             return self.vContT()
+        if packet.startswith("vKill;"):
+            return self.vKill(int(packet[6:], 16))
         if packet == "k":
             return self.k()
 
         return self.other(packet)
 
     def qsProcessInfo(self):
         return "E04"
 
def vCtrlC(self):
         return ""
 
     def vContT(self):
         return ""
 
     def k(self):
         return ["W01", self.RESPONSE_DISCONNECT]
 
+    def vKill(self, pid):
+        return ""
+
     """
     Raised when we receive a packet for which there is no default action.
     Override the responder class to implement behavior suitable for the test at
     hand.
     """
     class UnexpectedPacketException(Exception):
         pass
 

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

bool GDBRemoteCommunicationClient::UsesNativeSignals() {
 
   // If the remote didn't indicate native-signal support explicitly,
   // check whether it is an old version of lldb-server.
   return GetThreadSuffixSupported();
 }
 
 llvm::Expected<int> GDBRemoteCommunicationClient::KillProcess(lldb::pid_t pid) {
   StringExtractorGDBRemote response;
+  StreamString packet;
   GDBRemoteCommunication::ScopedTimeout(*this, seconds(3));
 
+  if (m_supports_multiprocess == eLazyBoolYes &&
+      pid != LLDB_INVALID_PROCESS_ID) {
+    packet.Format("vKill;{0:x-}", pid);
+    if (SendPacketAndWaitForResponse(packet.GetString(), response,
+                                     GetPacketTimeout()) !=
+        PacketResult::Success)
+      return llvm::createStringError(llvm::inconvertibleErrorCode(),
+                                     "failed to send vKill packet");
+
+    if (response.IsOKResponse())
+      return SIGKILL;
+
+    if (!response.IsUnsupportedResponse())
+      return llvm::createStringError(llvm::inconvertibleErrorCode(),
+                                     "unexpected response to vKill packet: %s",
+                                     response.GetStringRef().str().c_str());
+  }
+
   if (SendPacketAndWaitForResponse("k", response, GetPacketTimeout()) !=
       PacketResult::Success)
     return llvm::createStringError(llvm::inconvertibleErrorCode(),
                                    "failed to send k packet");
 
   char packet_cmd = response.GetChar(0);
   if (packet_cmd == 'W' || packet_cmd == 'X')
     return response.GetHexU8();
 
   return llvm::createStringError(llvm::inconvertibleErrorCode(),
                                  "unexpected response to k packet: %s",
                                  response.GetStringRef().str().c_str());
 }

lldb/test/API/functionalities/gdb_remote_client/TestMultiprocess.py

def test_stop_reason(self):
           self.addTearDownHook(
                 lambda: self.runCmd("log disable gdb-remote packets"))
         process = self.connect(target)
         process.Continue()
         self.assertEqual(process.GetThreadByID(0x10200).stop_reason,
                          lldb.eStopReasonSignal)
         self.assertEqual(process.GetThreadByID(0x10204).stop_reason,
                          lldb.eStopReasonNone)
+
+    def test_kill_no_mp(self):
+        class MyResponder(MockGDBServerResponder):
+            got_vKill = False
+
+            def qfThreadInfo(self):
+                return "mp100.100"
+
+            def vKill(self, pid):
+                self.got_vKill = True
+                return "E01"
+
+            def k(self):
+                return "X01"
+
+        self.server.responder = MyResponder()
+        target = self.dbg.CreateTarget("")
+        process = self.connect(target)
+        lldbutil.expect_state_changes(self, self.dbg.GetListener(), process,
+                                      [lldb.eStateStopped])
+        process.Kill()
+        lldbutil.expect_state_changes(self, self.dbg.GetListener(), process,
+                                      [lldb.eStateExited])
+        self.assertPacketLogContains(["k"])
+        self.assertFalse(self.server.responder.got_vKill)
+        self.assertEqual(process.GetExitStatus(), 1)
+
+    def test_kill_mp(self):
+        class MyResponder(MockGDBServerResponder):
+            got_k = False
+
+            def __init__(self, test_case):
+                super().__init__()
+                self.test_case = test_case
+
+            def qSupported(self, client_supported):
+                self.test_case.assertIn("multiprocess+", client_supported)
+                return "multiprocess+;" + super().qSupported(client_supported)
+
+            def qfThreadInfo(self):
+                return "mp100.100"
+
+            def vKill(self, pid):
+                return "OK"
+
+            def k(self):
+                self.got_k = True
+                return "X00"
+
+        self.server.responder = MyResponder(self)
+        target = self.dbg.CreateTarget("")
+        process = self.connect(target)
+        lldbutil.expect_state_changes(self, self.dbg.GetListener(), process,
+                                      [lldb.eStateStopped])
+        process.Kill()
+        lldbutil.expect_state_changes(self, self.dbg.GetListener(), process,
+                                      [lldb.eStateExited])
+        self.assertPacketLogContains(["vKill;100"])
+        self.assertFalse(self.server.responder.got_k)
+        self.assertEqual(process.GetExitStatus(),
+                         lldbutil.get_signal_number("SIGKILL"))
+
+    def test_kill_mp_no_vKill(self):
+        class MyResponder(MockGDBServerResponder):
+            def __init__(self, test_case):
+                super().__init__()
+                self.test_case = test_case
+
+            def qSupported(self, client_supported):
+                self.test_case.assertIn("multiprocess+", client_supported)
+                return "multiprocess+;" + super().qSupported(client_supported)
+
+            def qC(self):
+                return "QC100"
+
+            def qfThreadInfo(self):
+                return "mp100.100"
+
+            def cont(self):
+                return "S02"
+
+            def k(self):
+                return "X01"
+
+        self.server.responder = MyResponder(self)
+        target = self.dbg.CreateTarget("")
+        process = self.connect(target)
+        lldbutil.expect_state_changes(self, self.dbg.GetListener(), process,
+                                      [lldb.eStateStopped])
+        process.Kill()
+        lldbutil.expect_state_changes(self, self.dbg.GetListener(), process,
+                                      [lldb.eStateExited])
+        self.assertPacketLogContains(["vKill;100", "k"])
+        self.assertEqual(process.GetExitStatus(), 1)