[Lldb-commits] [PATCH] D46395: Remove Process references from the Host module

[Pavel Labath via Phabricator via lldb-commits]

labath marked an inline comment as done.
labath added a comment.

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.



================
Comment at: source/Host/macosx/Host.mm:1501
+    bool monitoring = launch_info.MonitorProcess();
+    (void)monitoring;
+    assert(monitoring);
----------------
clayborg wrote:
> Do we not have a macro for silencing unused variables?
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.


================
Comment at: source/Plugins/Platform/POSIX/PlatformPOSIX.cpp:873-874
+    const bool monitor_signals = false;
+    launch_info.SetMonitorProcessCallback(
+        [](lldb::pid_t, bool, int, int) { return true; }, monitor_signals);
     process_sp = Platform::DebugProcess(launch_info, debugger, target, error);
----------------
clayborg wrote:
> 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
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.


================
Comment at: unittests/tools/lldb-server/tests/TestClient.cpp:119
+  Info.SetMonitorProcessCallback(
+      [](lldb::pid_t, bool, int, int) { return true; }, false);
 
----------------
clayborg wrote:
> 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
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.


https://reviews.llvm.org/D46395