Skip to content

Commit 387f3e8

Browse files
labathlabath
authored
[lldb] s/ValidRange/ValidRanges in UnwindPlan (#127661)
To be able to describe discontinuous functions, this patch changes the UnwindPlan to accept more than one address range. I've also squeezed in a couple improvements/modernizations, for example using the lower_bound function instead of a linear scan.
1 parent 5488ad8 commit 387f3e8

File tree

10 files changed

+138
-75
lines changed

10 files changed

+138
-75
lines changed

‎lldb/include/lldb/Symbol/UnwindPlan.h

Lines changed: 7 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -438,7 +438,7 @@ class UnwindPlan {
438438

439439
// Performs a deep copy of the plan, including all the rows (expensive).
440440
UnwindPlan(const UnwindPlan &rhs)
441-
: m_plan_valid_address_range(rhs.m_plan_valid_address_range),
441+
: m_plan_valid_ranges(rhs.m_plan_valid_ranges),
442442
m_register_kind(rhs.m_register_kind),
443443
m_return_addr_register(rhs.m_return_addr_register),
444444
m_source_name(rhs.m_source_name),
@@ -492,10 +492,8 @@ class UnwindPlan {
492492
// This UnwindPlan may not be valid at every address of the function span.
493493
// For instance, a FastUnwindPlan will not be valid at the prologue setup
494494
// instructions - only in the body of the function.
495-
void SetPlanValidAddressRange(const AddressRange &range);
496-
497-
const AddressRange &GetAddressRange() const {
498-
return m_plan_valid_address_range;
495+
void SetPlanValidAddressRanges(std::vector<AddressRange> ranges) {
496+
m_plan_valid_ranges = std::move(ranges);
499497
}
500498

501499
bool PlanValidAtAddress(Address addr);
@@ -544,11 +542,11 @@ class UnwindPlan {
544542
m_plan_is_for_signal_trap = is_for_signal_trap;
545543
}
546544

547-
int GetRowCount() const;
545+
int GetRowCount() const { return m_row_list.size(); }
548546

549547
void Clear() {
550548
m_row_list.clear();
551-
m_plan_valid_address_range.Clear();
549+
m_plan_valid_ranges.clear();
552550
m_register_kind = lldb::eRegisterKindDWARF;
553551
m_source_name.Clear();
554552
m_plan_is_sourced_from_compiler = eLazyBoolCalculate;
@@ -571,9 +569,8 @@ class UnwindPlan {
571569
}
572570

573571
private:
574-
typedef std::vector<RowSP> collection;
575-
collection m_row_list;
576-
AddressRange m_plan_valid_address_range;
572+
std::vector<RowSP> m_row_list;
573+
std::vector<AddressRange> m_plan_valid_ranges;
577574
lldb::RegisterKind m_register_kind; // The RegisterKind these register numbers
578575
// are in terms of - will need to be
579576
// translated to lldb native reg nums at unwind time

‎lldb/source/Plugins/ObjectFile/PECOFF/PECallFrameInfo.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -495,9 +495,9 @@ bool PECallFrameInfo::GetUnwindPlan(const AddressRange &range,
495495
for (auto it = rows.rbegin(); it != rows.rend(); ++it)
496496
unwind_plan.AppendRow(std::move(*it));
497497

498-
unwind_plan.SetPlanValidAddressRange(AddressRange(
498+
unwind_plan.SetPlanValidAddressRanges({AddressRange(
499499
m_object_file.GetAddress(runtime_function->StartAddress),
500-
runtime_function->EndAddress - runtime_function->StartAddress));
500+
runtime_function->EndAddress - runtime_function->StartAddress)});
501501
unwind_plan.SetUnwindPlanValidAtAllInstructions(eLazyBoolNo);
502502

503503
return true;

‎lldb/source/Plugins/SymbolFile/Breakpad/SymbolFileBreakpad.cpp

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -656,9 +656,9 @@ SymbolFileBreakpad::ParseCFIUnwindPlan(const Bookmark &bookmark,
656656
plan_sp->SetUnwindPlanValidAtAllInstructions(eLazyBoolNo);
657657
plan_sp->SetUnwindPlanForSignalTrap(eLazyBoolNo);
658658
plan_sp->SetSourcedFromCompiler(eLazyBoolYes);
659-
plan_sp->SetPlanValidAddressRange(
660-
AddressRange(base + init_record->Address, *init_record->Size,
661-
m_objfile_sp->GetModule()->GetSectionList()));
659+
plan_sp->SetPlanValidAddressRanges(
660+
{AddressRange(base + init_record->Address, *init_record->Size,
661+
m_objfile_sp->GetModule()->GetSectionList())});
662662

663663
UnwindPlan::Row row;
664664
if (!ParseCFIUnwindRow(init_record->UnwindRules, resolver, row))
@@ -696,9 +696,9 @@ SymbolFileBreakpad::ParseWinUnwindPlan(const Bookmark &bookmark,
696696
plan_sp->SetUnwindPlanValidAtAllInstructions(eLazyBoolNo);
697697
plan_sp->SetUnwindPlanForSignalTrap(eLazyBoolNo);
698698
plan_sp->SetSourcedFromCompiler(eLazyBoolYes);
699-
plan_sp->SetPlanValidAddressRange(
700-
AddressRange(base + record->RVA, record->CodeSize,
701-
m_objfile_sp->GetModule()->GetSectionList()));
699+
plan_sp->SetPlanValidAddressRanges(
700+
{AddressRange(base + record->RVA, record->CodeSize,
701+
m_objfile_sp->GetModule()->GetSectionList())});
702702

703703
UnwindPlan::Row row;
704704

‎lldb/source/Plugins/UnwindAssembly/x86/x86AssemblyInspectionEngine.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -918,7 +918,7 @@ bool x86AssemblyInspectionEngine::GetNonCallSiteUnwindPlanFromAssembly(
918918
UnwindPlan::Row::AbstractRegisterLocation initial_regloc;
919919
UnwindPlan::Row row;
920920

921-
unwind_plan.SetPlanValidAddressRange(func_range);
921+
unwind_plan.SetPlanValidAddressRanges({func_range});
922922
unwind_plan.SetRegisterKind(eRegisterKindLLDB);
923923

924924
// At the start of the function, find the CFA by adding wordsize to the SP
@@ -1538,7 +1538,7 @@ bool x86AssemblyInspectionEngine::AugmentUnwindPlanFromCallSite(
15381538
}
15391539
}
15401540

1541-
unwind_plan.SetPlanValidAddressRange(func_range);
1541+
unwind_plan.SetPlanValidAddressRanges({func_range});
15421542
if (unwind_plan_updated) {
15431543
std::string unwind_plan_source(unwind_plan.GetSourceName().AsCString());
15441544
unwind_plan_source += " plus augmentation from assembly parsing";

‎lldb/source/Symbol/CompactUnwindInfo.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -206,7 +206,7 @@ bool CompactUnwindInfo::GetUnwindPlan(Target &target, Address addr,
206206
function_info.valid_range_offset_end -
207207
function_info.valid_range_offset_start,
208208
sl);
209-
unwind_plan.SetPlanValidAddressRange(func_range);
209+
unwind_plan.SetPlanValidAddressRanges({func_range});
210210
}
211211
}
212212

‎lldb/source/Symbol/DWARFCallFrameInfo.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -623,7 +623,7 @@ bool DWARFCallFrameInfo::FDEToUnwindPlan(dw_offset_t dwarf_offset,
623623
uint32_t code_align = cie->code_align;
624624
int32_t data_align = cie->data_align;
625625

626-
unwind_plan.SetPlanValidAddressRange(range);
626+
unwind_plan.SetPlanValidAddressRanges({range});
627627
UnwindPlan::Row row = cie->initial_row;
628628

629629
unwind_plan.SetRegisterKind(GetRegisterKind());

‎lldb/source/Symbol/UnwindPlan.cpp

Lines changed: 39 additions & 50 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515
#include "lldb/Utility/ConstString.h"
1616
#include "lldb/Utility/LLDBLog.h"
1717
#include "lldb/Utility/Log.h"
18+
#include "llvm/ADT/STLExtras.h"
1819
#include "llvm/DebugInfo/DIContext.h"
1920
#include "llvm/DebugInfo/DWARF/DWARFExpression.h"
2021
#include <optional>
@@ -396,34 +397,34 @@ void UnwindPlan::AppendRow(Row row) {
396397
*m_row_list.back() = std::move(row);
397398
}
398399

399-
void UnwindPlan::InsertRow(Row row, bool replace_existing) {
400-
collection::iterator it = m_row_list.begin();
401-
while (it != m_row_list.end()) {
402-
if ((*it)->GetOffset() >= row.GetOffset())
403-
break;
404-
it++;
400+
struct RowLess {
401+
bool operator()(addr_t a, const UnwindPlan::RowSP &b) const {
402+
return a < b->GetOffset();
405403
}
406-
if (it == m_row_list.end() || (*it)->GetOffset() != row.GetOffset())
404+
bool operator()(const UnwindPlan::RowSP &a, addr_t b) const {
405+
return a->GetOffset() < b;
406+
}
407+
};
408+
409+
void UnwindPlan::InsertRow(Row row, bool replace_existing) {
410+
auto it = llvm::lower_bound(m_row_list, row.GetOffset(), RowLess());
411+
if (it == m_row_list.end() || it->get()->GetOffset() > row.GetOffset())
407412
m_row_list.insert(it, std::make_shared<Row>(std::move(row)));
408-
else if (replace_existing)
409-
**it = std::move(row);
413+
else {
414+
assert(it->get()->GetOffset() == row.GetOffset());
415+
if (replace_existing)
416+
**it = std::move(row);
417+
}
410418
}
411419

412420
const UnwindPlan::Row *UnwindPlan::GetRowForFunctionOffset(int offset) const {
413-
if (m_row_list.empty())
421+
auto it = offset == -1 ? m_row_list.end()
422+
: llvm::upper_bound(m_row_list, offset, RowLess());
423+
if (it == m_row_list.begin())
414424
return nullptr;
415-
if (offset == -1)
416-
return m_row_list.back().get();
417-
418-
RowSP row;
419-
collection::const_iterator pos, end = m_row_list.end();
420-
for (pos = m_row_list.begin(); pos != end; ++pos) {
421-
if ((*pos)->GetOffset() <= static_cast<lldb::offset_t>(offset))
422-
row = *pos;
423-
else
424-
break;
425-
}
426-
return row.get();
425+
// upper_bound returns the row strictly greater than our desired offset, which
426+
// means that the row before it is a match.
427+
return std::prev(it)->get();
427428
}
428429

429430
bool UnwindPlan::IsValidRowIndex(uint32_t idx) const {
@@ -442,20 +443,13 @@ const UnwindPlan::Row *UnwindPlan::GetRowAtIndex(uint32_t idx) const {
442443

443444
const UnwindPlan::Row *UnwindPlan::GetLastRow() const {
444445
if (m_row_list.empty()) {
445-
Log *log = GetLog(LLDBLog::Unwind);
446-
LLDB_LOGF(log, "UnwindPlan::GetLastRow() when rows are empty");
446+
LLDB_LOG(GetLog(LLDBLog::Unwind),
447+
"UnwindPlan::GetLastRow() when rows are empty");
447448
return nullptr;
448449
}
449450
return m_row_list.back().get();
450451
}
451452

452-
int UnwindPlan::GetRowCount() const { return m_row_list.size(); }
453-
454-
void UnwindPlan::SetPlanValidAddressRange(const AddressRange &range) {
455-
if (range.GetBaseAddress().IsValid() && range.GetByteSize() != 0)
456-
m_plan_valid_address_range = range;
457-
}
458-
459453
bool UnwindPlan::PlanValidAtAddress(Address addr) {
460454
// If this UnwindPlan has no rows, it is an invalid UnwindPlan.
461455
if (GetRowCount() == 0) {
@@ -479,9 +473,9 @@ bool UnwindPlan::PlanValidAtAddress(Address addr) {
479473
// If the 0th Row of unwind instructions is missing, or if it doesn't provide
480474
// a register to use to find the Canonical Frame Address, this is not a valid
481475
// UnwindPlan.
482-
if (GetRowAtIndex(0) == nullptr ||
483-
GetRowAtIndex(0)->GetCFAValue().GetValueType() ==
484-
Row::FAValue::unspecified) {
476+
const Row *row0 = GetRowForFunctionOffset(0);
477+
if (!row0 ||
478+
row0->GetCFAValue().GetValueType() == Row::FAValue::unspecified) {
485479
Log *log = GetLog(LLDBLog::Unwind);
486480
if (log) {
487481
StreamString s;
@@ -500,17 +494,15 @@ bool UnwindPlan::PlanValidAtAddress(Address addr) {
500494
return false;
501495
}
502496

503-
if (!m_plan_valid_address_range.GetBaseAddress().IsValid() ||
504-
m_plan_valid_address_range.GetByteSize() == 0)
497+
if (m_plan_valid_ranges.empty())
505498
return true;
506499

507500
if (!addr.IsValid())
508501
return true;
509502

510-
if (m_plan_valid_address_range.ContainsFileAddress(addr))
511-
return true;
512-
513-
return false;
503+
return llvm::any_of(m_plan_valid_ranges, [&](const AddressRange &range) {
504+
return range.ContainsFileAddress(addr);
505+
});
514506
}
515507

516508
void UnwindPlan::Dump(Stream &s, Thread *thread, lldb::addr_t base_addr) const {
@@ -567,20 +559,17 @@ void UnwindPlan::Dump(Stream &s, Thread *thread, lldb::addr_t base_addr) const {
567559
s.Printf("not specified.\n");
568560
break;
569561
}
570-
if (m_plan_valid_address_range.GetBaseAddress().IsValid() &&
571-
m_plan_valid_address_range.GetByteSize() > 0) {
562+
if (!m_plan_valid_ranges.empty()) {
572563
s.PutCString("Address range of this UnwindPlan: ");
573564
TargetSP target_sp(thread->CalculateTarget());
574-
m_plan_valid_address_range.Dump(&s, target_sp.get(),
575-
Address::DumpStyleSectionNameOffset);
565+
for (const AddressRange &range : m_plan_valid_ranges)
566+
range.Dump(&s, target_sp.get(), Address::DumpStyleSectionNameOffset);
576567
s.EOL();
577568
}
578-
collection::const_iterator pos, begin = m_row_list.begin(),
579-
end = m_row_list.end();
580-
for (pos = begin; pos != end; ++pos) {
581-
s.Printf("row[%u]: ", (uint32_t)std::distance(begin, pos));
582-
(*pos)->Dump(s, this, thread, base_addr);
583-
s.Printf("\n");
569+
for (const auto &[index, row_sp] : llvm::enumerate(m_row_list)) {
570+
s.Format("row[{0}]: ", index);
571+
row_sp->Dump(s, this, thread, base_addr);
572+
s << "\n";
584573
}
585574
}
586575

‎lldb/unittests/Symbol/CMakeLists.txt

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ add_lldb_unittest(SymbolTests
1212
TestDWARFCallFrameInfo.cpp
1313
TestType.cpp
1414
TestLineEntry.cpp
15+
UnwindPlanTest.cpp
1516

1617
LINK_LIBS
1718
lldbCore

‎lldb/unittests/Symbol/UnwindPlanTest.cpp

Lines changed: 76 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,76 @@
1+
//===-- UnwindPlanTest.cpp ------------------------------------------------===//
2+
//
3+
//
4+
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
5+
// See https://llvm.org/LICENSE.txt for license information.
6+
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
7+
//
8+
//===----------------------------------------------------------------------===//
9+
10+
#include "lldb/Symbol/UnwindPlan.h"
11+
#include "gmock/gmock.h"
12+
#include "gtest/gtest.h"
13+
14+
using namespace lldb_private;
15+
using namespace lldb;
16+
17+
static UnwindPlan::Row make_simple_row(addr_t offset, uint64_t cfa_value) {
18+
UnwindPlan::Row row;
19+
row.SetOffset(offset);
20+
row.GetCFAValue().SetIsConstant(cfa_value);
21+
return row;
22+
}
23+
24+
TEST(UnwindPlan, InsertRow) {
25+
UnwindPlan::Row row1 = make_simple_row(0, 42);
26+
UnwindPlan::Row row2 = make_simple_row(0, 47);
27+
28+
UnwindPlan plan(eRegisterKindGeneric);
29+
plan.InsertRow(row1);
30+
EXPECT_THAT(plan.GetRowForFunctionOffset(0), testing::Pointee(row1));
31+
32+
plan.InsertRow(row2, /*replace_existing=*/false);
33+
EXPECT_THAT(plan.GetRowForFunctionOffset(0), testing::Pointee(row1));
34+
35+
plan.InsertRow(row2, /*replace_existing=*/true);
36+
EXPECT_THAT(plan.GetRowForFunctionOffset(0), testing::Pointee(row2));
37+
}
38+
39+
TEST(UnwindPlan, GetRowForFunctionOffset) {
40+
UnwindPlan::Row row1 = make_simple_row(10, 42);
41+
UnwindPlan::Row row2 = make_simple_row(20, 47);
42+
43+
UnwindPlan plan(eRegisterKindGeneric);
44+
plan.InsertRow(row1);
45+
plan.InsertRow(row2);
46+
47+
EXPECT_THAT(plan.GetRowForFunctionOffset(0), nullptr);
48+
EXPECT_THAT(plan.GetRowForFunctionOffset(9), nullptr);
49+
EXPECT_THAT(plan.GetRowForFunctionOffset(10), testing::Pointee(row1));
50+
EXPECT_THAT(plan.GetRowForFunctionOffset(19), testing::Pointee(row1));
51+
EXPECT_THAT(plan.GetRowForFunctionOffset(20), testing::Pointee(row2));
52+
EXPECT_THAT(plan.GetRowForFunctionOffset(99), testing::Pointee(row2));
53+
}
54+
55+
TEST(UnwindPlan, PlanValidAtAddress) {
56+
UnwindPlan::Row row1 = make_simple_row(0, 42);
57+
UnwindPlan::Row row2 = make_simple_row(10, 47);
58+
59+
UnwindPlan plan(eRegisterKindGeneric);
60+
EXPECT_FALSE(plan.PlanValidAtAddress(Address(0)));
61+
62+
plan.InsertRow(row2);
63+
EXPECT_FALSE(plan.PlanValidAtAddress(Address(0)));
64+
65+
plan.InsertRow(row1);
66+
EXPECT_TRUE(plan.PlanValidAtAddress(Address(0)));
67+
EXPECT_TRUE(plan.PlanValidAtAddress(Address(10)));
68+
69+
plan.SetPlanValidAddressRanges({AddressRange(0, 5), AddressRange(15, 5)});
70+
EXPECT_TRUE(plan.PlanValidAtAddress(Address(0)));
71+
EXPECT_FALSE(plan.PlanValidAtAddress(Address(5)));
72+
EXPECT_FALSE(plan.PlanValidAtAddress(Address(10)));
73+
EXPECT_TRUE(plan.PlanValidAtAddress(Address(15)));
74+
EXPECT_FALSE(plan.PlanValidAtAddress(Address(20)));
75+
EXPECT_FALSE(plan.PlanValidAtAddress(Address(25)));
76+
}

‎lldb/unittests/UnwindAssembly/x86/Testx86AssemblyInspectionEngine.cpp

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2247,7 +2247,7 @@ TEST_F(Testx86AssemblyInspectionEngine, TestSpArithx86_64Augmented) {
22472247
sample_range = AddressRange(0x1000, sizeof(data));
22482248

22492249
unwind_plan.SetSourceName("unit testing hand-created unwind plan");
2250-
unwind_plan.SetPlanValidAddressRange(sample_range);
2250+
unwind_plan.SetPlanValidAddressRanges({sample_range});
22512251
unwind_plan.SetRegisterKind(eRegisterKindLLDB);
22522252

22532253
{
@@ -2323,7 +2323,7 @@ TEST_F(Testx86AssemblyInspectionEngine, TestSimplex86_64Augmented) {
23232323
sample_range = AddressRange(0x1000, sizeof(data));
23242324

23252325
unwind_plan.SetSourceName("unit testing hand-created unwind plan");
2326-
unwind_plan.SetPlanValidAddressRange(sample_range);
2326+
unwind_plan.SetPlanValidAddressRanges({sample_range});
23272327
unwind_plan.SetRegisterKind(eRegisterKindLLDB);
23282328

23292329
{
@@ -2390,7 +2390,7 @@ TEST_F(Testx86AssemblyInspectionEngine, TestSimplei386ugmented) {
23902390
sample_range = AddressRange(0x1000, sizeof(data));
23912391

23922392
unwind_plan.SetSourceName("unit testing hand-created unwind plan");
2393-
unwind_plan.SetPlanValidAddressRange(sample_range);
2393+
unwind_plan.SetPlanValidAddressRanges({sample_range});
23942394
unwind_plan.SetRegisterKind(eRegisterKindLLDB);
23952395

23962396
{

0 commit comments

Comments
 (0)