JDevlieghere created this revision.
JDevlieghere added reviewers: aprantl, augusto2112.
Herald added a project: All.
JDevlieghere requested review of this revision.
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.
https://reviews.llvm.org/D157159
Files:
lldb/include/lldb/Core/Communication.h
lldb/source/Core/Communication.cpp
Index: lldb/source/Core/Communication.cpp
===================================================================
--- lldb/source/Core/Communication.cpp
+++ lldb/source/Core/Communication.cpp
@@ -27,8 +27,7 @@
using namespace lldb_private;
Communication::Communication()
- : m_connection_sp(), m_write_mutex(), m_close_on_eof(true) {
-}
+ : m_connection_sp(), m_shared_mutex(), m_close_on_eof(true) {}
Communication::~Communication() {
Clear();
@@ -41,54 +40,46 @@
ConnectionStatus Communication::Connect(const char *url, Status *error_ptr) {
Clear();
+ std::unique_lock guard(m_shared_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);
+ 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);
+
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());
@@ -97,16 +88,20 @@
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.");
@@ -116,10 +111,12 @@
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,
+ src_len - total_written, status, error_ptr);
while (status == eConnectionStatusSuccess && total_written < src_len);
return total_written;
}
@@ -128,6 +125,7 @@
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);
Index: lldb/include/lldb/Core/Communication.h
===================================================================
--- lldb/include/lldb/Core/Communication.h
+++ lldb/include/lldb/Core/Communication.h
@@ -15,7 +15,7 @@
#include "lldb/lldb-forward.h"
#include "lldb/lldb-types.h"
-#include <mutex>
+#include <shared_mutex>
#include <string>
namespace lldb_private {
@@ -165,10 +165,12 @@
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::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,
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
