Re-enable logging on the child side of a fork() in lldb-server platform mode

[jasonmolenda]

Summary

When lldb-server is in platform mode and waiting for a connection, when it receives a connection it fork()'s and then runs an lldb-server/debugserver to respond to that connection. The amount of things done between a fork() and an exec() should be very small, but if there is a bug in lldb-server and you're trying to use logging to debug it, the fact that logging does not happen in the child process makes it challenging to understand what is going on.

The fact that the logging re-enablement in the child will only happen when done by hand makes me more comfortable with this.

Pavel, do you think you're the best person to talk to about this? I've tested this on darwin and on ubuntu and both exhibit the same behavior. e.g.

terminal1% ./a.out

terminal2% bin/lldb-server platform --verbose --log-channels 'lldb all:gdb-remote all' --server --listen '*:11110' -m 11120 -M 11250

terminal3% lldb
(lldb) pla sel remote-linux
(lldb) pla connect connect://localhost:11110
(lldb) pla pro att -n a.out

without this patch, the only logging you'll see is lldb-server waiting for a connection, receiving a connection, and that's about it.

[davide]

I took a look and I think this is a sensible change, but we should really test it.

[jasonmolenda]

Jim remembers some problem with logging across a fork()'ed process, Pavel does this ring a bell? This change might be completely bogus -- but it's very difficult to debug the child side of an lldb-server platform today.

[labath]

In D42210#979608, @jasonmolenda wrote:

Jim remembers some problem with logging across a fork()'ed process, Pavel does this ring a bell? This change might be completely bogus -- but it's very difficult to debug the child side of an lldb-server platform today.

I believe Jim is thinking of D38938. The issue is that if another thread holds the logging mutex while we fork(), the mutex will remain in a bogus state in the child process. This would mean that any operation on the Log subsystem (such as enabling logging) could hang. We hold the mutex for just a couple of instructions (just enough to copy a shared_ptr), but after some code reshuffles, we were hitting this case very often in liblldb.

Now, I don't think this can happen here as at the point where we are forking, the platform process is single-threaded. So, enabling logging here should be safe, but only accidentally. It should be possible to make this always safe, but that will need to be done with a very steady hand. My opinion is we should not bother with that (i.e., just check this in as-is) until we actually need more threads in the platform, particularly as I think the long-term direction here should be to replace the fork with a new thread for handling the connection.

As for testing, the problem we have now is that we have no direct platform tests today -- remote tests will test platform by the virtue of all connections going through it but a standard check-lldb will not even run this code. I have been planning to add tests like these, but I haven't got around to that yet, and I'm not sure when will that happen. If you want to take a stab at it, I can give you some pointers.

BTW: for most debugging needs you should be able to just start the platform without the --server argument, which will disable forking and handle each connection in the main process.

tools/lldb-server/lldb-platform.cpp

         ;
       if (fork()) {
         // Parent doesn't need a connection to the lldb client
         delete conn;
 
         // Parent will continue to listen for new connections.
         continue;
       } else {
+        // We lose the logging on the child side of the fork, re-enable it.  
+        if (!LLDBServerUtilities::SetupLogging(log_file, log_channels, 0))
+          return -1;
+
         // Child process will handle the connection and exit.
         g_server = 0;
         // Listening socket is owned by parent process.
         acceptor_up.release();
       }
     } else {
       // If not running as a server, this process will not accept
       // connections while a connection is active.