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

[Davide Italiano via lldb-commits]

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?

--
Davide