Remove Process references from the Host module

[labath]

Summary

The Process class was only being referenced because of the last-ditch
effort in the process launchers to set a process exit 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).

[jingham]

This is a cleaner approach. Might be worth adding a comment in the MonitoringProcessLauncher.h saying that the monitoring callback in the ProcessLaunchInfo is required? You might from the name assume that it was doing monitoring for you.

[davide]

lg

[clayborg]

source/Host/macosx/Host.mm

1501

Do we not have a macro for silencing unused variables?

source/Plugins/Platform/POSIX/PlatformPOSIX.cpp

873–874

Now we won't reap the process from within LLDB or is the comment is wrong? This looks wrong. Also, if we truly have places that don't need to reap the process, then we should add a "launch_info.SetDefaultDontReapCallback()" or something to launch_info that will install a default do nothing callback instead of one or more clients doing this

unittests/tools/lldb-server/tests/TestClient.cpp

104

I know on Mac we still must reap the process from both the process that launches (lldb) it and the ptrace parent process (lldb-server or debugserver). This will create zombies and buildbots will die horribly if this is wrong

[labath]

I've added a comment about the requirement to the MonitoringProcessLauncher and to Host::LaunchProcess (most users go through the latter). I've also tried to make it more obvious that the no-op callback still causes the process to be reaped. Let me know if I succeeded.

source/Host/macosx/Host.mm

1501

For me, the UNUSED_IF_ASSERT_DISABLED macro does not seem to add any value, and it is inconsistent with the llvm style. But I don't feel that strongly, so I've changed this to use the macro instead.

source/Plugins/Platform/POSIX/PlatformPOSIX.cpp

873–874

The comment is correct. We still reap (waitpid) the child; we don't set the exit status on the Process class. The reaping is done in Host::StartMonitoringChildProcess (on macosx), and this function calls the callback with the waitpid result. Since all we're interested in here is the reaping, we set the callback to a function which just ignores the result.

I tried to make this more clear by introducing a special NoOpMonitorCallback function and making this code reference that.

unittests/tools/lldb-server/tests/TestClient.cpp

104

This is just launching the debugserver/lldb-server process, so there should be no special reaping going on here.
What would be interesting (and hence the TODO) is to make the tests more resilient to broken server binaries. Right now, if you break the server so much it does not even connect to the client, this test will hang. If we could check for the exit status, we could fail the test with a nice error message instead. The reason I did not do that now is because we don't currently have an easy way to wait for a process to exit *or* a socket to connect.

[labath]

Update the diff.

include/lldb/Host/Host.h

public:
   typedef std::map<lldb::pid_t, bool> TidMap;
   typedef std::pair<lldb::pid_t, bool> TidPair;
   static bool FindProcessThreads(const lldb::pid_t pid, TidMap &tids_to_attach);
 
   static bool GetProcessInfo(lldb::pid_t pid, ProcessInstanceInfo &proc_info);
 
   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);
 
   //------------------------------------------------------------------
   /// Perform expansion of the command-line for this launch info This can
   /// potentially involve wildcard expansion
   //  environment variable replacement, and whatever other
   //  argument magic the platform defines as part of its typical
   //  user experience

include/lldb/Host/MonitoringProcessLauncher.h

 
 namespace lldb_private {
 
 class MonitoringProcessLauncher : public ProcessLauncher {
 public:
   explicit MonitoringProcessLauncher(
       std::unique_ptr<ProcessLauncher> 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;
 
 private:
   std::unique_ptr<ProcessLauncher> m_delegate_launcher;
 };
 
 } // namespace lldb_private
 
 #endif // lldb_Host_MonitoringProcessLauncher_h_

include/lldb/Target/ProcessLaunchInfo.h

public:
   void
   SetMonitorProcessCallback(const Host::MonitorChildProcessCallback &callback,
                             bool monitor_signals);
 
   Host::MonitorChildProcessCallback GetMonitorProcessCallback() const {
     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
   // process. Return true if the LaunchInfo has taken care of monitoring the
   // process, and false if the caller might want to monitor the process
   // themselves.
 
   bool MonitorProcess() const;

source/Host/common/MonitoringProcessLauncher.cpp

 //===-- MonitoringProcessLauncher.cpp ---------------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #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"
 
 #include "llvm/Support/FileSystem.h"
 
 using namespace lldb;
 using namespace lldb_private;
MonitoringProcessLauncher::LaunchProcess(const ProcessLaunchInfo &launch_info,
   assert(!resolved_info.GetFlags().Test(eLaunchFlagLaunchInTTY));
 
   HostProcess process =
       m_delegate_launcher->LaunchProcess(resolved_info, error);
 
   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 {
     // Invalid process ID, something didn't go well
     if (error.Success())
       error.SetErrorString("process launch failed for unknown reasons");
   }
   return process;
 }

source/Host/common/NativeProcessProtocol.cpp

 //===-- NativeProcessProtocol.cpp -------------------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "lldb/Host/common/NativeProcessProtocol.h"
 #include "lldb/Core/State.h"
 #include "lldb/Host/Host.h"
 #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"
 
 using namespace lldb;
 using namespace lldb_private;
 
 // -----------------------------------------------------------------------------

source/Host/macosx/Host.mm

 #include <sys/stat.h>
 #include <sys/sysctl.h>
 #include <sys/types.h>
 #include <unistd.h>
 
 #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"
 #include "lldb/Utility/DataExtractor.h"
 #include "lldb/Utility/Endian.h"
 #include "lldb/Utility/FileSpec.h"
 #include "lldb/Utility/Log.h"
 #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"
 
 #include "cfcpp/CFCBundle.h"
 #include "cfcpp/CFCMutableArray.h"
 #include "cfcpp/CFCMutableDictionary.h"
 #include "cfcpp/CFCReleaser.h"
error =
         LaunchProcessPosixSpawn(exe_spec.GetPath().c_str(), launch_info, pid);
   }
 
   if (pid != LLDB_INVALID_PROCESS_ID) {
     // If all went well, then set the process ID into the 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())
       error.SetErrorString("process launch failed for unknown reasons");
   }
   return error;
 }
 

source/Plugins/Platform/POSIX/PlatformPOSIX.cpp

 PlatformPOSIX::DebugProcess(ProcessLaunchInfo &launch_info, Debugger &debugger,
                             Target *target, // Can be NULL, if NULL create a new
                                             // target, else use existing one
                             Status &error) {
   ProcessSP process_sp;
 
   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)
       process_sp = m_remote_platform_sp->DebugProcess(launch_info, debugger,
                                                       target, error);
     else
       error.SetErrorString("the platform is not currently connected");
   }

source/Target/ProcessLaunchInfo.cpp

 }
 
 void ProcessLaunchInfo::SetMonitorProcessCallback(
     const Host::MonitorChildProcessCallback &callback, bool monitor_signals) {
   m_monitor_callback = callback;
   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(),
                                       m_monitor_signals);
     return true;
   }
   return false;
 }

unittests/tools/lldb-server/tests/TestClient.cpp

if (!InferiorArgs.empty()) {
     for (StringRef arg : InferiorArgs)
       args.AppendArgument(arg);
   }
 
   ProcessLaunchInfo Info;
   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())
     return status.ToError();
 
   Socket *accept_socket;
   listen_socket.Accept(accept_socket);
   auto Conn = llvm::make_unique<ConnectionFileDescriptor>(accept_socket);