Page MenuHomePhabricator

Remove Process references from the Host module
ClosedPublic

Authored by labath on May 3 2018, 10:02 AM.

Details

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).

Diff Detail

Repository
rL LLVM

Event Timeline

labath created this revision.May 3 2018, 10:02 AM
jingham accepted this revision.May 3 2018, 10:23 AM

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.

This revision is now accepted and ready to land.May 3 2018, 10:23 AM
clayborg added inline comments.
source/Host/macosx/Host.mm
1501 ↗(On Diff #145037)

Do we not have a macro for silencing unused variables?

source/Plugins/Platform/POSIX/PlatformPOSIX.cpp
873–874 ↗(On Diff #145037)

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
119 ↗(On Diff #145037)

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 marked an inline comment as done.May 4 2018, 2:04 AM

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 ↗(On Diff #145037)

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 ↗(On Diff #145037)

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
119 ↗(On Diff #145037)

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 updated this revision to Diff 145163.May 4 2018, 2:06 AM
labath marked an inline comment as done.

Update the diff.

This revision was automatically updated to reflect the committed changes.