[Lldb-commits] [PATCH] D157648: [lldb] Fix data race in Process

[Augusto Noronha via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG79a8e006dbc4: [lldb] Fix data race in Process (authored by 
augusto2112).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157648

Files:
  lldb/include/lldb/Target/Process.h
  lldb/source/Target/Process.cpp

Index: lldb/source/Target/Process.cpp
===
--- lldb/source/Target/Process.cpp
+++ lldb/source/Target/Process.cpp
@@ -629,6 +629,7 @@
 const Timeout ) {
   // don't sync (potentially context switch) in case where there is no process
   // IO
+  std::lock_guard guard(m_process_input_reader_mutex);
   if (!m_process_input_reader)
 return;
 
@@ -2504,7 +2505,11 @@
   m_jit_loaders_up.reset();
   m_system_runtime_up.reset();
   m_os_up.reset();
-  m_process_input_reader.reset();
+
+  {
+std::lock_guard guard(m_process_input_reader_mutex);
+m_process_input_reader.reset();
+  }
 
   Module *exe_module = GetTarget().GetExecutableModulePointer();
 
@@ -2802,7 +2807,10 @@
 
 Status Process::Attach(ProcessAttachInfo _info) {
   m_abi_sp.reset();
-  m_process_input_reader.reset();
+  {
+std::lock_guard guard(m_process_input_reader_mutex);
+m_process_input_reader.reset();
+  }
   m_dyld_up.reset();
   m_jit_loaders_up.reset();
   m_system_runtime_up.reset();
@@ -3053,7 +3061,10 @@
 
 Status Process::ConnectRemote(llvm::StringRef remote_url) {
   m_abi_sp.reset();
-  m_process_input_reader.reset();
+  {
+std::lock_guard guard(m_process_input_reader_mutex);
+m_process_input_reader.reset();
+  }
 
   // Find the process and its architecture.  Make sure it matches the
   // architecture of the current Target, and if not adjust it.
@@ -3341,10 +3352,13 @@
 m_stdio_communication.Disconnect();
 m_stdin_forward = false;
 
-if (m_process_input_reader) {
-  m_process_input_reader->SetIsDone(true);
-  m_process_input_reader->Cancel();
-  m_process_input_reader.reset();
+{
+  std::lock_guard guard(m_process_input_reader_mutex);
+  if (m_process_input_reader) {
+m_process_input_reader->SetIsDone(true);
+m_process_input_reader->Cancel();
+m_process_input_reader.reset();
+  }
 }
 
 // If we exited when we were waiting for a process to stop, then forward
@@ -4522,20 +4536,25 @@
 m_stdio_communication.StartReadThread();
 
 // Now read thread is set up, set up input reader.
-
-if (!m_process_input_reader)
-  m_process_input_reader =
-  std::make_shared(this, fd);
+{
+  std::lock_guard guard(m_process_input_reader_mutex);
+  if (!m_process_input_reader)
+m_process_input_reader =
+std::make_shared(this, fd);
+}
   }
 }
 
 bool Process::ProcessIOHandlerIsActive() {
+  std::lock_guard guard(m_process_input_reader_mutex);
   IOHandlerSP io_handler_sp(m_process_input_reader);
   if (io_handler_sp)
 return GetTarget().GetDebugger().IsTopIOHandler(io_handler_sp);
   return false;
 }
+
 bool Process::PushProcessIOHandler() {
+  std::lock_guard guard(m_process_input_reader_mutex);
   IOHandlerSP io_handler_sp(m_process_input_reader);
   if (io_handler_sp) {
 Log *log = GetLog(LLDBLog::Process);
@@ -4555,6 +4574,7 @@
 }
 
 bool Process::PopProcessIOHandler() {
+  std::lock_guard guard(m_process_input_reader_mutex);
   IOHandlerSP io_handler_sp(m_process_input_reader);
   if (io_handler_sp)
 return GetTarget().GetDebugger().RemoveIOHandler(io_handler_sp);
Index: lldb/include/lldb/Target/Process.h
===
--- lldb/include/lldb/Target/Process.h
+++ lldb/include/lldb/Target/Process.h
@@ -3007,6 +3007,7 @@
   m_unix_signals_sp; /// This is the current signal set for this process.
   lldb::ABISP m_abi_sp;
   lldb::IOHandlerSP m_process_input_reader;
+  mutable std::mutex m_process_input_reader_mutex;
   ThreadedCommunication m_stdio_communication;
   std::recursive_mutex m_stdio_communication_mutex;
   bool m_stdin_forward; /// Remember if stdin must be forwarded to remote debug
@@ -3132,6 +3133,7 @@
   bool ProcessIOHandlerIsActive();
 
   bool ProcessIOHandlerExists() const {
+std::lock_guard guard(m_process_input_reader_mutex);
 return static_cast(m_process_input_reader);
   }
 
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D157648: [lldb] Fix data race in Process

[Augusto Noronha via Phabricator via lldb-commits]

augusto2112 added a comment.

@JDevlieghere do you have any opinions on this?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157648

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D157648: [lldb] Fix data race in Process

[Augusto Noronha via Phabricator via lldb-commits]

augusto2112 added a comment.

In D157648#4578420 , @JDevlieghere 
wrote:

> Is the reported race specifically about the shared pointer being accessed 
> concurrently or operations on the `IOHandler`?

I believe it's the shared pointer being accessed. Here's an example of what's 
being reported:

  WARNING: ThreadSanitizer: data race (pid=52249)
Read of size 8 at 0x00010731ce38 by thread T3:
  #0 lldb_private::Process::PopProcessIOHandler() Process.cpp:4559 
(liblldb.18.0.0git.dylib:arm64+0x53a5a4) (BuildId: 
6e77321523b936d2955d63d4d25df7cd320021000e00)
  #1 
lldb_private::Process::HandlePrivateEvent(std::__1::shared_ptr&)
 Process.cpp:3775 (liblldb.18.0.0git.dylib:arm64+0x54204c) (BuildId: 
6e77321523b936d2955d63d4d25df7cd320021000e00)
  #2 lldb_private::Process::RunPrivateStateThread(bool) Process.cpp:3904 
(liblldb.18.0.0git.dylib:arm64+0x54874c) (BuildId: 
6e77321523b936d2955d63d4d25df7cd320021000e00)
  #3 
std::__1::__function::__func, 
void* ()>::operator()() function.h:356 (liblldb.18.0.0git.dylib:arm64+0x555de4) 
(BuildId: 6e77321523b936d2955d63d4d25df7cd320021000e00)
  #4 lldb_private::HostNativeThreadBase::ThreadCreateTrampoline(void*) 
HostNativeThreadBase.cpp:62 (liblldb.18.0.0git.dylib:arm64+0x3e83fc) (BuildId: 
6e77321523b936d2955d63d4d25df7cd320021000e00)
  #5 lldb_private::HostThreadMacOSX::ThreadCreateTrampoline(void*) 
HostThreadMacOSX.mm:18 (liblldb.18.0.0git.dylib:arm64+0x175056c) (BuildId: 
6e77321523b936d2955d63d4d25df7cd320021000e00)
  
Previous write of size 8 at 0x00010731ce38 by main thread (mutexes: write 
M0):
  #0 lldb_private::Process::SetSTDIOFileDescriptor(int) Process.cpp:4528 
(liblldb.18.0.0git.dylib:arm64+0x54aa5c) (BuildId: 
6e77321523b936d2955d63d4d25df7cd320021000e00)
  #1 lldb_private::Platform::DebugProcess(lldb_private::ProcessLaunchInfo&, 
lldb_private::Debugger&, lldb_private::Target&, lldb_private::Status&) 
Platform.cpp:1120 (liblldb.18.0.0git.dylib:arm64+0x52bc54) (BuildId: 
6e77321523b936d2955d63d4d25df7cd320021000e00)
  #2 
lldb_private::PlatformDarwin::DebugProcess(lldb_private::ProcessLaunchInfo&, 
lldb_private::Debugger&, lldb_private::Target&, lldb_private::Status&) 
PlatformDarwin.cpp:711 (liblldb.18.0.0git.dylib:arm64+0x872cc8) (BuildId: 
6e77321523b936d2955d63d4d25df7cd320021000e00)
  #3 lldb_private::Target::Launch(lldb_private::ProcessLaunchInfo&, 
lldb_private::Stream*) Target.cpp:3235 (liblldb.18.0.0git.dylib:arm64+0x5b118c) 
(BuildId: 6e77321523b936d2955d63d4d25df7cd320021000e00)

Line 4559 is

  IOHandlerSP io_handler_sp(m_process_input_reader);

Line 4528 is

  m_process_input_reader =
  std::make_shared(this, fd);


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157648

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D157648: [lldb] Fix data race in Process

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added a comment.

Is the reported race specifically about the shared pointer being accessed 
concurrently or operations on the `IOHandler`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157648

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D157648: [lldb] Fix data race in Process

[Augusto Noronha via Phabricator via lldb-commits]

augusto2112 created this revision.
augusto2112 added reviewers: bulbazord, JDevlieghere.
Herald added a project: All.
augusto2112 requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

Thread sanitizer reports a data race in Process.cpp in the usage of
m_process_input_reader. Fix this data race by introducing a mutex
guarding the access to this variable.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D157648

Files:
  lldb/include/lldb/Target/Process.h
  lldb/source/Target/Process.cpp

Index: lldb/source/Target/Process.cpp
===
--- lldb/source/Target/Process.cpp
+++ lldb/source/Target/Process.cpp
@@ -630,6 +630,7 @@
 const Timeout ) {
   // don't sync (potentially context switch) in case where there is no process
   // IO
+  std::lock_guard guard(m_process_input_reader_mutex);
   if (!m_process_input_reader)
 return;
 
@@ -2505,7 +2506,11 @@
   m_jit_loaders_up.reset();
   m_system_runtime_up.reset();
   m_os_up.reset();
-  m_process_input_reader.reset();
+
+  {
+std::lock_guard guard(m_process_input_reader_mutex);
+m_process_input_reader.reset();
+  }
 
   Module *exe_module = GetTarget().GetExecutableModulePointer();
 
@@ -2803,7 +2808,10 @@
 
 Status Process::Attach(ProcessAttachInfo _info) {
   m_abi_sp.reset();
-  m_process_input_reader.reset();
+  {
+std::lock_guard guard(m_process_input_reader_mutex);
+m_process_input_reader.reset();
+  }
   m_dyld_up.reset();
   m_jit_loaders_up.reset();
   m_system_runtime_up.reset();
@@ -3054,7 +3062,10 @@
 
 Status Process::ConnectRemote(llvm::StringRef remote_url) {
   m_abi_sp.reset();
-  m_process_input_reader.reset();
+  {
+std::lock_guard guard(m_process_input_reader_mutex);
+m_process_input_reader.reset();
+  }
 
   // Find the process and its architecture.  Make sure it matches the
   // architecture of the current Target, and if not adjust it.
@@ -3342,10 +3353,13 @@
 m_stdio_communication.Disconnect();
 m_stdin_forward = false;
 
-if (m_process_input_reader) {
-  m_process_input_reader->SetIsDone(true);
-  m_process_input_reader->Cancel();
-  m_process_input_reader.reset();
+{
+  std::lock_guard guard(m_process_input_reader_mutex);
+  if (m_process_input_reader) {
+m_process_input_reader->SetIsDone(true);
+m_process_input_reader->Cancel();
+m_process_input_reader.reset();
+  }
 }
 
 // If we exited when we were waiting for a process to stop, then forward
@@ -4523,20 +4537,25 @@
 m_stdio_communication.StartReadThread();
 
 // Now read thread is set up, set up input reader.
-
-if (!m_process_input_reader)
-  m_process_input_reader =
-  std::make_shared(this, fd);
+{
+  std::lock_guard guard(m_process_input_reader_mutex);
+  if (!m_process_input_reader)
+m_process_input_reader =
+std::make_shared(this, fd);
+}
   }
 }
 
 bool Process::ProcessIOHandlerIsActive() {
+  std::lock_guard guard(m_process_input_reader_mutex);
   IOHandlerSP io_handler_sp(m_process_input_reader);
   if (io_handler_sp)
 return GetTarget().GetDebugger().IsTopIOHandler(io_handler_sp);
   return false;
 }
+
 bool Process::PushProcessIOHandler() {
+  std::lock_guard guard(m_process_input_reader_mutex);
   IOHandlerSP io_handler_sp(m_process_input_reader);
   if (io_handler_sp) {
 Log *log = GetLog(LLDBLog::Process);
@@ -4556,6 +4575,7 @@
 }
 
 bool Process::PopProcessIOHandler() {
+  std::lock_guard guard(m_process_input_reader_mutex);
   IOHandlerSP io_handler_sp(m_process_input_reader);
   if (io_handler_sp)
 return GetTarget().GetDebugger().RemoveIOHandler(io_handler_sp);
Index: lldb/include/lldb/Target/Process.h
===
--- lldb/include/lldb/Target/Process.h
+++ lldb/include/lldb/Target/Process.h
@@ -2992,6 +2992,7 @@
   m_unix_signals_sp; /// This is the current signal set for this process.
   lldb::ABISP m_abi_sp;
   lldb::IOHandlerSP m_process_input_reader;
+  mutable std::mutex m_process_input_reader_mutex;
   ThreadedCommunication m_stdio_communication;
   std::recursive_mutex m_stdio_communication_mutex;
   bool m_stdin_forward; /// Remember if stdin must be forwarded to remote debug
@@ -3117,6 +3118,7 @@
   bool ProcessIOHandlerIsActive();
 
   bool ProcessIOHandlerExists() const {
+std::lock_guard guard(m_process_input_reader_mutex);
 return static_cast(m_process_input_reader);
   }
 
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits