[Lldb-commits] [lldb] [lldb] Fix crash in BreakpointSite::BumpHitCounts (PR #166876)

[Jonas Devlieghere via lldb-commits]

https://github.com/JDevlieghere updated https://github.com/llvm/llvm-project/pull/166876

>From 38d0bd20d4b3d7b5fb7c053684a533b15510897f Mon Sep 17 00:00:00 2001
From: Jonas Devlieghere <jonas at devlieghere.com>
Date: Thu, 6 Nov 2025 16:07:30 -0800
Subject: [PATCH 1/2] [lldb] Fix crash in BreakpointSite::BumpHitCounts

Fix crash in BreakpointSite::BumpHitCounts due to missing
synchronization. When bumping the hit count, we were correctly acquiring
the constituents mutex, but didn't protect the breakpoint location
collection.

rdar://163760832
---
 lldb/include/lldb/Breakpoint/BreakpointLocationCollection.h | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/lldb/include/lldb/Breakpoint/BreakpointLocationCollection.h b/lldb/include/lldb/Breakpoint/BreakpointLocationCollection.h
index 124cb55eaf723..372bd0c51fe20 100644
--- a/lldb/include/lldb/Breakpoint/BreakpointLocationCollection.h
+++ b/lldb/include/lldb/Breakpoint/BreakpointLocationCollection.h
@@ -179,10 +179,11 @@ class BreakpointLocationCollection {
       m_preserved_bps;
 
 public:
-  typedef llvm::iterator_range<collection::const_iterator>
+  typedef LockingAdaptedIterable<std::mutex, collection>
       BreakpointLocationCollectionIterable;
   BreakpointLocationCollectionIterable BreakpointLocations() {
-    return BreakpointLocationCollectionIterable(m_break_loc_collection);
+    return BreakpointLocationCollectionIterable(m_break_loc_collection,
+                                                m_collection_mutex);
   }
 };
 } // namespace lldb_private

>From ea20d2745bb736abc01f3ae04af6c032fe22d124 Mon Sep 17 00:00:00 2001
From: Jonas Devlieghere <jonas at devlieghere.com>
Date: Mon, 10 Nov 2025 09:50:25 -0800
Subject: [PATCH 2/2] Make the BreakpointLocationCollection mutex recursive

---
 .../Breakpoint/BreakpointLocationCollection.h |  7 ++++---
 .../BreakpointLocationCollection.cpp          | 20 ++++++++++---------
 2 files changed, 15 insertions(+), 12 deletions(-)

diff --git a/lldb/include/lldb/Breakpoint/BreakpointLocationCollection.h b/lldb/include/lldb/Breakpoint/BreakpointLocationCollection.h
index 372bd0c51fe20..57acb82dd96e9 100644
--- a/lldb/include/lldb/Breakpoint/BreakpointLocationCollection.h
+++ b/lldb/include/lldb/Breakpoint/BreakpointLocationCollection.h
@@ -32,7 +32,8 @@ class BreakpointLocationCollection {
 
   ~BreakpointLocationCollection();
 
-  BreakpointLocationCollection &operator=(const BreakpointLocationCollection &rhs);
+  BreakpointLocationCollection &
+  operator=(const BreakpointLocationCollection &rhs);
 
   /// Add the breakpoint \a bp_loc_sp to the list.
   ///
@@ -172,14 +173,14 @@ class BreakpointLocationCollection {
                          lldb::break_id_t break_loc_id) const;
 
   collection m_break_loc_collection;
-  mutable std::mutex m_collection_mutex;
+  mutable std::recursive_mutex m_collection_mutex;
   /// These are used if we're preserving breakpoints in this list:
   const bool m_preserving_bkpts = false;
   std::map<std::pair<lldb::break_id_t, lldb::break_id_t>, lldb::BreakpointSP>
       m_preserved_bps;
 
 public:
-  typedef LockingAdaptedIterable<std::mutex, collection>
+  typedef LockingAdaptedIterable<std::recursive_mutex, collection>
       BreakpointLocationCollectionIterable;
   BreakpointLocationCollectionIterable BreakpointLocations() {
     return BreakpointLocationCollectionIterable(m_break_loc_collection,
diff --git a/lldb/source/Breakpoint/BreakpointLocationCollection.cpp b/lldb/source/Breakpoint/BreakpointLocationCollection.cpp
index 97715836ec104..adff4299a5289 100644
--- a/lldb/source/Breakpoint/BreakpointLocationCollection.cpp
+++ b/lldb/source/Breakpoint/BreakpointLocationCollection.cpp
@@ -24,7 +24,7 @@ BreakpointLocationCollection::BreakpointLocationCollection(bool preserving)
 BreakpointLocationCollection::~BreakpointLocationCollection() = default;
 
 void BreakpointLocationCollection::Add(const BreakpointLocationSP &bp_loc) {
-  std::lock_guard<std::mutex> guard(m_collection_mutex);
+  std::lock_guard<std::recursive_mutex> guard(m_collection_mutex);
   BreakpointLocationSP old_bp_loc =
       FindByIDPair(bp_loc->GetBreakpoint().GetID(), bp_loc->GetID());
   if (!old_bp_loc.get()) {
@@ -44,7 +44,7 @@ void BreakpointLocationCollection::Add(const BreakpointLocationSP &bp_loc) {
 
 bool BreakpointLocationCollection::Remove(lldb::break_id_t bp_id,
                                           lldb::break_id_t bp_loc_id) {
-  std::lock_guard<std::mutex> guard(m_collection_mutex);
+  std::lock_guard<std::recursive_mutex> guard(m_collection_mutex);
   collection::iterator pos = GetIDPairIterator(bp_id, bp_loc_id); // Predicate
   if (pos != m_break_loc_collection.end()) {
     if (m_preserving_bkpts) {
@@ -117,7 +117,7 @@ const BreakpointLocationSP BreakpointLocationCollection::FindByIDPair(
 }
 
 BreakpointLocationSP BreakpointLocationCollection::GetByIndex(size_t i) {
-  std::lock_guard<std::mutex> guard(m_collection_mutex);
+  std::lock_guard<std::recursive_mutex> guard(m_collection_mutex);
   BreakpointLocationSP stop_sp;
   if (i < m_break_loc_collection.size())
     stop_sp = m_break_loc_collection[i];
@@ -127,7 +127,7 @@ BreakpointLocationSP BreakpointLocationCollection::GetByIndex(size_t i) {
 
 const BreakpointLocationSP
 BreakpointLocationCollection::GetByIndex(size_t i) const {
-  std::lock_guard<std::mutex> guard(m_collection_mutex);
+  std::lock_guard<std::recursive_mutex> guard(m_collection_mutex);
   BreakpointLocationSP stop_sp;
   if (i < m_break_loc_collection.size())
     stop_sp = m_break_loc_collection[i];
@@ -168,7 +168,7 @@ bool BreakpointLocationCollection::ShouldStop(
 }
 
 bool BreakpointLocationCollection::ValidForThisThread(Thread &thread) {
-  std::lock_guard<std::mutex> guard(m_collection_mutex);
+  std::lock_guard<std::recursive_mutex> guard(m_collection_mutex);
   collection::iterator pos, begin = m_break_loc_collection.begin(),
                             end = m_break_loc_collection.end();
 
@@ -180,7 +180,7 @@ bool BreakpointLocationCollection::ValidForThisThread(Thread &thread) {
 }
 
 bool BreakpointLocationCollection::IsInternal() const {
-  std::lock_guard<std::mutex> guard(m_collection_mutex);
+  std::lock_guard<std::recursive_mutex> guard(m_collection_mutex);
   collection::const_iterator pos, begin = m_break_loc_collection.begin(),
                                   end = m_break_loc_collection.end();
 
@@ -197,7 +197,7 @@ bool BreakpointLocationCollection::IsInternal() const {
 
 void BreakpointLocationCollection::GetDescription(
     Stream *s, lldb::DescriptionLevel level) {
-  std::lock_guard<std::mutex> guard(m_collection_mutex);
+  std::lock_guard<std::recursive_mutex> guard(m_collection_mutex);
   collection::iterator pos, begin = m_break_loc_collection.begin(),
                             end = m_break_loc_collection.end();
 
@@ -212,8 +212,10 @@ BreakpointLocationCollection &BreakpointLocationCollection::operator=(
     const BreakpointLocationCollection &rhs) {
   if (this != &rhs) {
       std::lock(m_collection_mutex, rhs.m_collection_mutex);
-      std::lock_guard<std::mutex> lhs_guard(m_collection_mutex, std::adopt_lock);
-      std::lock_guard<std::mutex> rhs_guard(rhs.m_collection_mutex, std::adopt_lock);
+      std::lock_guard<std::recursive_mutex> lhs_guard(m_collection_mutex,
+                                                      std::adopt_lock);
+      std::lock_guard<std::recursive_mutex> rhs_guard(rhs.m_collection_mutex,
+                                                      std::adopt_lock);
       m_break_loc_collection = rhs.m_break_loc_collection;
   }
   return *this;