JDevlieghere created this revision.
JDevlieghere added reviewers: bulbazord, augusto2112.
Herald added a project: All.
JDevlieghere requested review of this revision.
TSan reports the following race:
Write of size 8 at 0x000107707ee8 by main thread:
#0 lldb_private::ThreadedCommunication::StartReadThread(...)
ThreadedCommunication.cpp:175
#1 lldb_private::Process::SetSTDIOFileDescriptor(...) Process.cpp:4533
#2 lldb_private::Platform::DebugProcess(...) Platform.cpp:1121
#3 lldb_private::PlatformDarwin::DebugProcess(...) PlatformDarwin.cpp:711
#4 lldb_private::Target::Launch(...) Target.cpp:3235
#5 CommandObjectProcessLaunch::DoExecute(...) CommandObjectProcess.cpp:256
#6 lldb_private::CommandObjectParsed::Execute(...) CommandObject.cpp:751
#7 lldb_private::CommandInterpreter::HandleCommand(...)
CommandInterpreter.cpp:2054
Previous read of size 8 at 0x000107707ee8 by thread T5:
#0 lldb_private::HostThread::IsJoinable(...) const HostThread.cpp:30
#1 lldb_private::ThreadedCommunication::StopReadThread(...)
ThreadedCommunication.cpp:192
#2 lldb_private::Process::ShouldBroadcastEvent(...) Process.cpp:3420
#3 lldb_private::Process::HandlePrivateEvent(...) Process.cpp:3728
#4 lldb_private::Process::RunPrivateStateThread(...) Process.cpp:3914
#5
std::__1::__function::__func<lldb_private::Process::StartPrivateStateThread(...)
function.h:356
#6 lldb_private::HostNativeThreadBase::ThreadCreateTrampoline(...)
HostNativeThreadBase.cpp:62
#7 lldb_private::HostThreadMacOSX::ThreadCreateTrampoline(...)
HostThreadMacOSX.mm:18
The problem is the lack of synchronization between starting and stopping the
read thread. This patch fixes that by protecting those operations with a mutex.
https://reviews.llvm.org/D157361
Files:
lldb/include/lldb/Core/ThreadedCommunication.h
lldb/source/Core/ThreadedCommunication.cpp
Index: lldb/source/Core/ThreadedCommunication.cpp
===================================================================
--- lldb/source/Core/ThreadedCommunication.cpp
+++ lldb/source/Core/ThreadedCommunication.cpp
@@ -23,6 +23,7 @@
#include <chrono>
#include <cstring>
#include <memory>
+#include <shared_mutex>
#include <cerrno>
#include <cinttypes>
@@ -155,6 +156,8 @@
}
bool ThreadedCommunication::StartReadThread(Status *error_ptr) {
+ std::lock_guard<std::mutex> lock(m_read_thread_mutex);
+
if (error_ptr)
error_ptr->Clear();
@@ -189,6 +192,8 @@
}
bool ThreadedCommunication::StopReadThread(Status *error_ptr) {
+ std::lock_guard<std::mutex> lock(m_read_thread_mutex);
+
if (!m_read_thread.IsJoinable())
return true;
@@ -199,13 +204,13 @@
BroadcastEvent(eBroadcastBitReadThreadShouldExit, nullptr);
- // error = m_read_thread.Cancel();
-
Status error = m_read_thread.Join(nullptr);
return error.Success();
}
bool ThreadedCommunication::JoinReadThread(Status *error_ptr) {
+ std::lock_guard<std::mutex> lock(m_read_thread_mutex);
+
if (!m_read_thread.IsJoinable())
return true;
Index: lldb/include/lldb/Core/ThreadedCommunication.h
===================================================================
--- lldb/include/lldb/Core/ThreadedCommunication.h
+++ lldb/include/lldb/Core/ThreadedCommunication.h
@@ -223,10 +223,22 @@
}
protected:
- HostThread m_read_thread; ///< The read thread handle in case we need to
- /// cancel the thread.
+ /// The read thread handle in case we need to cancel the thread.
+ /// @{
+ HostThread m_read_thread;
+ std::mutex m_read_thread_mutex;
+ /// @}
+
+ /// Whether the read thread is enabled. This cannot be guarded by the read
+ /// thread mutex becuase it is used as the control variable to exit the read
+ /// thread.
std::atomic<bool> m_read_thread_enabled;
+
+ /// Whether the read thread is enabled. Technically this could be guarded by
+ /// the read thread mutex but that needlessly complicates things to
+ /// check this variables momentary value.
std::atomic<bool> m_read_thread_did_exit;
+
std::string
m_bytes; ///< A buffer to cache bytes read in the ReadThread function.
std::recursive_mutex m_bytes_mutex; ///< A mutex to protect multi-threaded
Index: lldb/source/Core/ThreadedCommunication.cpp
===================================================================
--- lldb/source/Core/ThreadedCommunication.cpp
+++ lldb/source/Core/ThreadedCommunication.cpp
@@ -23,6 +23,7 @@
#include <chrono>
#include <cstring>
#include <memory>
+#include <shared_mutex>
#include <cerrno>
#include <cinttypes>
@@ -155,6 +156,8 @@
}
bool ThreadedCommunication::StartReadThread(Status *error_ptr) {
+ std::lock_guard<std::mutex> lock(m_read_thread_mutex);
+
if (error_ptr)
error_ptr->Clear();
@@ -189,6 +192,8 @@
}
bool ThreadedCommunication::StopReadThread(Status *error_ptr) {
+ std::lock_guard<std::mutex> lock(m_read_thread_mutex);
+
if (!m_read_thread.IsJoinable())
return true;
@@ -199,13 +204,13 @@
BroadcastEvent(eBroadcastBitReadThreadShouldExit, nullptr);
- // error = m_read_thread.Cancel();
-
Status error = m_read_thread.Join(nullptr);
return error.Success();
}
bool ThreadedCommunication::JoinReadThread(Status *error_ptr) {
+ std::lock_guard<std::mutex> lock(m_read_thread_mutex);
+
if (!m_read_thread.IsJoinable())
return true;
Index: lldb/include/lldb/Core/ThreadedCommunication.h
===================================================================
--- lldb/include/lldb/Core/ThreadedCommunication.h
+++ lldb/include/lldb/Core/ThreadedCommunication.h
@@ -223,10 +223,22 @@
}
protected:
- HostThread m_read_thread; ///< The read thread handle in case we need to
- /// cancel the thread.
+ /// The read thread handle in case we need to cancel the thread.
+ /// @{
+ HostThread m_read_thread;
+ std::mutex m_read_thread_mutex;
+ /// @}
+
+ /// Whether the read thread is enabled. This cannot be guarded by the read
+ /// thread mutex becuase it is used as the control variable to exit the read
+ /// thread.
std::atomic<bool> m_read_thread_enabled;
+
+ /// Whether the read thread is enabled. Technically this could be guarded by
+ /// the read thread mutex but that needlessly complicates things to
+ /// check this variables momentary value.
std::atomic<bool> m_read_thread_did_exit;
+
std::string
m_bytes; ///< A buffer to cache bytes read in the ReadThread function.
std::recursive_mutex m_bytes_mutex; ///< A mutex to protect multi-threaded
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
