[Lldb-commits] [lldb] 89a3691 - [lldb] Fix ThreadedCommunication races

[Pavel Labath via lldb-commits]

Author: Pavel Labath
Date: 2022-09-09T15:10:38+02:00
New Revision: 89a3691b794cee20187e14a750ecde8b6d3f7e71

URL: https://github.com/llvm/llvm-project/commit/89a3691b794cee20187e14a750ecde8b6d3f7e71
DIFF: https://github.com/llvm/llvm-project/commit/89a3691b794cee20187e14a750ecde8b6d3f7e71.diff

LOG: [lldb] Fix ThreadedCommunication races

The Read function could end up blocking if data (or EOF) arrived just as
it was about to start waiting for the events. This was only discovered
now, because we did not have unit tests for this functionality before.
We need to check for data *after* we start listening for incoming
events. There were no changes to the read thread code needed, as we
already use this pattern in SynchronizeWithReadThread, so I just updated
the comments to make it clear that it is used for reading as well.

Differential Revision: https://reviews.llvm.org/D133410

Added: 
    

Modified: 
    lldb/source/Core/ThreadedCommunication.cpp

Removed: 
    


################################################################################
diff  --git a/lldb/source/Core/ThreadedCommunication.cpp b/lldb/source/Core/ThreadedCommunication.cpp
index ab89b474769a5..bc4c6e0214ef2 100644
--- a/lldb/source/Core/ThreadedCommunication.cpp
+++ b/lldb/source/Core/ThreadedCommunication.cpp
@@ -104,34 +104,50 @@ size_t ThreadedCommunication::Read(void *dst, size_t dst_len,
       return 0;
     }
 
+    // No data yet, we have to start listening.
     ListenerSP listener_sp(
         Listener::MakeListener("ThreadedCommunication::Read"));
     listener_sp->StartListeningForEvents(
         this, eBroadcastBitReadThreadGotBytes | eBroadcastBitReadThreadDidExit);
-    EventSP event_sp;
-    while (listener_sp->GetEvent(event_sp, timeout)) {
-      const uint32_t event_type = event_sp->GetType();
-      if (event_type & eBroadcastBitReadThreadGotBytes) {
-        return GetCachedBytes(dst, dst_len);
-      }
 
-      if (event_type & eBroadcastBitReadThreadDidExit) {
-        // If the thread exited of its own accord, it either means it
-        // hit an end-of-file condition or an error.
-        status = m_pass_status;
-        if (error_ptr)
-          *error_ptr = std::move(m_pass_error);
+    // Re-check for data, as it might have arrived while we were setting up our
+    // listener.
+    cached_bytes = GetCachedBytes(dst, dst_len);
+    if (cached_bytes > 0) {
+      status = eConnectionStatusSuccess;
+      return cached_bytes;
+    }
 
-        if (GetCloseOnEOF())
-          Disconnect(nullptr);
+    EventSP event_sp;
+    // Explicitly check for the thread exit, for the same reason.
+    if (m_read_thread_did_exit) {
+      // We've missed the event, lets just conjure one up.
+      event_sp = std::make_shared<Event>(eBroadcastBitReadThreadDidExit);
+    } else {
+      if (!listener_sp->GetEvent(event_sp, timeout)) {
+        if (error_ptr)
+          error_ptr->SetErrorString("Timed out.");
+        status = eConnectionStatusTimedOut;
         return 0;
       }
     }
+    const uint32_t event_type = event_sp->GetType();
+    if (event_type & eBroadcastBitReadThreadGotBytes) {
+      return GetCachedBytes(dst, dst_len);
+    }
 
-    if (error_ptr)
-      error_ptr->SetErrorString("Timed out.");
-    status = eConnectionStatusTimedOut;
-    return 0;
+    if (event_type & eBroadcastBitReadThreadDidExit) {
+      // If the thread exited of its own accord, it either means it
+      // hit an end-of-file condition or an error.
+      status = m_pass_status;
+      if (error_ptr)
+        *error_ptr = std::move(m_pass_error);
+
+      if (GetCloseOnEOF())
+        Disconnect(nullptr);
+      return 0;
+    }
+    llvm_unreachable("Got unexpected event type!");
   }
 
   // We aren't using a read thread, just read the data synchronously in this
@@ -299,22 +315,25 @@ lldb::thread_result_t ThreadedCommunication::ReadThread() {
   m_pass_error = std::move(error);
   LLDB_LOG(log, "Communication({0}) thread exiting...", this);
 
-  // Handle threads wishing to synchronize with us.
-  {
-    // Prevent new ones from showing up.
-    m_read_thread_did_exit = true;
+  // Start shutting down. We need to do this in a very specific order to ensure
+  // we don't race with threads wanting to read/synchronize with us.
 
-    // Unblock any existing thread waiting for the synchronization signal.
-    BroadcastEvent(eBroadcastBitNoMorePendingInput);
+  // First, we signal our intent to exit. This ensures no new thread start
+  // waiting on events from us.
+  m_read_thread_did_exit = true;
 
-    // Wait for the thread to finish...
+  // Unblock any existing thread waiting for the synchronization signal.
+  BroadcastEvent(eBroadcastBitNoMorePendingInput);
+
+  {
+    // Wait for the synchronization thread to finish...
     std::lock_guard<std::mutex> guard(m_synchronize_mutex);
     // ... and disconnect.
     if (disconnect)
       Disconnect();
   }
 
-  // Let clients know that this thread is exiting
+  // Finally, unblock any readers waiting for us to exit.
   BroadcastEvent(eBroadcastBitReadThreadDidExit);
   return {};
 }