[lldb] Fix cross-platform kills

[labath]

Summary

This patch fixes an amusing bug where a Platform::Kill operation would
happily terminate a proces on a completely different platform, as long
as they have the same process ID. This was due to the fact that the
implementation was iterating through all known (debugged) processes in
order terminate them directly.

This patch just deletes that logic, and makes everything go through the
OS process termination APIs. While it would be possible to fix the logic
to check for a platform match, it seemed to me that the implementation
was being too smart for its own good -- accessing random Process
objects without knowing anything about their state is risky at best.
Going through the os ensures we avoid any races.

I also "upgrade" the termination signal to a SIGKILL to ensure the
process really dies after this operation.

[JDevlieghere]

LGTM

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

18–28

I always appreciate the elegance of these tests.

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

def terminate(self):
             if self._proc.poll() is not None:
                 return
             self._proc.kill()
             time.sleep(self._delayafterterminate)
 
     def poll(self):
         return self._proc.poll()
 
+    def wait(self, timeout=None):
+        return self._proc.wait(timeout)
+
 
 class _RemoteProcess(_BaseProcess):
 
     def __init__(self, install_remote):
         self._pid = None
         self._install_remote = install_remote
 
     @property

lldb/source/Target/Platform.cpp

if (IsHost())
     return Host::ShellExpandArguments(launch_info);
   return Status("base lldb_private::Platform class can't expand arguments");
 }
 
 Status Platform::KillProcess(const lldb::pid_t pid) {
   Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_PLATFORM));
   LLDB_LOGF(log, "Platform::%s, pid %" PRIu64, __FUNCTION__, pid);
 
-  // Try to find a process plugin to handle this Kill request.  If we can't,
-  // fall back to the default OS implementation.
-  size_t num_debuggers = Debugger::GetNumDebuggers();
-  for (size_t didx = 0; didx < num_debuggers; ++didx) {
-    DebuggerSP debugger = Debugger::GetDebuggerAtIndex(didx);
-    lldb_private::TargetList &targets = debugger->GetTargetList();
-    for (int tidx = 0; tidx < targets.GetNumTargets(); ++tidx) {
-      ProcessSP process = targets.GetTargetAtIndex(tidx)->GetProcessSP();
-      if (process->GetID() == pid)
-        return process->Destroy(true);
-    }
-  }
-
   if (!IsHost()) {
     return Status(
-        "base lldb_private::Platform class can't kill remote processes unless "
-        "they are controlled by a process plugin");
+        "base lldb_private::Platform class can't kill remote processes");
   }
-  Host::Kill(pid, SIGTERM);
+  Host::Kill(pid, SIGKILL);
   return Status();
 }
 
 lldb::ProcessSP Platform::DebugProcess(ProcessLaunchInfo &launch_info,
                                        Debugger &debugger, Target &target,
                                        Status &error) {
   Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_PLATFORM));
   LLDB_LOG(log, "target = {0})", &target);

lldb/test/API/functionalities/gdb_remote_client/Makefile

+include Makefile.rules

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

+import lldb
+import time
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test.decorators import *
+from gdbclientutils import *
+
+class TestPlatformKill(GDBRemoteTestBase):
+
+    @skipIfRemote
+    def test_kill_different_platform(self):
+        """Test connecting to a remote linux platform"""
+
+        self.build(dictionary={"CXX_SOURCES":"sleep.cpp"})
+        host_process = self.spawnSubprocess(self.getBuildArtifact())
+
+        # Create a fake remote process with the same PID as host_process
+        class MyResponder(MockGDBServerResponder):
+            def __init__(self):
+                MockGDBServerResponder.__init__(self)
+                self.got_kill = False
+
+            def qC(self):
+                return "QC%x"%host_process.pid
+
+            def k(self):
+                self.got_kill = True
+                return "X09"
+
+        self.server.responder = MyResponder()
+
+        error = lldb.SBError()
+        target = self.dbg.CreateTarget("", "x86_64-pc-linux", "remote-linux",
+                False, error)
+        self.assertSuccess(error)
+        process = self.connect(target)
+        self.assertEqual(process.GetProcessID(), host_process.pid)
+
+        host_platform = lldb.SBPlatform("host")
+        self.assertSuccess(host_platform.Kill(host_process.pid))
+
+        # Host dies, remote process lives.
+        self.assertFalse(self.server.responder.got_kill)
+        self.assertIsNotNone(host_process.wait(timeout=10))
+
+        # Now kill the remote one as well
+        self.assertSuccess(process.Kill())
+        self.assertTrue(self.server.responder.got_kill)

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

def respond(self, packet):
             return self.qLaunchSuccess()
         if packet.startswith("QEnvironment:"):
             return self.QEnvironment(packet)
         if packet.startswith("QEnvironmentHexEncoded:"):
             return self.QEnvironmentHexEncoded(packet)
         if packet.startswith("qRegisterInfo"):
             regnum = int(packet[len("qRegisterInfo"):], 16)
             return self.qRegisterInfo(regnum)
+        if packet == "k":
+            return self.k()
 
         return self.other(packet)
 
     def qsProcessInfo(self):
         return "E04"
 
     def qfProcessInfo(self, packet):
         return "E04"
def QEnvironment(self, packet):
         return "OK"
 
     def QEnvironmentHexEncoded(self, packet):
         return "OK"
 
     def qRegisterInfo(self, num):
         return ""
 
+    def k(self):
+        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/test/API/functionalities/gdb_remote_client/sleep.cpp

+#include <thread>
+
+int main() {
+  std::this_thread::sleep_for(std::chrono::minutes(1));
+  return 0;
+}