labath created this revision.
labath added reviewers: JDevlieghere, jingham.
labath requested review of this revision.
Herald added a project: LLDB.
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.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D113184
Files:
lldb/packages/Python/lldbsuite/test/lldbtest.py
lldb/source/Target/Platform.cpp
lldb/test/API/functionalities/gdb_remote_client/Makefile
lldb/test/API/functionalities/gdb_remote_client/TestPlatformKill.py
lldb/test/API/functionalities/gdb_remote_client/gdbclientutils.py
lldb/test/API/functionalities/gdb_remote_client/sleep.cpp
Index: lldb/test/API/functionalities/gdb_remote_client/sleep.cpp
===================================================================
--- /dev/null
+++ lldb/test/API/functionalities/gdb_remote_client/sleep.cpp
@@ -0,0 +1,6 @@
+#include <thread>
+
+int main() {
+ std::this_thread::sleep_for(std::chrono::minutes(1));
+ return 0;
+}
Index: lldb/test/API/functionalities/gdb_remote_client/gdbclientutils.py
===================================================================
--- lldb/test/API/functionalities/gdb_remote_client/gdbclientutils.py
+++ lldb/test/API/functionalities/gdb_remote_client/gdbclientutils.py
@@ -203,6 +203,8 @@
if packet.startswith("qRegisterInfo"):
regnum = int(packet[len("qRegisterInfo"):], 16)
return self.qRegisterInfo(regnum)
+ if packet == "k":
+ return self.k()
return self.other(packet)
@@ -331,6 +333,9 @@
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
Index: lldb/test/API/functionalities/gdb_remote_client/TestPlatformKill.py
===================================================================
--- /dev/null
+++ lldb/test/API/functionalities/gdb_remote_client/TestPlatformKill.py
@@ -0,0 +1,47 @@
+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)
Index: lldb/test/API/functionalities/gdb_remote_client/Makefile
===================================================================
--- /dev/null
+++ lldb/test/API/functionalities/gdb_remote_client/Makefile
@@ -0,0 +1 @@
+include Makefile.rules
Index: lldb/source/Target/Platform.cpp
===================================================================
--- lldb/source/Target/Platform.cpp
+++ lldb/source/Target/Platform.cpp
@@ -1044,25 +1044,11 @@
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();
}
Index: lldb/packages/Python/lldbsuite/test/lldbtest.py
===================================================================
--- lldb/packages/Python/lldbsuite/test/lldbtest.py
+++ lldb/packages/Python/lldbsuite/test/lldbtest.py
@@ -422,6 +422,9 @@
def poll(self):
return self._proc.poll()
+ def wait(self, timeout=None):
+ return self._proc.wait(timeout)
+
class _RemoteProcess(_BaseProcess):
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
