[Lldb-commits] [lldb] [lldb] Fix RangeDataVector::CombineConsecutiveEntriesWithEqualData (PR #127059)

[Pavel Labath via lldb-commits]

https://github.com/labath created https://github.com/llvm/llvm-project/pull/127059

Function was merging equal data even if they weren't adjecant. This caused a problem in command-disassemble.s test because the two ranges describing the function would be merged and "swallow" the function between them.

This PR copies/adapts the algorithm from
RangeVector::CombineConsecutiveEntries (which does not have the same problem) and also adds a call to ComputeUpperBounds as moving entries around invalidates the binary tree. (The lack of this call wasn't noticed until now either because we were not calling methods which rely on upper bounds (right now, it's only the ill-named FindEntryIndexes method), or because we weren't merging anything.

>From 2647987f2a33ddd2cd11ea6c4b8d125ccffcf89e Mon Sep 17 00:00:00 2001
From: Pavel Labath <pavel at labath.sk>
Date: Thu, 13 Feb 2025 14:08:25 +0100
Subject: [PATCH] [lldb] Fix
 RangeDataVector::CombineConsecutiveEntriesWithEqualData

Function was merging equal data even if they weren't adjecant. This
caused a problem in command-disassemble.s test because the two ranges
describing the function would be merged and "swallow" the function
between them.

This PR copies/adapts the algorithm from
RangeVector::CombineConsecutiveEntries (which does not have the same
problem) and also adds a call to ComputeUpperBounds as moving entries
around invalidates the binary tree. (The lack of this call wasn't
noticed until now either because we were not calling methods which rely
on upper bounds (right now, it's only the ill-named FindEntryIndexes
method), or because we weren't merging anything.
---
 lldb/include/lldb/Utility/RangeMap.h          | 47 ++++++++-----------
 .../test/Shell/Commands/command-disassemble.s |  3 +-
 lldb/unittests/Utility/RangeMapTest.cpp       | 21 +++++++++
 3 files changed, 41 insertions(+), 30 deletions(-)

diff --git a/lldb/include/lldb/Utility/RangeMap.h b/lldb/include/lldb/Utility/RangeMap.h
index 433466eebced8..2bebe74cc4cfe 100644
--- a/lldb/include/lldb/Utility/RangeMap.h
+++ b/lldb/include/lldb/Utility/RangeMap.h
@@ -493,36 +493,27 @@ class RangeDataVector {
 #ifdef ASSERT_RANGEMAP_ARE_SORTED
     assert(IsSorted());
 #endif
-    typename Collection::iterator pos;
-    typename Collection::iterator end;
-    typename Collection::iterator prev;
-    bool can_combine = false;
-    // First we determine if we can combine any of the Entry objects so we
-    // don't end up allocating and making a new collection for no reason
-    for (pos = m_entries.begin(), end = m_entries.end(), prev = end; pos != end;
-         prev = pos++) {
-      if (prev != end && prev->data == pos->data) {
-        can_combine = true;
-        break;
-      }
-    }
+    auto first_intersect = std::adjacent_find(
+        m_entries.begin(), m_entries.end(), [](const Entry &a, const Entry &b) {
+          return a.DoesAdjoinOrIntersect(b) && a.data == b.data;
+        });
+    if (first_intersect == m_entries.end())
+      return;
 
-    // We can combine at least one entry, then we make a new collection and
-    // populate it accordingly, and then swap it into place.
-    if (can_combine) {
-      Collection minimal_ranges;
-      for (pos = m_entries.begin(), end = m_entries.end(), prev = end;
-           pos != end; prev = pos++) {
-        if (prev != end && prev->data == pos->data)
-          minimal_ranges.back().SetRangeEnd(pos->GetRangeEnd());
-        else
-          minimal_ranges.push_back(*pos);
-      }
-      // Use the swap technique in case our new vector is much smaller. We must
-      // swap when using the STL because std::vector objects never release or
-      // reduce the memory once it has been allocated/reserved.
-      m_entries.swap(minimal_ranges);
+    // We can combine at least one entry. Make a new collection and populate it
+    // accordingly, and then swap it into place.
+    auto pos = std::next(first_intersect);
+    Collection minimal_ranges(m_entries.begin(), pos);
+    for (; pos != m_entries.end(); ++pos) {
+      Entry &back = minimal_ranges.back();
+      if (back.DoesAdjoinOrIntersect(*pos) && back.data == pos->data)
+        back.SetRangeEnd(std::max(back.GetRangeEnd(), pos->GetRangeEnd()));
+      else
+        minimal_ranges.push_back(*pos);
     }
+    m_entries.swap(minimal_ranges);
+    if (!m_entries.empty())
+      ComputeUpperBounds(0, m_entries.size());
   }
 
   void Clear() { m_entries.clear(); }
diff --git a/lldb/test/Shell/Commands/command-disassemble.s b/lldb/test/Shell/Commands/command-disassemble.s
index eb84a9ce39d4a..14f416d221231 100644
--- a/lldb/test/Shell/Commands/command-disassemble.s
+++ b/lldb/test/Shell/Commands/command-disassemble.s
@@ -94,8 +94,7 @@
 # CHECK-EMPTY:
 # CHECK-NEXT: command-disassemble.s.tmp`n2::case3:
 # CHECK-NEXT: command-disassemble.s.tmp[0x9046] <+0>: jmp 0x6046 ; <-12288>
-## FIXME: This should resolve to `middle_of_case3`
-# CHECK-NEXT: command-disassemble.s.tmp[0x904b] <+5>: jmp 0x7046 ; n2::case3 - 8192
+# CHECK-NEXT: command-disassemble.s.tmp[0x904b] <+5>: jmp 0x7046 ; middle_of_case3
 # CHECK-NEXT: command-disassemble.s.tmp[0x9050] <+10>: int    $0x2a
 # CHECK-EMPTY:
 # CHECK-NEXT: command-disassemble.s.tmp`n1::case3:
diff --git a/lldb/unittests/Utility/RangeMapTest.cpp b/lldb/unittests/Utility/RangeMapTest.cpp
index 981fa2a7d1c34..337e74a0e3d8c 100644
--- a/lldb/unittests/Utility/RangeMapTest.cpp
+++ b/lldb/unittests/Utility/RangeMapTest.cpp
@@ -238,3 +238,24 @@ TEST(RangeDataVector, FindEntryIndexesThatContain_Overlap) {
   EXPECT_THAT(FindEntryIndexes(39, Map), testing::ElementsAre(10));
   EXPECT_THAT(FindEntryIndexes(40, Map), testing::ElementsAre());
 }
+
+TEST(RangeDataVector, CombineConsecutiveEntriesWithEqualData) {
+  RangeDataVectorT Map;
+  Map.Append(EntryT(0, 10, 10));
+  Map.Append(EntryT(10, 10, 10));
+  Map.Sort();
+  Map.CombineConsecutiveEntriesWithEqualData();
+  EXPECT_THAT(FindEntryIndexes(5, Map), testing::ElementsAre(10));
+  EXPECT_THAT(FindEntryIndexes(15, Map), testing::ElementsAre(10));
+  EXPECT_THAT(FindEntryIndexes(25, Map), testing::ElementsAre());
+
+  Map.Clear();
+  Map.Append(EntryT(0, 10, 10));
+  Map.Append(EntryT(20, 10, 10));
+  Map.Sort();
+  Map.CombineConsecutiveEntriesWithEqualData();
+  EXPECT_THAT(FindEntryIndexes(5, Map), testing::ElementsAre(10));
+  EXPECT_THAT(FindEntryIndexes(15, Map), testing::ElementsAre());
+  EXPECT_THAT(FindEntryIndexes(25, Map), testing::ElementsAre(10));
+  EXPECT_THAT(FindEntryIndexes(35, Map), testing::ElementsAre());
+}