Skip to content

[lldb] Fix RangeDataVector::CombineConsecutiveEntriesWithEqualData #127059

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Feb 20, 2025
Merged

[lldb] Fix RangeDataVector::CombineConsecutiveEntriesWithEqualData #127059

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
450371c
[lldb] Fix RangeDataVector::CombineConsecutiveEntriesWithEqualData
labath Feb 13, 2025
a698b19
add empty line, remove check
labath Feb 19, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 19 additions & 28 deletions lldb/include/lldb/Utility/RangeMap.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Contributor

@Jlalond Jlalond Feb 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the equal check cheaper than the Intersect or Adjacency check? If so we should short circuit on equality before checking for an intersection

Copy link
Collaborator Author

@labath labath Feb 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both of them are integer comparisons (in practice -- technically, this is a template so it could be whatever), so it really comes down to "which one is more likely to be false". I'm not sure about that, but I doubt this code is hot enough for it to matter. If we wanted to optimize this we could change the DoesAdjoinOrIntersect call to b.GetRangeBase() <= a.GetRangeEnd() (since the other check inside that function is guaranteed to be true due to sorting).

});
Copy link
Contributor

@Jlalond Jlalond Feb 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: an empty line before the if is in my opinion nicer to read

Copy link
Collaborator Author

@labath labath Feb 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

works for me.

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())
Copy link
Contributor

@Jlalond Jlalond Feb 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible for this to be empty? We would have at least one entry to pass the intersection check, and I don't see how we would empty the collection via merging ranges

Copy link
Collaborator Author

@labath labath Feb 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. I am usually all about deleting these.

I was actually thinking about follow this up with a patch to move the check inside the function, as it's currently repeated at all call sites (including the recursive ones).

ComputeUpperBounds(0, m_entries.size());
}

void Clear() { m_entries.clear(); }
Expand Down
3 changes: 1 addition & 2 deletions lldb/test/Shell/Commands/command-disassemble.s
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
21 changes: 21 additions & 0 deletions lldb/unittests/Utility/RangeMapTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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, 47));
Map.Append(EntryT(10, 10, 47));
Map.Sort();
Map.CombineConsecutiveEntriesWithEqualData();
EXPECT_THAT(FindEntryIndexes(5, Map), testing::ElementsAre(47));
EXPECT_THAT(FindEntryIndexes(15, Map), testing::ElementsAre(47));
EXPECT_THAT(FindEntryIndexes(25, Map), testing::ElementsAre());

Map.Clear();
Map.Append(EntryT(0, 10, 47));
Map.Append(EntryT(20, 10, 47));
Map.Sort();
Map.CombineConsecutiveEntriesWithEqualData();
EXPECT_THAT(FindEntryIndexes(5, Map), testing::ElementsAre(47));
EXPECT_THAT(FindEntryIndexes(15, Map), testing::ElementsAre());
EXPECT_THAT(FindEntryIndexes(25, Map), testing::ElementsAre(47));
EXPECT_THAT(FindEntryIndexes(35, Map), testing::ElementsAre());
}