From 58b54894c7b66a565e9dcf5236ff1425ab8053dc Mon Sep 17 00:00:00 2001 From: Pavel Labath Date: Mon, 14 May 2018 15:13:13 +0000 Subject: [PATCH] Remove Process references from the Host module The Process class was only being referenced because of the last-ditch effort in the process launchers to set a process death callback in case one isn't set already. Although launching a process for debugging is the most important kind of "launch" we are doing, it is by far not the only one, so assuming this particular callback is the one to be used is not a good idea (besides breaking layering). Instead of assuming a particular exit callback, I change the launcher code to require the callback to be set by the user (and fix up the two call sites which did not set the callback already). Reviewers: jingham, davide Subscribers: lldb-commits Differential Revision: https://reviews.llvm.org/D46395 llvm-svn: 332250 --- lldb/include/lldb/Host/Host.h | 3 +++ .../lldb/Host/MonitoringProcessLauncher.h | 3 +++ lldb/include/lldb/Target/ProcessLaunchInfo.h | 6 ++++++ .../Host/common/MonitoringProcessLauncher.cpp | 16 +++------------- .../source/Host/common/NativeProcessProtocol.cpp | 1 - lldb/source/Host/macosx/Host.mm | 14 ++++---------- .../Plugins/Platform/POSIX/PlatformPOSIX.cpp | 11 ++++++----- lldb/source/Target/ProcessLaunchInfo.cpp | 7 +++++++ .../tools/lldb-server/tests/TestClient.cpp | 5 +++++ 9 files changed, 37 insertions(+), 29 deletions(-) diff --git a/lldb/include/lldb/Host/Host.h b/lldb/include/lldb/Host/Host.h index c6d5c3c4b67a..459e9f563d1e 100644 --- a/lldb/include/lldb/Host/Host.h +++ b/lldb/include/lldb/Host/Host.h @@ -199,6 +199,9 @@ public: static const lldb::UnixSignalsSP &GetUnixSignals(); + /// Launch the process specified in launch_info. The monitoring callback in + /// launch_info must be set, and it will be called when the process + /// terminates. static Status LaunchProcess(ProcessLaunchInfo &launch_info); //------------------------------------------------------------------ diff --git a/lldb/include/lldb/Host/MonitoringProcessLauncher.h b/lldb/include/lldb/Host/MonitoringProcessLauncher.h index 9ad36e90a779..341284800a4e 100644 --- a/lldb/include/lldb/Host/MonitoringProcessLauncher.h +++ b/lldb/include/lldb/Host/MonitoringProcessLauncher.h @@ -24,6 +24,9 @@ public: explicit MonitoringProcessLauncher( std::unique_ptr delegate_launcher); + /// Launch the process specified in launch_info. The monitoring callback in + /// launch_info must be set, and it will be called when the process + /// terminates. HostProcess LaunchProcess(const ProcessLaunchInfo &launch_info, Status &error) override; diff --git a/lldb/include/lldb/Target/ProcessLaunchInfo.h b/lldb/include/lldb/Target/ProcessLaunchInfo.h index fc7cdea04dfe..92c517a3e460 100644 --- a/lldb/include/lldb/Target/ProcessLaunchInfo.h +++ b/lldb/include/lldb/Target/ProcessLaunchInfo.h @@ -107,6 +107,12 @@ public: return m_monitor_callback; } + /// A Monitor callback which does not take any action on process events. Use + /// this if you don't need to take any particular action when the process + /// terminates, but you still need to reap it. + static bool NoOpMonitorCallback(lldb::pid_t pid, bool exited, int signal, + int status); + bool GetMonitorSignals() const { return m_monitor_signals; } // If the LaunchInfo has a monitor callback, then arrange to monitor the diff --git a/lldb/source/Host/common/MonitoringProcessLauncher.cpp b/lldb/source/Host/common/MonitoringProcessLauncher.cpp index efc10004b5fd..76c11454f573 100644 --- a/lldb/source/Host/common/MonitoringProcessLauncher.cpp +++ b/lldb/source/Host/common/MonitoringProcessLauncher.cpp @@ -9,7 +9,6 @@ #include "lldb/Host/MonitoringProcessLauncher.h" #include "lldb/Host/HostProcess.h" -#include "lldb/Target/Process.h" #include "lldb/Target/ProcessLaunchInfo.h" #include "lldb/Utility/Log.h" #include "lldb/Utility/Status.h" @@ -58,18 +57,9 @@ MonitoringProcessLauncher::LaunchProcess(const ProcessLaunchInfo &launch_info, if (process.GetProcessId() != LLDB_INVALID_PROCESS_ID) { Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_PROCESS)); - Host::MonitorChildProcessCallback callback = - launch_info.GetMonitorProcessCallback(); - - bool monitor_signals = false; - if (callback) { - // If the ProcessLaunchInfo specified a callback, use that. - monitor_signals = launch_info.GetMonitorSignals(); - } else { - callback = Process::SetProcessExitStatus; - } - - process.StartMonitoring(callback, monitor_signals); + assert(launch_info.GetMonitorProcessCallback()); + process.StartMonitoring(launch_info.GetMonitorProcessCallback(), + launch_info.GetMonitorSignals()); if (log) log->PutCString("started monitoring child process."); } else { diff --git a/lldb/source/Host/common/NativeProcessProtocol.cpp b/lldb/source/Host/common/NativeProcessProtocol.cpp index cc998e6da9ff..3e8648f81473 100644 --- a/lldb/source/Host/common/NativeProcessProtocol.cpp +++ b/lldb/source/Host/common/NativeProcessProtocol.cpp @@ -13,7 +13,6 @@ #include "lldb/Host/common/NativeRegisterContext.h" #include "lldb/Host/common/NativeThreadProtocol.h" #include "lldb/Host/common/SoftwareBreakpoint.h" -#include "lldb/Target/Process.h" #include "lldb/Utility/LLDBAssert.h" #include "lldb/Utility/Log.h" #include "lldb/lldb-enumerations.h" diff --git a/lldb/source/Host/macosx/Host.mm b/lldb/source/Host/macosx/Host.mm index d87c6be915ce..c2aea0afb882 100644 --- a/lldb/source/Host/macosx/Host.mm +++ b/lldb/source/Host/macosx/Host.mm @@ -57,7 +57,6 @@ #include "lldb/Host/ConnectionFileDescriptor.h" #include "lldb/Host/HostInfo.h" #include "lldb/Host/ThreadLauncher.h" -#include "lldb/Target/Process.h" #include "lldb/Utility/ArchSpec.h" #include "lldb/Utility/CleanUp.h" #include "lldb/Utility/DataBufferHeap.h" @@ -68,6 +67,7 @@ #include "lldb/Utility/NameMatches.h" #include "lldb/Utility/StreamString.h" #include "lldb/Utility/StructuredData.h" +#include "lldb/lldb-defines.h" #include "llvm/Support/FileSystem.h" #include "llvm/Support/Errno.h" @@ -1497,15 +1497,9 @@ Status Host::LaunchProcess(ProcessLaunchInfo &launch_info) { launch_info.SetProcessID(pid); // Make sure we reap any processes we spawn or we will have zombies. - if (!launch_info.MonitorProcess()) { - const bool monitor_signals = false; - Host::MonitorChildProcessCallback callback = nullptr; - - if (!launch_info.GetFlags().Test(lldb::eLaunchFlagDontSetExitStatus)) - callback = Process::SetProcessExitStatus; - - StartMonitoringChildProcess(callback, pid, monitor_signals); - } + bool monitoring = launch_info.MonitorProcess()); + UNUSED_IF_ASSERT_DISABLED(monitoring); + assert(monitoring); } else { // Invalid process ID, something didn't go well if (error.Success()) diff --git a/lldb/source/Plugins/Platform/POSIX/PlatformPOSIX.cpp b/lldb/source/Plugins/Platform/POSIX/PlatformPOSIX.cpp index 9b2c86a5f686..eeff8f3518ae 100644 --- a/lldb/source/Plugins/Platform/POSIX/PlatformPOSIX.cpp +++ b/lldb/source/Plugins/Platform/POSIX/PlatformPOSIX.cpp @@ -868,11 +868,12 @@ PlatformPOSIX::DebugProcess(ProcessLaunchInfo &launch_info, Debugger &debugger, if (IsHost()) { // We are going to hand this process off to debugserver which will be in - // charge of setting the exit status. We still need to reap it from lldb - // but if we let the monitor thread also set the exit status, we set up a - // race between debugserver & us for who will find out about the debugged - // process's death. - launch_info.GetFlags().Set(eLaunchFlagDontSetExitStatus); + // charge of setting the exit status. However, we still need to reap it + // from lldb. So, make sure we use a exit callback which does not set exit + // status. + const bool monitor_signals = false; + launch_info.SetMonitorProcessCallback( + &ProcessLaunchInfo::NoOpMonitorCallback, monitor_signals); process_sp = Platform::DebugProcess(launch_info, debugger, target, error); } else { if (m_remote_platform_sp) diff --git a/lldb/source/Target/ProcessLaunchInfo.cpp b/lldb/source/Target/ProcessLaunchInfo.cpp index 60fe26bb60da..9569750bc5fd 100644 --- a/lldb/source/Target/ProcessLaunchInfo.cpp +++ b/lldb/source/Target/ProcessLaunchInfo.cpp @@ -189,6 +189,13 @@ void ProcessLaunchInfo::SetMonitorProcessCallback( m_monitor_signals = monitor_signals; } +bool ProcessLaunchInfo::NoOpMonitorCallback(lldb::pid_t pid, bool exited, int signal, int status) { + Log *log = lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_PROCESS); + LLDB_LOG(log, "pid = {0}, exited = {1}, signal = {2}, status = {3}", pid, + exited, signal, status); + return true; +} + bool ProcessLaunchInfo::MonitorProcess() const { if (m_monitor_callback && ProcessIDIsValid()) { Host::StartMonitoringChildProcess(m_monitor_callback, GetProcessID(), diff --git a/lldb/unittests/tools/lldb-server/tests/TestClient.cpp b/lldb/unittests/tools/lldb-server/tests/TestClient.cpp index 2410b6270f28..3dd5be2c6e8e 100644 --- a/lldb/unittests/tools/lldb-server/tests/TestClient.cpp +++ b/lldb/unittests/tools/lldb-server/tests/TestClient.cpp @@ -97,6 +97,11 @@ Expected> TestClient::launchCustom(StringRef Log, Ar Info.SetArchitecture(arch_spec); Info.SetArguments(args, true); Info.GetEnvironment() = Host::GetEnvironment(); + // TODO: Use this callback to detect botched launches. If lldb-server does not + // start, we can print a nice error message here instead of hanging in + // Accept(). + Info.SetMonitorProcessCallback(&ProcessLaunchInfo::NoOpMonitorCallback, + false); status = Host::LaunchProcess(Info); if (status.Fail())