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

[via lldb-commits]

jimingham wrote:

I don't think the locking should be a real issue.  You should only hold the m_events mutex for a listener when adding or removing an event from the Listener's event queue.  But it doesn't make sense to actually do work when handing off the event.  

For instance, the primary place where events get pulled is Listener::FindNextEventInternal.  That function does trigger arbitrary work to be done after the event is fetched - in the DoOnRemoval call.  Since that's just a callout into a plugin provided callback we can't guarantee it won't do something that ends up requiring the listeners mutex.

But we are careful to do this like:

    if (remove) {
      m_events.erase(pos);
      // Unlock the event queue here.  We've removed this event and are about
      // to return it so it should be okay to get the next event off the queue
      // here - and it might be useful to do that in the "DoOnRemoval".
      lock.unlock();
      event_sp->DoOnRemoval();
    }

where that `lock` was the lock_guard for the m_events_mutex, so that the actual work is done on fetching the event is done after we drop the m_events_mutex.

I can't think of a good way to enforce this, but I'm also not sure that's necessary.  However, it might be a good idea to leave a comment by the definition of m_events_mutex saying that this should only be held when adding or fetching events from the queue, and not over any function that does arbitrary work on the event's behalf.

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