[Lldb-commits] [PATCH] D157361: [lldb] FIx data race in ThreadedCommunication

[Jonas Devlieghere via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG1a8d9a7657bb: [lldb] Fix data race in ThreadedCommunication 
(authored by JDevlieghere).
Herald added a project: LLDB.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D157361/new/

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