[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