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

[Pavel Labath via lldb-commits]

On 28/03/2019 02:54, Davide Italiano via lldb-commits wrote:
> On Wed, Mar 27, 2019 at 6:49 PM Jim Ingham via lldb-commits
> <lldb-commits at lists.llvm.org> wrote:
>>
>> 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;
>> +    }
>> +  }
> 
> The two branches are identical, is this intentional?
> 

It probably isn't. Also, a better way to avoid lock inversion would be 
with std::lock <https://en.cppreference.com/w/cpp/thread/lock>.