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

[Jim Ingham via lldb-commits]

Does 

https://reviews.llvm.org/D59957 

look right?  I hadn't used this before but it seems to do the right thing.

Jim


> On Mar 28, 2019, at 2:01 AM, Pavel Labath <pavel at labath.sk> wrote:
> 
> 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>.