[Lldb-commits] [PATCH] D21296: [lldb] Fixed race condition on private state thread exit, take 2

[Greg Clayton via lldb-commits]

clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.

This looks like it would work for normal operation. I am not sure it will work when an extra private state thread is spun up. In certain circumstances we need to create more private state threads. This happens when you have an expression that is run from the private state thread. Since the private state thread is what controls the actual process, we can't run an expression from the current private state thread, so we spin up new ones. The current code is doing some tricky things to deal with this, and that was part of the reason Process::ControlPrivateStateThread() was making a copy of the current value of "m_private_state_thread" into a local variable named "private_state_thread":

  HostThread private_state_thread(m_private_state_thread);

See the code in "Process::RunThreadPlan()" around the code:

  if (m_private_state_thread.EqualsThread(Host::GetCurrentThread()))

The new loop as written in Process::ControlPrivateStateThread() could end up using m_private_state_thread with differing contents in the "if (m_private_state_thread.IsJoinable())" if statement. Jim Ingham made these changes, so we should add him to the reviewer list. I am going to mark as "Request Changes" so we can address any such issues, but Jim should chime in on this before we proceed.

The way this normally happens is an expression is being run and while handling the expression on the private state thread, we need to run another expression (like a call to "mmap" in the debugged process so we can allocate memory), so we need to spin up another private state thread to handle the needed starts and stops. Only one of these threads will be actively grabbing events at a single time, so your patch might just work, but I want to get some more eyes on this to be sure.

Please add Jim Ingham as a reviewer.


http://reviews.llvm.org/D21296