[Lldb-commits] [lldb] [lldb] Fix race condition in Process::WaitForProcessToStop() (PR #144919)

[via lldb-commits]

athierry-oct wrote:

Thanks for the feedback and the good discussion!

> I think I understand most of what you're saying, and I think the proposed solution may solve the problem you have in mind -- but I don't think it can solve the problem I have in mind (and I'm not sure which of those is OP's problem :P). The race I think I see is in Process::StopForDestroyOrDetach.

The race I have in mind is the one described by @labath, ie. this one:

> We first check the process state (both of them), and if it's running, we install the hijack listener (and wait on it). However if the stopped event is sent (enqueued to the primary listener) before the hijack listener is installed, the listener will never get anything. I don't see how fiddling with thread identities can help there. Even if it does, it will only help for one particular thread. OP is calling Kill from the event handling thread, but I don't think we require that. If Kill is called from some other thread, I think it could still run into the same bug (although the race window will most likely be smaller, since the event thread will be alive and pulling events from the queue).

I'm not exactly sure if it's also related to what you're describing @jimingham. From my (limited) understanding, it looks like a separate issue: in my case I'm calling `Kill()` from the same thread that's handling the stop, so nobody is actually handling the stop (because the thread handling the stop is blocked inside `Kill()`).

> The race Pavel is pointing out also seems real to me, but that should be fixed at the source, rather than after the fact. Process::HijackProcessEvents should check the previous "primary event listener" to see if it has any events and if so they should be transferred to the Hijack Listener. We should also do the same thing in RestoreProcessEvents. That way we'll never drop an event when shuffling primary listeners.

I can try to submit another version of the patch doing that

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