[Lldb-commits] [lldb] r357141 - Copy the breakpoint site owner's collection so we can drop

[Jim Ingham via lldb-commits]

Author: jingham
Date: Wed Mar 27 18:51:33 2019
New Revision: 357141

URL: http://llvm.org/viewvc/llvm-project?rev=357141&view=rev
Log:
Copy the breakpoint site owner's collection so we can drop
the collection lock before we iterate over the owners calling ShouldStop.

BreakpointSite::ShouldStop can do a lot of work, and might by chance hit the same breakpoint
site again on another thread.  So instead of holding the site's owners lock
while iterating over them calling ShouldStop, I make a local copy of the list, drop the lock 
and then iterate over the copy calling BreakpointLocation::ShouldStop.

It's actually quite difficult to make this cause problems because usually all the
action happens on the private state thread, and the lock is recursive.

I have a report where some code hit the ASAN error breakpoint, went to 
compile the ASAN error gathering expression, in the course of compiling
that we went to fetch the ObjC runtime data, but the state of the program
was such that the ObjC runtime grubbing function triggered an ASAN error and 
we were executing that function on another thread.  

I couldn't figure out a way to reproduce that situation in a test.  But this is an 
NFC change anyway, it just makes the locking strategy more narrowly focused.

<rdar://problem/49074093>

Modified:
    lldb/trunk/include/lldb/Breakpoint/BreakpointLocationCollection.h
    lldb/trunk/source/Breakpoint/BreakpointLocationCollection.cpp
    lldb/trunk/source/Breakpoint/BreakpointSite.cpp

Modified: lldb/trunk/include/lldb/Breakpoint/BreakpointLocationCollection.h
URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/include/lldb/Breakpoint/BreakpointLocationCollection.h?rev=357141&r1=357140&r2=357141&view=diff
==============================================================================
--- lldb/trunk/include/lldb/Breakpoint/BreakpointLocationCollection.h (original)
+++ lldb/trunk/include/lldb/Breakpoint/BreakpointLocationCollection.h Wed Mar 27 18:51:33 2019
@@ -22,6 +22,8 @@ public:
   BreakpointLocationCollection();
 
   ~BreakpointLocationCollection();
+  
+  BreakpointLocationCollection &operator=(const BreakpointLocationCollection &rhs);
 
   //------------------------------------------------------------------
   /// Add the breakpoint \a bp_loc_sp to the list.

Modified: lldb/trunk/source/Breakpoint/BreakpointLocationCollection.cpp
URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Breakpoint/BreakpointLocationCollection.cpp?rev=357141&r1=357140&r2=357141&view=diff
==============================================================================
--- lldb/trunk/source/Breakpoint/BreakpointLocationCollection.cpp (original)
+++ lldb/trunk/source/Breakpoint/BreakpointLocationCollection.cpp Wed Mar 27 18:51:33 2019
@@ -178,3 +178,20 @@ void BreakpointLocationCollection::GetDe
     (*pos)->GetDescription(s, level);
   }
 }
+
+BreakpointLocationCollection &BreakpointLocationCollection::operator=(
+    const BreakpointLocationCollection &rhs) {
+  // Same trick as in ModuleList to avoid lock inversion.
+  if (this != &rhs) {
+    if (uintptr_t(this) > uintptr_t(&rhs)) {
+      std::lock_guard<std::mutex> lhs_guard(m_collection_mutex);
+      std::lock_guard<std::mutex> rhs_guard(rhs.m_collection_mutex);
+      m_break_loc_collection = rhs.m_break_loc_collection;
+    } else {
+      std::lock_guard<std::mutex> lhs_guard(m_collection_mutex);
+      std::lock_guard<std::mutex> rhs_guard(rhs.m_collection_mutex);
+      m_break_loc_collection = rhs.m_break_loc_collection;
+    }
+  }
+  return *this;
+}

Modified: lldb/trunk/source/Breakpoint/BreakpointSite.cpp
URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Breakpoint/BreakpointSite.cpp?rev=357141&r1=357140&r2=357141&view=diff
==============================================================================
--- lldb/trunk/source/Breakpoint/BreakpointSite.cpp (original)
+++ lldb/trunk/source/Breakpoint/BreakpointSite.cpp Wed Mar 27 18:51:33 2019
@@ -48,9 +48,17 @@ break_id_t BreakpointSite::GetNextID() {
 // should continue.
 
 bool BreakpointSite::ShouldStop(StoppointCallbackContext *context) {
-  std::lock_guard<std::recursive_mutex> guard(m_owners_mutex);
   IncrementHitCount();
-  return m_owners.ShouldStop(context);
+  // ShouldStop can do a lot of work, and might even come come back and hit
+  // this breakpoint site again.  So don't hold the m_owners_mutex the whole
+  // while.  Instead make a local copy of the collection and call ShouldStop on
+  // the copy.
+  BreakpointLocationCollection owners_copy;
+  {
+    std::lock_guard<std::recursive_mutex> guard(m_owners_mutex);
+    owners_copy = m_owners;
+  }
+  return owners_copy.ShouldStop(context);
 }
 
 bool BreakpointSite::IsBreakpointAtThisSite(lldb::break_id_t bp_id) {