[Lldb-commits] [lldb] 5d16957 - [lldb] Properly protect the Communication class with reader/writer lock

Augusto Noronha via lldb-commits lldb-commits at lists.llvm.org
Tue Aug 15 15:46:58 PDT 2023


Author: Augusto Noronha
Date: 2023-08-15T15:43:36-07:00
New Revision: 5d16957207ce1bd1a2091f3677e176012009c59a

URL: https://github.com/llvm/llvm-project/commit/5d16957207ce1bd1a2091f3677e176012009c59a
DIFF: https://github.com/llvm/llvm-project/commit/5d16957207ce1bd1a2091f3677e176012009c59a.diff

LOG: [lldb] Properly protect the Communication class with reader/writer lock

This patch picks up where https://reviews.llvm.org/D157159 left of, but
allows for concurrent reads/writes, but protects setting up and tearing
down the underlying Connection object.

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

Added: 
    

Modified: 
    lldb/include/lldb/Core/Communication.h
    lldb/include/lldb/Core/ThreadedCommunication.h
    lldb/source/Core/Communication.cpp
    lldb/source/Core/ThreadedCommunication.cpp
    lldb/source/Plugins/Process/MacOSX-Kernel/ProcessKDP.cpp

Removed: 
    


################################################################################
diff  --git a/lldb/include/lldb/Core/Communication.h b/lldb/include/lldb/Core/Communication.h
index f5f636816cb4f9..7e4ce362acf9a6 100644
--- a/lldb/include/lldb/Core/Communication.h
+++ b/lldb/include/lldb/Core/Communication.h
@@ -16,6 +16,7 @@
 #include "lldb/lldb-types.h"
 
 #include <mutex>
+#include <shared_mutex>
 #include <string>
 
 namespace lldb_private {
@@ -46,8 +47,6 @@ class Communication {
   /// The destructor is virtual since this class gets subclassed.
   virtual ~Communication();
 
-  virtual void Clear();
-
   /// Connect using the current connection by passing \a url to its connect
   /// function. string.
   ///
@@ -84,7 +83,10 @@ class Communication {
 
   bool HasConnection() const;
 
-  lldb_private::Connection *GetConnection() { return m_connection_sp.get(); }
+  lldb_private::Connection *GetConnection() {
+    std::shared_lock guard(m_connection_mutex);
+    return m_connection_sp.get();
+  }
 
   /// Read bytes from the current connection.
   ///
@@ -169,13 +171,24 @@ class Communication {
                                       ///by this communications class.
   std::mutex
       m_write_mutex; ///< Don't let multiple threads write at the same time...
+  mutable std::shared_mutex m_connection_mutex;
   bool m_close_on_eof;
 
+  /// Same as read but with m_connection_mutex unlocked.
+  size_t ReadUnlocked(void *dst, size_t dst_len,
+                      const Timeout<std::micro> &timeout,
+                      lldb::ConnectionStatus &status, Status *error_ptr);
+
   size_t ReadFromConnection(void *dst, size_t dst_len,
                             const Timeout<std::micro> &timeout,
                             lldb::ConnectionStatus &status, Status *error_ptr);
 
 private:
+  /// Same as Disconnect but with with m_connection_mutex unlocked.
+  lldb::ConnectionStatus DisconnectUnlocked(Status *error_ptr = nullptr);
+  /// Same as Write but with both m_write_mutex and m_connection_mutex unlocked.
+  size_t WriteUnlocked(const void *src, size_t src_len,
+                       lldb::ConnectionStatus &status, Status *error_ptr);
   Communication(const Communication &) = delete;
   const Communication &operator=(const Communication &) = delete;
 };

diff  --git a/lldb/include/lldb/Core/ThreadedCommunication.h b/lldb/include/lldb/Core/ThreadedCommunication.h
index 7ebb77beb77f3d..170fd2dfcb555d 100644
--- a/lldb/include/lldb/Core/ThreadedCommunication.h
+++ b/lldb/include/lldb/Core/ThreadedCommunication.h
@@ -97,8 +97,6 @@ class ThreadedCommunication : public Communication, public Broadcaster {
   /// The destructor is virtual since this class gets subclassed.
   ~ThreadedCommunication() override;
 
-  void Clear() override;
-
   /// Disconnect the communications connection if one is currently connected.
   ///
   /// \return

diff  --git a/lldb/source/Core/Communication.cpp b/lldb/source/Core/Communication.cpp
index 5d890632ccc6a9..e2ba461a02329a 100644
--- a/lldb/source/Core/Communication.cpp
+++ b/lldb/source/Core/Communication.cpp
@@ -27,110 +27,110 @@ using namespace lldb;
 using namespace lldb_private;
 
 Communication::Communication()
-    : m_connection_sp(), m_write_mutex(), m_close_on_eof(true) {
-}
-
-Communication::~Communication() {
-  Clear();
-}
+    : m_connection_sp(), m_connection_mutex(), m_close_on_eof(true) {}
 
-void Communication::Clear() {
-  Disconnect(nullptr);
-}
+Communication::~Communication() { Disconnect(nullptr); }
 
 ConnectionStatus Communication::Connect(const char *url, Status *error_ptr) {
-  Clear();
+  std::unique_lock guard(m_connection_mutex);
 
   LLDB_LOG(GetLog(LLDBLog::Communication),
            "{0} Communication::Connect (url = {1})", this, url);
 
-  lldb::ConnectionSP connection_sp(m_connection_sp);
-  if (connection_sp)
-    return connection_sp->Connect(url, error_ptr);
+  DisconnectUnlocked();
+
+  if (m_connection_sp)
+    return m_connection_sp->Connect(url, error_ptr);
   if (error_ptr)
     error_ptr->SetErrorString("Invalid connection.");
   return eConnectionStatusNoConnection;
 }
 
 ConnectionStatus Communication::Disconnect(Status *error_ptr) {
+  std::unique_lock guard(m_connection_mutex);
+  return DisconnectUnlocked(error_ptr);
+}
+
+ConnectionStatus Communication::DisconnectUnlocked(Status *error_ptr) {
   LLDB_LOG(GetLog(LLDBLog::Communication), "{0} Communication::Disconnect ()",
            this);
 
-  lldb::ConnectionSP connection_sp(m_connection_sp);
-  if (connection_sp) {
-    ConnectionStatus status = connection_sp->Disconnect(error_ptr);
-    // We currently don't protect connection_sp with any mutex for multi-
-    // threaded environments. So lets not nuke our connection class without
-    // putting some multi-threaded protections in. We also probably don't want
-    // to pay for the overhead it might cause if every time we access the
-    // connection we have to take a lock.
-    //
-    // This unique pointer will cleanup after itself when this object goes
-    // away, so there is no need to currently have it destroy itself
-    // immediately upon disconnect.
-    // connection_sp.reset();
+  if (m_connection_sp) {
+    ConnectionStatus status = m_connection_sp->Disconnect(error_ptr);
     return status;
   }
   return eConnectionStatusNoConnection;
 }
 
 bool Communication::IsConnected() const {
-  lldb::ConnectionSP connection_sp(m_connection_sp);
-  return (connection_sp ? connection_sp->IsConnected() : false);
+  std::shared_lock guard(m_connection_mutex);
+  return (m_connection_sp ? m_connection_sp->IsConnected() : false);
 }
 
 bool Communication::HasConnection() const {
+  std::shared_lock guard(m_connection_mutex);
   return m_connection_sp.get() != nullptr;
 }
 
 size_t Communication::Read(void *dst, size_t dst_len,
                            const Timeout<std::micro> &timeout,
                            ConnectionStatus &status, Status *error_ptr) {
-  Log *log = GetLog(LLDBLog::Communication);
-  LLDB_LOG(
-      log,
-      "this = {0}, dst = {1}, dst_len = {2}, timeout = {3}, connection = {4}",
-      this, dst, dst_len, timeout, m_connection_sp.get());
-
-  return ReadFromConnection(dst, dst_len, timeout, status, error_ptr);
+  std::shared_lock guard(m_connection_mutex);
+  return ReadUnlocked(dst, dst_len, timeout, status, error_ptr);
 }
 
 size_t Communication::Write(const void *src, size_t src_len,
                             ConnectionStatus &status, Status *error_ptr) {
-  lldb::ConnectionSP connection_sp(m_connection_sp);
+  // We need to lock the write mutex so no concurrent writes happen, but also
+  // lock the connection mutex so it's not reset mid write. We need both mutexes
+  // because reads and writes from the connection can happen concurrently.
+  std::shared_lock guard(m_connection_mutex);
+  std::lock_guard<std::mutex> guard_write(m_write_mutex);
+  return WriteUnlocked(src, src_len, status, error_ptr);
+}
+
+size_t Communication::WriteUnlocked(const void *src, size_t src_len,
+                                    ConnectionStatus &status,
+                                    Status *error_ptr) {
+  if (!m_connection_sp) {
+    if (error_ptr)
+      error_ptr->SetErrorString("Invalid connection.");
+    status = eConnectionStatusNoConnection;
+    return 0;
+  }
 
-  std::lock_guard<std::mutex> guard(m_write_mutex);
   LLDB_LOG(GetLog(LLDBLog::Communication),
            "{0} Communication::Write (src = {1}, src_len = {2}"
            ") connection = {3}",
-           this, src, (uint64_t)src_len, connection_sp.get());
-
-  if (connection_sp)
-    return connection_sp->Write(src, src_len, status, error_ptr);
+           this, src, (uint64_t)src_len, m_connection_sp.get());
 
-  if (error_ptr)
-    error_ptr->SetErrorString("Invalid connection.");
-  status = eConnectionStatusNoConnection;
-  return 0;
+  return m_connection_sp->Write(src, src_len, status, error_ptr);
 }
 
 size_t Communication::WriteAll(const void *src, size_t src_len,
                                ConnectionStatus &status, Status *error_ptr) {
+  std::shared_lock guard(m_connection_mutex);
+  std::lock_guard<std::mutex> guard_write(m_write_mutex);
   size_t total_written = 0;
   do
-    total_written += Write(static_cast<const char *>(src) + total_written,
-                           src_len - total_written, status, error_ptr);
+    total_written +=
+        WriteUnlocked(static_cast<const char *>(src) + total_written,
+                      src_len - total_written, status, error_ptr);
   while (status == eConnectionStatusSuccess && total_written < src_len);
   return total_written;
 }
 
-size_t Communication::ReadFromConnection(void *dst, size_t dst_len,
-                                         const Timeout<std::micro> &timeout,
-                                         ConnectionStatus &status,
-                                         Status *error_ptr) {
-  lldb::ConnectionSP connection_sp(m_connection_sp);
-  if (connection_sp)
-    return connection_sp->Read(dst, dst_len, timeout, status, error_ptr);
+size_t Communication::ReadUnlocked(void *dst, size_t dst_len,
+                                   const Timeout<std::micro> &timeout,
+                                   ConnectionStatus &status,
+                                   Status *error_ptr) {
+  Log *log = GetLog(LLDBLog::Communication);
+  LLDB_LOG(
+      log,
+      "this = {0}, dst = {1}, dst_len = {2}, timeout = {3}, connection = {4}",
+      this, dst, dst_len, timeout, m_connection_sp.get());
+  if (m_connection_sp)
+    return m_connection_sp->Read(dst, dst_len, timeout, status, error_ptr);
 
   if (error_ptr)
     error_ptr->SetErrorString("Invalid connection.");
@@ -139,7 +139,8 @@ size_t Communication::ReadFromConnection(void *dst, size_t dst_len,
 }
 
 void Communication::SetConnection(std::unique_ptr<Connection> connection) {
-  Disconnect(nullptr);
+  std::unique_lock guard(m_connection_mutex);
+  DisconnectUnlocked(nullptr);
   m_connection_sp = std::move(connection);
 }
 

diff  --git a/lldb/source/Core/ThreadedCommunication.cpp b/lldb/source/Core/ThreadedCommunication.cpp
index 7d8aae5d8ff689..8d3515652d8b85 100644
--- a/lldb/source/Core/ThreadedCommunication.cpp
+++ b/lldb/source/Core/ThreadedCommunication.cpp
@@ -61,11 +61,6 @@ ThreadedCommunication::~ThreadedCommunication() {
            this, GetBroadcasterName());
 }
 
-void ThreadedCommunication::Clear() {
-  SetReadThreadBytesReceivedCallback(nullptr, nullptr);
-  StopReadThread(nullptr);
-  Communication::Clear();
-}
 
 ConnectionStatus ThreadedCommunication::Disconnect(Status *error_ptr) {
   assert((!m_read_thread_enabled || m_read_thread_did_exit) &&
@@ -77,6 +72,7 @@ size_t ThreadedCommunication::Read(void *dst, size_t dst_len,
                                    const Timeout<std::micro> &timeout,
                                    ConnectionStatus &status,
                                    Status *error_ptr) {
+  std::shared_lock guard(m_connection_mutex);
   Log *log = GetLog(LLDBLog::Communication);
   LLDB_LOG(
       log,
@@ -152,7 +148,7 @@ size_t ThreadedCommunication::Read(void *dst, size_t dst_len,
 
   // We aren't using a read thread, just read the data synchronously in this
   // thread.
-  return Communication::Read(dst, dst_len, timeout, status, error_ptr);
+  return Communication::ReadUnlocked(dst, dst_len, timeout, status, error_ptr);
 }
 
 bool ThreadedCommunication::StartReadThread(Status *error_ptr) {
@@ -273,46 +269,50 @@ lldb::thread_result_t ThreadedCommunication::ReadThread() {
   ConnectionStatus status = eConnectionStatusSuccess;
   bool done = false;
   bool disconnect = false;
-  while (!done && m_read_thread_enabled) {
-    size_t bytes_read = ReadFromConnection(
-        buf, sizeof(buf), std::chrono::seconds(5), status, &error);
-    if (bytes_read > 0 || status == eConnectionStatusEndOfFile)
-      AppendBytesToCache(buf, bytes_read, true, status);
-
-    switch (status) {
-    case eConnectionStatusSuccess:
-      break;
-
-    case eConnectionStatusEndOfFile:
-      done = true;
-      disconnect = GetCloseOnEOF();
-      break;
-    case eConnectionStatusError: // Check GetError() for details
-      if (error.GetType() == eErrorTypePOSIX && error.GetError() == EIO) {
-        // EIO on a pipe is usually caused by remote shutdown
+
+  {
+    std::shared_lock guard(m_connection_mutex);
+    while (!done && m_read_thread_enabled) {
+      size_t bytes_read = ReadUnlocked(buf, sizeof(buf),
+                                       std::chrono::seconds(5), status, &error);
+      if (bytes_read > 0 || status == eConnectionStatusEndOfFile)
+        AppendBytesToCache(buf, bytes_read, true, status);
+
+      switch (status) {
+      case eConnectionStatusSuccess:
+        break;
+
+      case eConnectionStatusEndOfFile:
+        done = true;
         disconnect = GetCloseOnEOF();
+        break;
+      case eConnectionStatusError: // Check GetError() for details
+        if (error.GetType() == eErrorTypePOSIX && error.GetError() == EIO) {
+          // EIO on a pipe is usually caused by remote shutdown
+          disconnect = GetCloseOnEOF();
+          done = true;
+        }
+        if (error.Fail())
+          LLDB_LOG(log, "error: {0}, status = {1}", error,
+                   ThreadedCommunication::ConnectionStatusAsString(status));
+        break;
+      case eConnectionStatusInterrupted: // Synchronization signal from
+                                         // SynchronizeWithReadThread()
+        // The connection returns eConnectionStatusInterrupted only when there
+        // is no input pending to be read, so we can signal that.
+        BroadcastEvent(eBroadcastBitNoMorePendingInput);
+        break;
+      case eConnectionStatusNoConnection:   // No connection
+      case eConnectionStatusLostConnection: // Lost connection while connected
+                                            // to a valid connection
         done = true;
+        [[fallthrough]];
+      case eConnectionStatusTimedOut: // Request timed out
+        if (error.Fail())
+          LLDB_LOG(log, "error: {0}, status = {1}", error,
+                   ThreadedCommunication::ConnectionStatusAsString(status));
+        break;
       }
-      if (error.Fail())
-        LLDB_LOG(log, "error: {0}, status = {1}", error,
-                 ThreadedCommunication::ConnectionStatusAsString(status));
-      break;
-    case eConnectionStatusInterrupted: // Synchronization signal from
-                                       // SynchronizeWithReadThread()
-      // The connection returns eConnectionStatusInterrupted only when there is
-      // no input pending to be read, so we can signal that.
-      BroadcastEvent(eBroadcastBitNoMorePendingInput);
-      break;
-    case eConnectionStatusNoConnection:   // No connection
-    case eConnectionStatusLostConnection: // Lost connection while connected to
-                                          // a valid connection
-      done = true;
-      [[fallthrough]];
-    case eConnectionStatusTimedOut: // Request timed out
-      if (error.Fail())
-        LLDB_LOG(log, "error: {0}, status = {1}", error,
-                 ThreadedCommunication::ConnectionStatusAsString(status));
-      break;
     }
   }
   m_pass_status = status;
@@ -361,8 +361,12 @@ void ThreadedCommunication::SynchronizeWithReadThread() {
   if (!m_read_thread_enabled || m_read_thread_did_exit)
     return;
 
-  // Notify the read thread.
-  m_connection_sp->InterruptRead();
+  {
+    // Notify the read thread.
+    std::shared_lock guard(m_connection_mutex);
+    if (m_connection_sp)
+    m_connection_sp->InterruptRead();
+  }
 
   // Wait for the synchronization event.
   EventSP event_sp;

diff  --git a/lldb/source/Plugins/Process/MacOSX-Kernel/ProcessKDP.cpp b/lldb/source/Plugins/Process/MacOSX-Kernel/ProcessKDP.cpp
index b17701ea4a15cf..6862e43b6d5d35 100644
--- a/lldb/source/Plugins/Process/MacOSX-Kernel/ProcessKDP.cpp
+++ b/lldb/source/Plugins/Process/MacOSX-Kernel/ProcessKDP.cpp
@@ -557,7 +557,6 @@ Status ProcessKDP::DoDetach(bool keep_stopped) {
     }
   }
   StopAsyncThread();
-  m_comm.Clear();
 
   SetPrivateState(eStateDetached);
   ResumePrivateStateThread();


        


More information about the lldb-commits mailing list