[Lldb-commits] [lldb] r357141 - Copy the breakpoint site owner's collection so we can drop
Pavel Labath via lldb-commits
lldb-commits at lists.llvm.org
Thu Mar 28 02:01:44 PDT 2019
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>.
More information about the lldb-commits
mailing list