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

[JDevlieghere]

Summary

The Communication class was trying to (ab)use unique_ptr's atomic properties to protected it from concurrent access. Replace it with a regular reader/writer lock.

[augusto2112]

On the description you mention unique_ptr but I only see shared_ptr here. Was that a typo or do you mean that users of this class kept it behind a unique_ptr?

In any case I think this change is great.

lldb/source/Core/Communication.cpp

35

Do you think it's possible that between the call to Clear and this lock someone acquires the lock?

[bulbazord]

lldb/source/Core/Communication.cpp

34–35

I think this one may be slightly incorrect. Here's how I imagine it going wrong:

• Thread 1 calls Connect. It only runs Clear() (which disconnects) and then Thread 2 is scheduled.
• Thread 2 then calls Connect. It also only runs Clear() and then Thread 1 is scheduled.
• Thread 1 grabs the lock, connects, and exits.
• Thread 2 also grabs the lock, connects, and exits.

I'm not sure how gracefully Connection::Connect handles another connection if not properly disconnected from the first url beforehand. Reading through some of the Connection derived class implementations of Connect I'm not sure exactly what will happen though. The documentation isn't much clearer either...

[JDevlieghere]

lldb/source/Core/Communication.cpp

35

Yes, you're both correct. We can't use the ClearUnlocked trick here because it's Disconnect that locks and Clear is virtual. I'll need to play around with this. Maybe we don't need a Clear and we can call DisconnectUnlocked directly ?

[bulbazord]

lldb/source/Core/Communication.cpp

35

Yeah calling Disconnect in an unlocked fashion would solve that if you called it after locking in Connect.

[JDevlieghere]

• Remove Communication::Clear method

[JDevlieghere]

lldb/source/Core/ThreadedCommunication.cpp

63–67

This wasn't used: nobody called Clear on an instance of ThreadedCommunication.

lldb/source/Plugins/Process/MacOSX-Kernel/ProcessKDP.cpp

560

According to the comment on line 541 this is wrong:

 // We are running and we can't interrupt a running kernel, so we need to
// just close the connection to the kernel and hope for the best

m_comm is an instance of CommunicationKDP which didn't override Communication::Clear and hence just called Disconnected.

[bulbazord]

LGTM

lldb/source/Core/Communication.cpp

115–118

Unrelated to your patch but something about this is triggering my spider sense. Is it possible that status == eConnectionStatusSuccess while WriteUnlocked returns 0? This might end up in an infinite loop then, no?

Obviously you don't need to address this since it's unrelated, but something to think about for resiliency.

[JDevlieghere]

Abandoning this in favor of D157347

lldb/include/lldb/Core/Communication.h

 #define LLDB_CORE_COMMUNICATION_H
 
 #include "lldb/Utility/Timeout.h"
 #include "lldb/lldb-defines.h"
 #include "lldb/lldb-enumerations.h"
 #include "lldb/lldb-forward.h"
 #include "lldb/lldb-types.h"
 
-#include <mutex>
+#include <shared_mutex>
 #include <string>
 
 namespace lldb_private {
 class Connection;
 class ConstString;
 class Status;
 
 /// \class Communication Communication.h "lldb/Core/Communication.h" An
public:
   /// Construct the Communication object.
   Communication();
 
   /// Destructor.
   ///
   /// 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.
   ///
   /// \param[in] url
   ///     A string that contains all information needed by the
   ///     subclass to connect to another client.
   ///
   /// \return
public:
 
   static std::string ConnectionStatusAsString(lldb::ConnectionStatus status);
 
   bool GetCloseOnEOF() const { return m_close_on_eof; }
 
   void SetCloseOnEOF(bool b) { m_close_on_eof = b; }
 
 protected:
+  size_t WriteUnlocked(const void *src, size_t src_len,
+                       lldb::ConnectionStatus &status, Status *error_ptr);
+  lldb::ConnectionStatus DisconnectUnlocked(Status *error_ptr = nullptr);
+
   lldb::ConnectionSP m_connection_sp; ///< The connection that is current in use
                                       ///by this communications class.
-  std::mutex
-      m_write_mutex; ///< Don't let multiple threads write at the same time...
+  mutable std::shared_mutex m_shared_mutex;
   bool m_close_on_eof;
 
   size_t ReadFromConnection(void *dst, size_t dst_len,
                             const Timeout<std::micro> &timeout,
                             lldb::ConnectionStatus &status, Status *error_ptr);
 
 private:
   Communication(const Communication &) = delete;
   const Communication &operator=(const Communication &) = delete;
 };
 
 } // namespace lldb_private
 
 #endif // LLDB_CORE_COMMUNICATION_H

lldb/include/lldb/Core/ThreadedCommunication.h

public:
   ///     is called.
   ThreadedCommunication(const char *broadcaster_name);
 
   /// Destructor.
   ///
   /// The destructor is virtual since this class gets subclassed.
   ~ThreadedCommunication() override;
 
-  void Clear() override;
-
   /// Disconnect the communications connection if one is currently connected.
   ///
   /// \return
   ///     \b True if the disconnect succeeded, \b false otherwise. The
   ///     internal error object should be filled in with an
   ///     appropriate value based on the result of this function.
   ///
   /// \see Status& Communication::GetError ();

lldb/source/Core/Communication.cpp

 #include <cerrno>
 #include <cinttypes>
 #include <cstdio>
 
 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_shared_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();
-
   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);
+  std::unique_lock guard(m_shared_mutex);
+
+  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_shared_mutex);
+  return DisconnectUnlocked();
+}
+
+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_shared_mutex);
+  return (m_connection_sp ? m_connection_sp->IsConnected() : false);
 }
 
 bool Communication::HasConnection() const {
+  std::shared_lock guard(m_shared_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);
+  std::shared_lock guard(m_shared_mutex);
   LLDB_LOG(
-      log,
+      GetLog(LLDBLog::Communication),
       "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);
 }
 
 size_t Communication::Write(const void *src, size_t src_len,
                             ConnectionStatus &status, Status *error_ptr) {
-  lldb::ConnectionSP connection_sp(m_connection_sp);
+  std::unique_lock guard(m_shared_mutex);
+  return WriteUnlocked(src, src_len, status, error_ptr);
+}
 
-  std::lock_guard<std::mutex> guard(m_write_mutex);
+size_t Communication::WriteUnlocked(const void *src, size_t src_len,
+                                    ConnectionStatus &status,
+                                    Status *error_ptr) {
   LLDB_LOG(GetLog(LLDBLog::Communication),
            "{0} Communication::Write (src = {1}, src_len = {2}"
            ") connection = {3}",
-           this, src, (uint64_t)src_len, connection_sp.get());
+           this, src, (uint64_t)src_len, m_connection_sp.get());
 
-  if (connection_sp)
-    return connection_sp->Write(src, src_len, status, error_ptr);
+  if (m_connection_sp)
+    return m_connection_sp->Write(src, src_len, status, error_ptr);
 
   if (error_ptr)
     error_ptr->SetErrorString("Invalid connection.");
   status = eConnectionStatusNoConnection;
   return 0;
 }
 
 size_t Communication::WriteAll(const void *src, size_t src_len,
                                ConnectionStatus &status, Status *error_ptr) {
+  std::unique_lock guard(m_shared_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,
   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) {
+  std::shared_lock guard(m_shared_mutex);
   lldb::ConnectionSP connection_sp(m_connection_sp);
   if (connection_sp)
     return connection_sp->Read(dst, dst_len, timeout, status, error_ptr);
 
   if (error_ptr)
     error_ptr->SetErrorString("Invalid connection.");
   status = eConnectionStatusNoConnection;
   return 0;

lldb/source/Core/ThreadedCommunication.cpp

 }
 
 ThreadedCommunication::~ThreadedCommunication() {
   LLDB_LOG(GetLog(LLDBLog::Object | LLDBLog::Communication),
            "{0} ThreadedCommunication::~ThreadedCommunication (name = {1})",
            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) &&
          "Disconnecting while the read thread is running is racy!");
   return Communication::Disconnect(error_ptr);
 }
 
 size_t ThreadedCommunication::Read(void *dst, size_t dst_len,
                                    const Timeout<std::micro> &timeout,

lldb/source/Plugins/Process/MacOSX-Kernel/ProcessKDP.cpp

if (!keep_stopped && m_comm.IsConnected()) {
         else
           log->PutCString(
               "ProcessKDP::DoDetach() connection channel shutdown failed");
       }
       m_comm.Disconnect();
     }
   }
   StopAsyncThread();
-  m_comm.Clear();
 
   SetPrivateState(eStateDetached);
   ResumePrivateStateThread();
 
   // KillDebugserverProcess ();
   return error;
 }