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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
