Author: Augusto Noronha Date: 2023-08-15T16:03:28-07:00 New Revision: 3b919570f2f08581987de7851f3673352afb1578
URL: https://github.com/llvm/llvm-project/commit/3b919570f2f08581987de7851f3673352afb1578 DIFF: https://github.com/llvm/llvm-project/commit/3b919570f2f08581987de7851f3673352afb1578.diff LOG: Revert "[lldb] Properly protect the Communication class with reader/writer lock" This reverts commit 5d16957207ce1bd1a2091f3677e176012009c59a. 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 7e4ce362acf9a6..f5f636816cb4f9 100644 --- a/lldb/include/lldb/Core/Communication.h +++ b/lldb/include/lldb/Core/Communication.h @@ -16,7 +16,6 @@ #include "lldb/lldb-types.h" #include <mutex> -#include <shared_mutex> #include <string> namespace lldb_private { @@ -47,6 +46,8 @@ 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. /// @@ -83,10 +84,7 @@ class Communication { bool HasConnection() const; - lldb_private::Connection *GetConnection() { - std::shared_lock guard(m_connection_mutex); - return m_connection_sp.get(); - } + lldb_private::Connection *GetConnection() { return m_connection_sp.get(); } /// Read bytes from the current connection. /// @@ -171,24 +169,13 @@ 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 170fd2dfcb555d..7ebb77beb77f3d 100644 --- a/lldb/include/lldb/Core/ThreadedCommunication.h +++ b/lldb/include/lldb/Core/ThreadedCommunication.h @@ -97,6 +97,8 @@ 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 e2ba461a02329a..5d890632ccc6a9 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_connection_mutex(), m_close_on_eof(true) {} + : m_connection_sp(), m_write_mutex(), m_close_on_eof(true) { +} + +Communication::~Communication() { + Clear(); +} -Communication::~Communication() { Disconnect(nullptr); } +void Communication::Clear() { + Disconnect(nullptr); +} ConnectionStatus Communication::Connect(const char *url, Status *error_ptr) { - std::unique_lock guard(m_connection_mutex); + Clear(); LLDB_LOG(GetLog(LLDBLog::Communication), "{0} Communication::Connect (url = {1})", this, url); - DisconnectUnlocked(); - - if (m_connection_sp) - return m_connection_sp->Connect(url, error_ptr); + lldb::ConnectionSP connection_sp(m_connection_sp); + if (connection_sp) + return 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); - if (m_connection_sp) { - ConnectionStatus status = m_connection_sp->Disconnect(error_ptr); + 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(); return status; } return eConnectionStatusNoConnection; } bool Communication::IsConnected() const { - std::shared_lock guard(m_connection_mutex); - return (m_connection_sp ? m_connection_sp->IsConnected() : false); + lldb::ConnectionSP connection_sp(m_connection_sp); + return (connection_sp ? 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) { - std::shared_lock guard(m_connection_mutex); - return ReadUnlocked(dst, dst_len, timeout, 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); } size_t Communication::Write(const void *src, size_t src_len, ConnectionStatus &status, Status *error_ptr) { - // 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; - } + lldb::ConnectionSP connection_sp(m_connection_sp); + 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, m_connection_sp.get()); + this, src, (uint64_t)src_len, connection_sp.get()); + + if (connection_sp) + return connection_sp->Write(src, src_len, status, error_ptr); - 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::shared_lock guard(m_connection_mutex); - std::lock_guard<std::mutex> guard_write(m_write_mutex); size_t total_written = 0; do - total_written += - WriteUnlocked(static_cast<const char *>(src) + total_written, - src_len - total_written, status, error_ptr); + total_written += Write(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::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); +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); if (error_ptr) error_ptr->SetErrorString("Invalid connection."); @@ -139,8 +139,7 @@ size_t Communication::ReadUnlocked(void *dst, size_t dst_len, } void Communication::SetConnection(std::unique_ptr<Connection> connection) { - std::unique_lock guard(m_connection_mutex); - DisconnectUnlocked(nullptr); + Disconnect(nullptr); m_connection_sp = std::move(connection); } diff --git a/lldb/source/Core/ThreadedCommunication.cpp b/lldb/source/Core/ThreadedCommunication.cpp index 8d3515652d8b85..7d8aae5d8ff689 100644 --- a/lldb/source/Core/ThreadedCommunication.cpp +++ b/lldb/source/Core/ThreadedCommunication.cpp @@ -61,6 +61,11 @@ 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) && @@ -72,7 +77,6 @@ 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, @@ -148,7 +152,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::ReadUnlocked(dst, dst_len, timeout, status, error_ptr); + return Communication::Read(dst, dst_len, timeout, status, error_ptr); } bool ThreadedCommunication::StartReadThread(Status *error_ptr) { @@ -269,50 +273,46 @@ lldb::thread_result_t ThreadedCommunication::ReadThread() { ConnectionStatus status = eConnectionStatusSuccess; bool done = false; bool disconnect = false; - - { - 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; + 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 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,12 +361,8 @@ void ThreadedCommunication::SynchronizeWithReadThread() { if (!m_read_thread_enabled || m_read_thread_did_exit) return; - { - // Notify the read thread. - std::shared_lock guard(m_connection_mutex); - if (m_connection_sp) - m_connection_sp->InterruptRead(); - } + // Notify the read thread. + 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 6862e43b6d5d35..b17701ea4a15cf 100644 --- a/lldb/source/Plugins/Process/MacOSX-Kernel/ProcessKDP.cpp +++ b/lldb/source/Plugins/Process/MacOSX-Kernel/ProcessKDP.cpp @@ -557,6 +557,7 @@ Status ProcessKDP::DoDetach(bool keep_stopped) { } } StopAsyncThread(); + m_comm.Clear(); SetPrivateState(eStateDetached); ResumePrivateStateThread(); _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
