Skip to content

Commit 064801d

Browse files
labathlabath
committed
[lldb] s/ValidRange/ValidRanges in UnwindPlan
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 861efd4 commit 064801d

File tree

10 files changed

+138
-76
lines changed

10 files changed

+138
-76
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),
@@ -496,10 +496,8 @@ class UnwindPlan {
496496
// This UnwindPlan may not be valid at every address of the function span.
497497
// For instance, a FastUnwindPlan will not be valid at the prologue setup
498498
// instructions - only in the body of the function.
499-
void SetPlanValidAddressRange(const AddressRange &range);
500-
501-
const AddressRange &GetAddressRange() const {
502-
return m_plan_valid_address_range;
499+
void SetPlanValidAddressRanges(std::vector<AddressRange> ranges) {
500+
m_plan_valid_ranges = std::move(ranges);
503501
}
504502

505503
bool PlanValidAtAddress(Address addr);
@@ -548,11 +546,11 @@ class UnwindPlan {
548546
m_plan_is_for_signal_trap = is_for_signal_trap;
549547
}
550548

551-
int GetRowCount() const;
549+
int GetRowCount() const { return m_row_list.size(); }
552550

553551
void Clear() {
554552
m_row_list.clear();
555-
m_plan_valid_address_range.Clear();
553+
m_plan_valid_ranges.clear();
556554
m_register_kind = lldb::eRegisterKindDWARF;
557555
m_source_name.Clear();
558556
m_plan_is_sourced_from_compiler = eLazyBoolCalculate;
@@ -575,9 +573,8 @@ class UnwindPlan {
575573
}
576574

577575
private:
578-
typedef std::vector<RowSP> collection;
579-
collection m_row_list;
580-
AddressRange m_plan_valid_address_range;
576+
std::vector<RowSP> m_row_list;
577+
std::vector<AddressRange> m_plan_valid_ranges;
581578
lldb::RegisterKind m_register_kind; // The RegisterKind these register numbers
582579
// are in terms of - will need to be
583580
// 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 & 51 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>
@@ -397,36 +398,35 @@ void UnwindPlan::AppendRow(const UnwindPlan::RowSP &row_sp) {
397398
m_row_list.back() = row_sp;
398399
}
399400

401+
struct RowLess {
402+
bool operator()(addr_t a, const UnwindPlan::RowSP &b) const {
403+
return a < b->GetOffset();
404+
}
405+
bool operator()(const UnwindPlan::RowSP &a, addr_t b) const {
406+
return a->GetOffset() < b;
407+
}
408+
};
409+
400410
void UnwindPlan::InsertRow(const UnwindPlan::RowSP &row_sp,
401411
bool replace_existing) {
402-
collection::iterator it = m_row_list.begin();
403-
while (it != m_row_list.end()) {
404-
RowSP row = *it;
405-
if (row->GetOffset() >= row_sp->GetOffset())
406-
break;
407-
it++;
408-
}
409-
if (it == m_row_list.end() || (*it)->GetOffset() != row_sp->GetOffset())
412+
auto it = llvm::lower_bound(m_row_list, row_sp->GetOffset(), RowLess());
413+
if (it == m_row_list.end() || it->get()->GetOffset() > row_sp->GetOffset())
410414
m_row_list.insert(it, row_sp);
411-
else if (replace_existing)
412-
*it = row_sp;
415+
else {
416+
assert(it->get()->GetOffset() == row_sp->GetOffset());
417+
if (replace_existing)
418+
*it = row_sp;
419+
}
413420
}
414421

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

432432
bool UnwindPlan::IsValidRowIndex(uint32_t idx) const {
@@ -445,20 +445,13 @@ const UnwindPlan::Row *UnwindPlan::GetRowAtIndex(uint32_t idx) const {
445445

446446
const UnwindPlan::Row *UnwindPlan::GetLastRow() const {
447447
if (m_row_list.empty()) {
448-
Log *log = GetLog(LLDBLog::Unwind);
449-
LLDB_LOGF(log, "UnwindPlan::GetLastRow() when rows are empty");
448+
LLDB_LOG(GetLog(LLDBLog::Unwind),
449+
"UnwindPlan::GetLastRow() when rows are empty");
450450
return nullptr;
451451
}
452452
return m_row_list.back().get();
453453
}
454454

455-
int UnwindPlan::GetRowCount() const { return m_row_list.size(); }
456-
457-
void UnwindPlan::SetPlanValidAddressRange(const AddressRange &range) {
458-
if (range.GetBaseAddress().IsValid() && range.GetByteSize() != 0)
459-
m_plan_valid_address_range = range;
460-
}
461-
462455
bool UnwindPlan::PlanValidAtAddress(Address addr) {
463456
// If this UnwindPlan has no rows, it is an invalid UnwindPlan.
464457
if (GetRowCount() == 0) {
@@ -482,9 +475,9 @@ bool UnwindPlan::PlanValidAtAddress(Address addr) {
482475
// If the 0th Row of unwind instructions is missing, or if it doesn't provide
483476
// a register to use to find the Canonical Frame Address, this is not a valid
484477
// UnwindPlan.
485-
if (GetRowAtIndex(0) == nullptr ||
486-
GetRowAtIndex(0)->GetCFAValue().GetValueType() ==
487-
Row::FAValue::unspecified) {
478+
const Row *row0 = GetRowForFunctionOffset(0);
479+
if (!row0 ||
480+
row0->GetCFAValue().GetValueType() == Row::FAValue::unspecified) {
488481
Log *log = GetLog(LLDBLog::Unwind);
489482
if (log) {
490483
StreamString s;
@@ -503,17 +496,15 @@ bool UnwindPlan::PlanValidAtAddress(Address addr) {
503496
return false;
504497
}
505498

506-
if (!m_plan_valid_address_range.GetBaseAddress().IsValid() ||
507-
m_plan_valid_address_range.GetByteSize() == 0)
499+
if (m_plan_valid_ranges.empty())
508500
return true;
509501

510502
if (!addr.IsValid())
511503
return true;
512504

513-
if (m_plan_valid_address_range.ContainsFileAddress(addr))
514-
return true;
515-
516-
return false;
505+
return llvm::any_of(m_plan_valid_ranges, [&](const AddressRange &range) {
506+
return range.ContainsFileAddress(addr);
507+
});
517508
}
518509

519510
void UnwindPlan::Dump(Stream &s, Thread *thread, lldb::addr_t base_addr) const {
@@ -570,20 +561,17 @@ void UnwindPlan::Dump(Stream &s, Thread *thread, lldb::addr_t base_addr) const {
570561
s.Printf("not specified.\n");
571562
break;
572563
}
573-
if (m_plan_valid_address_range.GetBaseAddress().IsValid() &&
574-
m_plan_valid_address_range.GetByteSize() > 0) {
564+
if (!m_plan_valid_ranges.empty()) {
575565
s.PutCString("Address range of this UnwindPlan: ");
576566
TargetSP target_sp(thread->CalculateTarget());
577-
m_plan_valid_address_range.Dump(&s, target_sp.get(),
578-
Address::DumpStyleSectionNameOffset);
567+
for (const AddressRange &range : m_plan_valid_ranges)
568+
range.Dump(&s, target_sp.get(), Address::DumpStyleSectionNameOffset);
579569
s.EOL();
580570
}
581-
collection::const_iterator pos, begin = m_row_list.begin(),
582-
end = m_row_list.end();
583-
for (pos = begin; pos != end; ++pos) {
584-
s.Printf("row[%u]: ", (uint32_t)std::distance(begin, pos));
585-
(*pos)->Dump(s, this, thread, base_addr);
586-
s.Printf("\n");
571+
for (const auto &[index, row_sp] : llvm::enumerate(m_row_list)) {
572+
s.Format("row[{0}]: ", index);
573+
row_sp->Dump(s, this, thread, base_addr);
574+
s << "\n";
587575
}
588576
}
589577

‎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::RowSP make_simple_row(addr_t offset, uint64_t cfa_value) {
18+
UnwindPlan::RowSP row_sp = std::make_shared<UnwindPlan::Row>();
19+
row_sp->SetOffset(offset);
20+
row_sp->GetCFAValue().SetIsConstant(cfa_value);
21+
return row_sp;
22+
}
23+
24+
TEST(UnwindPlan, InsertRow) {
25+
UnwindPlan::RowSP row1_sp = make_simple_row(0, 42);
26+
UnwindPlan::RowSP row2_sp = make_simple_row(0, 47);
27+
28+
UnwindPlan plan(eRegisterKindGeneric);
29+
plan.InsertRow(row1_sp);
30+
EXPECT_THAT(plan.GetRowForFunctionOffset(0), testing::Pointee(*row1_sp));
31+
32+
plan.InsertRow(row2_sp, /*replace_existing=*/false);
33+
EXPECT_THAT(plan.GetRowForFunctionOffset(0), testing::Pointee(*row1_sp));
34+
35+
plan.InsertRow(row2_sp, /*replace_existing=*/true);
36+
EXPECT_THAT(plan.GetRowForFunctionOffset(0), testing::Pointee(*row2_sp));
37+
}
38+
39+
TEST(UnwindPlan, GetRowForFunctionOffset) {
40+
UnwindPlan::RowSP row1_sp = make_simple_row(10, 42);
41+
UnwindPlan::RowSP row2_sp = make_simple_row(20, 47);
42+
43+
UnwindPlan plan(eRegisterKindGeneric);
44+
plan.InsertRow(row1_sp);
45+
plan.InsertRow(row2_sp);
46+
47+
EXPECT_THAT(plan.GetRowForFunctionOffset(0), nullptr);
48+
EXPECT_THAT(plan.GetRowForFunctionOffset(9), nullptr);
49+
EXPECT_THAT(plan.GetRowForFunctionOffset(10), testing::Pointee(*row1_sp));
50+
EXPECT_THAT(plan.GetRowForFunctionOffset(19), testing::Pointee(*row1_sp));
51+
EXPECT_THAT(plan.GetRowForFunctionOffset(20), testing::Pointee(*row2_sp));
52+
EXPECT_THAT(plan.GetRowForFunctionOffset(99), testing::Pointee(*row2_sp));
53+
}
54+
55+
TEST(UnwindPlan, PlanValidAtAddress) {
56+
UnwindPlan::RowSP row1_sp = make_simple_row(0, 42);
57+
UnwindPlan::RowSP row2_sp = make_simple_row(10, 47);
58+
59+
UnwindPlan plan(eRegisterKindGeneric);
60+
EXPECT_FALSE(plan.PlanValidAtAddress(Address(0)));
61+
62+
plan.InsertRow(row2_sp);
63+
EXPECT_FALSE(plan.PlanValidAtAddress(Address(0)));
64+
65+
plan.InsertRow(row1_sp);
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)