[Lldb-commits] [lldb] [lldb] Updated lldb-server to spawn the child process and share socket on Windows (PR #101283)

[Pavel Labath via lldb-commits]

labath wrote:

> @labath
> 
> > This is where I'll have to disagree with you. The exec following the fork is not "redundant" on linux. A naked fork is an extremely dangerous thing in todays (multithreaded) applications. I'm absolutely certain that the improper use of fork with threads (and the FD leaks that go along with it) is the cause of at least one of the issues (ETXTBSY) you refer to as "system bugs", and fairly sure it's also the cause of the other one (the pipe workaround) as well. In a multithreaded application (which lldb-platform isn't right now, but more of an accident than design, and which you've tried to make extremely multithreaded) the only safe use of fork is if it is immediately followed by an exec, and this has to be done in an extremely careful way (we try to do that, and we still failed).
> 
> I meant https://bugzilla.kernel.org/show_bug.cgi?id=546 and [this discussion](https://github.com/llvm/llvm-project/pull/100670/files#r1693201568). I'm sad that you don't believe.

I know what you're referring to. However, that's a statement that comes with a very heavy burden of proof, one that you simply have not provided. The number of users of linux kernel exceeds the users of lldb by many orders of magnitude, so it's very unlikely that the problem is on their side. It's not that it's impossible -- I personally have tracked down lldb problems down to bugs in our dependencies -- from the libstdc++ to the kernel. However, this just doesn't look like one of those cases.

> I concluded that no one uses the real multithreading on Linux.

That's another very bold statement. I am not sure what you meant by "real multithreading", but I know for a fact that some Linux applications run thousands of threads in one process without any issues. All of them are very careful about how they use fork though.

> Someone tried and faced unsolvable problems. Therefore, #100670 is doomed.
> 
> > But even if it weren't for all of this, I'd still think it makes sense to start a new process, just for symmetry with windows. That's the nature of portable programming.
> 
> Not all code may be portable. It is better to use threads on Windows, but not on Posix. fork() is missing on Windows and starting a new process is a workaround. When we have the accepted socket and everything is ready to just handle the connection on Posix (after fork), you suggest to drop all initialized parameters, start a new process, pass all necessary parameters via the command line, initialize all parameters again, go to the same point and finally handle the connection. What is the reason? To be similar how it will work on Windows.

That is exactly what I'm suggesting.

> No, I don't agree.

Well, at least we can agree to disagree. ;)

> You are worried about some FD. But the main process `lldb-server platform` does not launch anything. There are no any FDs which may leak.

It forks. Forking duplicates any FD opened into the parent into the child. Even those marked with CLOEXEC, which is the usual way to guard against FD leakage. If you're single-threaded than this is manageable, but as soon as you have multiple threads (believe it or not, many applications on linux use multiple threads, and a lot of code creates background threads; lldb-server platform code executed here is pretty small, but it still calls into a lot of code that could create those), fork() becomes a ticking time bomb. In a multithreaded application, the only code that can be safely run after a fork, is that which can run in a signal handler. You can't write a server like that.

Using fork to serve multiple connections is a pattern from the seventies. Modern applications don't do it this way. As far as I'm concerned, this code should not have been written this way (and if the author cared about windows, they would have never done it), and I'd support removing this pattern even if it weren't for windows.

> We can't unify socket sharing nicely for Windows and Posix. We can try to combine the common code in the part 2 of this patch when the socket sharing will be added to GDBRemoteCommunication::StartDebugserverProcess() too.

That's nice, but I think we should figure out how to reduce the number of ifdefs in this patch. Porting linux away from fork may not be your concern, but figuring out how to make the code less branchy is. If you can do that without removing forks, I can take it upon myself to handle the rest. I just think it would be easier to do it both together...

https://github.com/llvm/llvm-project/pull/101283