[Lldb-commits] [lldb] r272682 - [lldb] Fixed race conditions on private state thread exit

[Cameron Desrochers via lldb-commits]

Author: cameron314
Date: Tue Jun 14 11:22:45 2016
New Revision: 272682

URL: http://llvm.org/viewvc/llvm-project?rev=272682&view=rev
Log:
[lldb] Fixed race conditions on private state thread exit

This patch fixes various races between the time the private state thread is signaled to exit and the time it actually exits (during which it no longer responds to events). Previously, this was consistently causing 2-second timeout delays on process detach/stop for us.

This also prevents crashes that were caused by the thread controlling its own owning pointer while the controller was using it (copying the thread wrapper is not enough to mitigate this, since the internal thread object was getting reset anyway). Again, we were seeing this consistently.

Differential Revision: http://reviews.llvm.org/D21296

Modified:
    lldb/trunk/include/lldb/Target/Process.h
    lldb/trunk/source/Target/Process.cpp

Modified: lldb/trunk/include/lldb/Target/Process.h
URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/include/lldb/Target/Process.h?rev=272682&r1=272681&r2=272682&view=diff
==============================================================================
--- lldb/trunk/include/lldb/Target/Process.h (original)
+++ lldb/trunk/include/lldb/Target/Process.h Tue Jun 14 11:22:45 2016
@@ -3309,9 +3309,13 @@ protected:
     bool
     PrivateStateThreadIsValid () const
     {
-        return m_private_state_thread.IsJoinable();
+        lldb::StateType state = m_private_state.GetValue();
+        return state != lldb::eStateInvalid &&
+               state != lldb::eStateDetached &&
+               state != lldb::eStateExited &&
+               m_private_state_thread.IsJoinable();
     }
-    
+
     void
     ForceNextEventDelivery()
     {

Modified: lldb/trunk/source/Target/Process.cpp
URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Target/Process.cpp?rev=272682&r1=272681&r2=272682&view=diff
==============================================================================
--- lldb/trunk/source/Target/Process.cpp (original)
+++ lldb/trunk/source/Target/Process.cpp Tue Jun 14 11:22:45 2016
@@ -4088,7 +4088,7 @@ Process::ResumePrivateStateThread ()
 void
 Process::StopPrivateStateThread ()
 {
-    if (PrivateStateThreadIsValid ())
+    if (m_private_state_thread.IsJoinable ())
         ControlPrivateStateThread (eBroadcastInternalStateControlStop);
     else
     {
@@ -4110,21 +4110,23 @@ Process::ControlPrivateStateThread (uint
     if (log)
         log->Printf ("Process::%s (signal = %d)", __FUNCTION__, signal);
 
-    // Signal the private state thread. First we should copy this is case the
-    // thread starts exiting since the private state thread will NULL this out
-    // when it exits
+    // Signal the private state thread
+    if (m_private_state_thread.IsJoinable())
     {
-        HostThread private_state_thread(m_private_state_thread);
-        if (private_state_thread.IsJoinable())
+        // Broadcast the event.
+        // It is important to do this outside of the if below, because
+        // it's possible that the thread state is invalid but that the
+        // thread is waiting on a control event instead of simply being
+        // on its way out (this should not happen, but it apparently can).
+        if (log)
+            log->Printf ("Sending control event of type: %d.", signal);
+        std::shared_ptr<EventDataReceipt> event_receipt_sp(new EventDataReceipt());
+        m_private_state_control_broadcaster.BroadcastEvent(signal, event_receipt_sp);
+
+        // Wait for the event receipt or for the private state thread to exit
+        bool receipt_received = false;
+        if (PrivateStateThreadIsValid())
         {
-            if (log)
-                log->Printf ("Sending control event of type: %d.", signal);
-            // Send the control event and wait for the receipt or for the private state
-            // thread to exit
-            std::shared_ptr<EventDataReceipt> event_receipt_sp(new EventDataReceipt());
-            m_private_state_control_broadcaster.BroadcastEvent(signal, event_receipt_sp);
-
-            bool receipt_received = false;
             while (!receipt_received)
             {
                 bool timed_out = false;
@@ -4137,23 +4139,24 @@ Process::ControlPrivateStateThread (uint
                 if (!receipt_received)
                 {
                     // Check if the private state thread is still around. If it isn't then we are done waiting
-                    if (!m_private_state_thread.IsJoinable())
-                        break; // Private state thread exited, we are done
+                    if (!PrivateStateThreadIsValid())
+                        break; // Private state thread exited or is exiting, we are done
                 }
             }
-
-            if (signal == eBroadcastInternalStateControlStop)
-            {
-                thread_result_t result = NULL;
-                private_state_thread.Join(&result);
-            }
         }
-        else
+
+        if (signal == eBroadcastInternalStateControlStop)
         {
-            if (log)
-                log->Printf ("Private state thread already dead, no need to signal it to stop.");
+            thread_result_t result = NULL;
+            m_private_state_thread.Join(&result);
+            m_private_state_thread.Reset();
         }
     }
+    else
+    {
+        if (log)
+            log->Printf("Private state thread already dead, no need to signal it to stop.");
+    }
 }
 
 void
@@ -4446,7 +4449,6 @@ Process::RunPrivateStateThread (bool is_
     // try to change it on the way out.
     if (!is_secondary_thread)
         m_public_run_lock.SetStopped();
-    m_private_state_thread.Reset();
     return NULL;
 }