[lldb] Fix data race in ThreadList

[augusto2112]

Summary

ThreadSanitizer reports the following issue:

Write of size 8 at 0x00010a70abb0 by thread T3 (mutexes: write M0):
  #0 lldb_private::ThreadList::Update(lldb_private::ThreadList&) ThreadList.cpp:741 (liblldb.18.0.0git.dylib:arm64+0x5dedf4) (BuildId: 9bced2aafa373580ae9d750d9cf79a8f32000000200000000100000000000e00)
  #1 lldb_private::Process::UpdateThreadListIfNeeded() Process.cpp:1212 (liblldb.18.0.0git.dylib:arm64+0x53bbec) (BuildId: 9bced2aafa373580ae9d750d9cf79a8f32000000200000000100000000000e00)

Previous read of size 8 at 0x00010a70abb0 by main thread (mutexes: write M1):
  #0 lldb_private::ThreadList::GetMutex() const ThreadList.cpp:785 (liblldb.18.0.0git.dylib:arm64+0x5df138) (BuildId: 9bced2aafa373580ae9d750d9cf79a8f32000000200000000100000000000e00)
  #1 lldb_private::ThreadList::DidResume() ThreadList.cpp:656 (liblldb.18.0.0git.dylib:arm64+0x5de5c0) (BuildId: 9bced2aafa373580ae9d750d9cf79a8f32000000200000000100000000000e00)
  #2 lldb_private::Process::PrivateResume() Process.cpp:3130 (liblldb.18.0.0git.dylib:arm64+0x53cd7c) (BuildId: 9bced2aafa373580ae9d750d9cf79a8f32000000200000000100000000000e00)

Fix this by only using the mutex in ThreadList and removing the one in
process entirely.

[JDevlieghere]

LGTM

[labath]

After this patch, python_api/run_locker/TestRunLocker.py becomes flaky. https://lab.llvm.org/buildbot/#/builders/68/builds/58456 is the first such failure, but there have been about a dozen failures since then. The backtraces on the buildbot page are fairly useless, but I was able to capture this backtrace locally:

include/c++/v1/vector:1537: assertion __n < size() failed: vector[] index out of bounds
Stack dump without symbol names (ensure you have llvm-symbolizer in your PATH or set the environment var `LLVM_SYMBOLIZER_PATH` to point to it):
0  0x000055dd180146be llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) + 46
1  0x000055dd18014d6c
2  0x00007fea9b3ce1c0
3  0x00007fea9b236347 raise + 167
4  0x00007fea9b237797 abort + 247
5  0x000055dd18862597
6  0x000055dd124c4563 lldb_private::process_gdb_remote::ProcessGDBRemote::SetThreadPc(std::__u::shared_ptr<lldb_private::Thread> const&, unsigned long) + 275
7  0x000055dd124c42fc lldb_private::process_gdb_remote::ProcessGDBRemote::DoUpdateThreadList(lldb_private::ThreadList&, lldb_private::ThreadList&) + 748
8  0x000055dd129c8687 lldb_private::Process::UpdateThreadListIfNeeded() + 247
9  0x000055dd12a39797 lldb_private::ThreadList::FindThreadByID(unsigned long, bool) + 55
10 0x000055dd12a396db lldb_private::ThreadList::GetSelectedThread() + 43
11 0x000055dd129a8dea lldb_private::ExecutionContext::ExecutionContext(lldb_private::Target*, bool) + 234
12 0x00007fea99840bf9 lldb::SBTarget::EvaluateExpression(char const*, lldb::SBExpressionOptions const&) + 281
13 0x00007fea99840a7b lldb::SBTarget::EvaluateExpression(char const*) + 187
14 0x00007fea9995eb77
15 0x000055dd185907cd
16 0x000055dd18545814 _PyObject_Call + 292
17 0x000055dd1865fecb _PyEval_EvalFrameDefault + 11515
18 0x000055dd1865cfa8 _PyEval_Vector + 184
19 0x000055dd186666c3
20 0x000055dd18666906
21 0x000055dd1865e56f _PyEval_EvalFrameDefault + 5023
22 0x000055dd1865cfa8 _PyEval_Vector + 184
23 0x000055dd186666c3
24 0x000055dd18666906
25 0x000055dd1865e56f _PyEval_EvalFrameDefault + 5023
26 0x000055dd1865cfa8 _PyEval_Vector + 184
27 0x000055dd1865fecb _PyEval_EvalFrameDefault + 11515
28 0x000055dd1865cfa8 _PyEval_Vector + 184
<a lot of additional python frames>

Given that the failure is in the DoUpdateThreadList function, I'm guessing that this patch removed/reduced some existing synchronization which prevented this code from executing concurrently with something else (I don't know what), although it's possible that the right fix (since updating the thread list while the process is running doesn't make sense) is to bail out of EvaluateExpression sooner (@jingham ?).

If you want to reproduce this, note that the bug reproduces relatively infrequently (~1% for me, though it seems to be a bit higher on the buildbot), so you may need to run it many times before you can catch it in action. The failing part is not always the same (e.g. sometimes it just hangs), but I expect the root cause to be the same. I've also confirmed that the failure rate of the test goes down to zero after reverting this patch.

Can you investigate (and possibly revert this patch in the mean time)?

lldb/include/lldb/Target/Process.h

uint32_t m_process_unique_id; ///< Each lldb_private::Process class that is
                                 ///increments with each new instance
   uint32_t m_thread_index_id;   ///< Each thread is created with a 1 based index
                                 ///that won't get re-used.
   std::map<uint64_t, uint32_t> m_thread_id_to_index_id_map;
   int m_exit_status; ///< The exit status of the process, or -1 if not set.
   std::string m_exit_string; ///< A textual description of why a process exited.
   std::mutex m_exit_status_mutex; ///< Mutex so m_exit_status m_exit_string can
                                   ///be safely accessed from multiple threads
-  std::recursive_mutex m_thread_mutex;
   ThreadList m_thread_list_real; ///< The threads for this process as are known
                                  ///to the protocol we are debugging with
   ThreadList m_thread_list; ///< The threads for this process as the user will
                             ///see them. This is usually the same as
   ///< m_thread_list_real, but might be different if there is an OS plug-in
   ///creating memory threads
   ThreadPlanStackMap m_thread_plans; ///< This is the list of thread plans for
                                      /// threads in m_thread_list, as well as

lldb/include/lldb/Target/ThreadCollection.h

public:
   // index to accessing the current threads, whereas "thread_index" is a unique
   // index assigned
   lldb::ThreadSP GetThreadAtIndex(uint32_t idx);
 
   virtual ThreadIterable Threads() {
     return ThreadIterable(m_threads, GetMutex());
   }
 
-  virtual std::recursive_mutex &GetMutex() const { return m_mutex; }
+  std::recursive_mutex &GetMutex() const { return m_mutex; }
 
 protected:
   collection m_threads;
   mutable std::recursive_mutex m_mutex;
 };
 
 } // namespace lldb_private
 
 #endif // LLDB_TARGET_THREADCOLLECTION_H

lldb/include/lldb/Target/ThreadList.h

public:
   void DidStop();
 
   void DiscardThreadPlans();
 
   uint32_t GetStopID() const;
 
   void SetStopID(uint32_t stop_id);
 
-  std::recursive_mutex &GetMutex() const override;
-
   void Update(ThreadList &rhs);
 
 protected:
   void SetShouldReportStop(Vote vote);
 
   void NotifySelectedThreadChanged(lldb::tid_t tid);
 
   // Classes that inherit from Process can see and modify these

lldb/source/Target/Process.cpp

: ProcessProperties(this),
       m_private_state_broadcaster(nullptr,
                                   "lldb.process.internal_state_broadcaster"),
       m_private_state_control_broadcaster(
           nullptr, "lldb.process.internal_state_control_broadcaster"),
       m_private_state_listener_sp(
           Listener::MakeListener("lldb.process.internal_state_listener")),
       m_mod_id(), m_process_unique_id(0), m_thread_index_id(0),
       m_thread_id_to_index_id_map(), m_exit_status(-1), m_exit_string(),
-      m_exit_status_mutex(), m_thread_mutex(), m_thread_list_real(this),
+      m_exit_status_mutex(), m_thread_list_real(this),
       m_thread_list(this), m_thread_plans(*this), m_extended_thread_list(this),
       m_extended_thread_stop_id(0), m_queue_list(this), m_queue_list_stop_id(0),
       m_notifications(), m_image_tokens(),
       m_breakpoint_site_list(), m_dynamic_checkers_up(),
       m_unix_signals_sp(unix_signals_sp), m_abi_sp(), m_process_input_reader(),
       m_stdio_communication("process.stdio"), m_stdio_communication_mutex(),
       m_stdin_forward(false), m_stdout_data(), m_stderr_data(),
       m_profile_data_comm_mutex(), m_profile_data(), m_iohandler_sync(0),
while (true) {
 
     if (event_sp)
       HandlePrivateEvent(event_sp);
   }
   return state;
 }
 
 void Process::LoadOperatingSystemPlugin(bool flush) {
-  std::lock_guard<std::recursive_mutex> guard(m_thread_mutex);
+  std::lock_guard<std::recursive_mutex> guard(GetThreadList().GetMutex());
   if (flush)
     m_thread_list.Clear();
   m_os_up.reset(OperatingSystem::FindPlugin(this, nullptr));
   if (flush)
     Flush();
 }
 
 Status Process::Launch(ProcessLaunchInfo &launch_info) {

lldb/source/Target/ThreadList.cpp

selected_thread_sp->BroadcastEvent(
         Thread::eBroadcastBitThreadSelected,
         new Thread::ThreadEventData(selected_thread_sp));
 }
 
 void ThreadList::Update(ThreadList &rhs) {
   if (this != &rhs) {
     // Lock both mutexes to make sure neither side changes anyone on us while
     // the assignment occurs
-    std::scoped_lock<std::recursive_mutex, std::recursive_mutex> guard(GetMutex(), rhs.GetMutex());
+    std::scoped_lock<std::recursive_mutex, std::recursive_mutex> guard(
+        GetMutex(), rhs.GetMutex());
 
     m_process = rhs.m_process;
     m_stop_id = rhs.m_stop_id;
     m_threads.swap(rhs.m_threads);
     m_selected_tid = rhs.m_selected_tid;
 
     // Now we look for threads that we are done with and make sure to clear
     // them up as much as possible so anyone with a shared pointer will still

 
 void ThreadList::Flush() {
   std::lock_guard<std::recursive_mutex> guard(GetMutex());
   collection::iterator pos, end = m_threads.end();
   for (pos = m_threads.begin(); pos != end; ++pos)
     (*pos)->Flush();
 }
 
-std::recursive_mutex &ThreadList::GetMutex() const {
-  return m_process->m_thread_mutex;
-}
-
 ThreadList::ExpressionExecutionThreadPusher::ExpressionExecutionThreadPusher(
     lldb::ThreadSP thread_sp)
     : m_thread_list(nullptr), m_tid(LLDB_INVALID_THREAD_ID) {
   if (thread_sp) {
     m_tid = thread_sp->GetID();
     m_thread_list = &thread_sp->GetProcess()->GetThreadList();
     m_thread_list->PushExpressionExecutionThread(m_tid);
   }
 }