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 <pa...@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@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>. _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
