Author: Pavel Labath Date: 2025-03-21T10:46:48+01:00 New Revision: 387f3e8f986d53067a68aa0d7b058a0ce81ec941
URL: https://github.com/llvm/llvm-project/commit/387f3e8f986d53067a68aa0d7b058a0ce81ec941 DIFF: https://github.com/llvm/llvm-project/commit/387f3e8f986d53067a68aa0d7b058a0ce81ec941.diff LOG: [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. Added: lldb/unittests/Symbol/UnwindPlanTest.cpp Modified: lldb/include/lldb/Symbol/UnwindPlan.h lldb/source/Plugins/ObjectFile/PECOFF/PECallFrameInfo.cpp lldb/source/Plugins/SymbolFile/Breakpad/SymbolFileBreakpad.cpp lldb/source/Plugins/UnwindAssembly/x86/x86AssemblyInspectionEngine.cpp lldb/source/Symbol/CompactUnwindInfo.cpp lldb/source/Symbol/DWARFCallFrameInfo.cpp lldb/source/Symbol/UnwindPlan.cpp lldb/unittests/Symbol/CMakeLists.txt lldb/unittests/UnwindAssembly/x86/Testx86AssemblyInspectionEngine.cpp Removed: ################################################################################ diff --git a/lldb/include/lldb/Symbol/UnwindPlan.h b/lldb/include/lldb/Symbol/UnwindPlan.h index 7c361bc08bcfe..db9aade93b6ba 100644 --- a/lldb/include/lldb/Symbol/UnwindPlan.h +++ b/lldb/include/lldb/Symbol/UnwindPlan.h @@ -438,7 +438,7 @@ class UnwindPlan { // Performs a deep copy of the plan, including all the rows (expensive). UnwindPlan(const UnwindPlan &rhs) - : m_plan_valid_address_range(rhs.m_plan_valid_address_range), + : m_plan_valid_ranges(rhs.m_plan_valid_ranges), m_register_kind(rhs.m_register_kind), m_return_addr_register(rhs.m_return_addr_register), m_source_name(rhs.m_source_name), @@ -492,10 +492,8 @@ class UnwindPlan { // This UnwindPlan may not be valid at every address of the function span. // For instance, a FastUnwindPlan will not be valid at the prologue setup // instructions - only in the body of the function. - void SetPlanValidAddressRange(const AddressRange &range); - - const AddressRange &GetAddressRange() const { - return m_plan_valid_address_range; + void SetPlanValidAddressRanges(std::vector<AddressRange> ranges) { + m_plan_valid_ranges = std::move(ranges); } bool PlanValidAtAddress(Address addr); @@ -544,11 +542,11 @@ class UnwindPlan { m_plan_is_for_signal_trap = is_for_signal_trap; } - int GetRowCount() const; + int GetRowCount() const { return m_row_list.size(); } void Clear() { m_row_list.clear(); - m_plan_valid_address_range.Clear(); + m_plan_valid_ranges.clear(); m_register_kind = lldb::eRegisterKindDWARF; m_source_name.Clear(); m_plan_is_sourced_from_compiler = eLazyBoolCalculate; @@ -571,9 +569,8 @@ class UnwindPlan { } private: - typedef std::vector<RowSP> collection; - collection m_row_list; - AddressRange m_plan_valid_address_range; + std::vector<RowSP> m_row_list; + std::vector<AddressRange> m_plan_valid_ranges; lldb::RegisterKind m_register_kind; // The RegisterKind these register numbers // are in terms of - will need to be // translated to lldb native reg nums at unwind time diff --git a/lldb/source/Plugins/ObjectFile/PECOFF/PECallFrameInfo.cpp b/lldb/source/Plugins/ObjectFile/PECOFF/PECallFrameInfo.cpp index 459abe417035e..a7c2e4f0b8dbc 100644 --- a/lldb/source/Plugins/ObjectFile/PECOFF/PECallFrameInfo.cpp +++ b/lldb/source/Plugins/ObjectFile/PECOFF/PECallFrameInfo.cpp @@ -495,9 +495,9 @@ bool PECallFrameInfo::GetUnwindPlan(const AddressRange &range, for (auto it = rows.rbegin(); it != rows.rend(); ++it) unwind_plan.AppendRow(std::move(*it)); - unwind_plan.SetPlanValidAddressRange(AddressRange( + unwind_plan.SetPlanValidAddressRanges({AddressRange( m_object_file.GetAddress(runtime_function->StartAddress), - runtime_function->EndAddress - runtime_function->StartAddress)); + runtime_function->EndAddress - runtime_function->StartAddress)}); unwind_plan.SetUnwindPlanValidAtAllInstructions(eLazyBoolNo); return true; diff --git a/lldb/source/Plugins/SymbolFile/Breakpad/SymbolFileBreakpad.cpp b/lldb/source/Plugins/SymbolFile/Breakpad/SymbolFileBreakpad.cpp index 222e04a6a3464..ce2ba69be2e96 100644 --- a/lldb/source/Plugins/SymbolFile/Breakpad/SymbolFileBreakpad.cpp +++ b/lldb/source/Plugins/SymbolFile/Breakpad/SymbolFileBreakpad.cpp @@ -656,9 +656,9 @@ SymbolFileBreakpad::ParseCFIUnwindPlan(const Bookmark &bookmark, plan_sp->SetUnwindPlanValidAtAllInstructions(eLazyBoolNo); plan_sp->SetUnwindPlanForSignalTrap(eLazyBoolNo); plan_sp->SetSourcedFromCompiler(eLazyBoolYes); - plan_sp->SetPlanValidAddressRange( - AddressRange(base + init_record->Address, *init_record->Size, - m_objfile_sp->GetModule()->GetSectionList())); + plan_sp->SetPlanValidAddressRanges( + {AddressRange(base + init_record->Address, *init_record->Size, + m_objfile_sp->GetModule()->GetSectionList())}); UnwindPlan::Row row; if (!ParseCFIUnwindRow(init_record->UnwindRules, resolver, row)) @@ -696,9 +696,9 @@ SymbolFileBreakpad::ParseWinUnwindPlan(const Bookmark &bookmark, plan_sp->SetUnwindPlanValidAtAllInstructions(eLazyBoolNo); plan_sp->SetUnwindPlanForSignalTrap(eLazyBoolNo); plan_sp->SetSourcedFromCompiler(eLazyBoolYes); - plan_sp->SetPlanValidAddressRange( - AddressRange(base + record->RVA, record->CodeSize, - m_objfile_sp->GetModule()->GetSectionList())); + plan_sp->SetPlanValidAddressRanges( + {AddressRange(base + record->RVA, record->CodeSize, + m_objfile_sp->GetModule()->GetSectionList())}); UnwindPlan::Row row; diff --git a/lldb/source/Plugins/UnwindAssembly/x86/x86AssemblyInspectionEngine.cpp b/lldb/source/Plugins/UnwindAssembly/x86/x86AssemblyInspectionEngine.cpp index 298111a496104..3eaa2f33fce3e 100644 --- a/lldb/source/Plugins/UnwindAssembly/x86/x86AssemblyInspectionEngine.cpp +++ b/lldb/source/Plugins/UnwindAssembly/x86/x86AssemblyInspectionEngine.cpp @@ -918,7 +918,7 @@ bool x86AssemblyInspectionEngine::GetNonCallSiteUnwindPlanFromAssembly( UnwindPlan::Row::AbstractRegisterLocation initial_regloc; UnwindPlan::Row row; - unwind_plan.SetPlanValidAddressRange(func_range); + unwind_plan.SetPlanValidAddressRanges({func_range}); unwind_plan.SetRegisterKind(eRegisterKindLLDB); // At the start of the function, find the CFA by adding wordsize to the SP @@ -1538,7 +1538,7 @@ bool x86AssemblyInspectionEngine::AugmentUnwindPlanFromCallSite( } } - unwind_plan.SetPlanValidAddressRange(func_range); + unwind_plan.SetPlanValidAddressRanges({func_range}); if (unwind_plan_updated) { std::string unwind_plan_source(unwind_plan.GetSourceName().AsCString()); unwind_plan_source += " plus augmentation from assembly parsing"; diff --git a/lldb/source/Symbol/CompactUnwindInfo.cpp b/lldb/source/Symbol/CompactUnwindInfo.cpp index 035b630740620..3c97d2ca11fbc 100644 --- a/lldb/source/Symbol/CompactUnwindInfo.cpp +++ b/lldb/source/Symbol/CompactUnwindInfo.cpp @@ -206,7 +206,7 @@ bool CompactUnwindInfo::GetUnwindPlan(Target &target, Address addr, function_info.valid_range_offset_end - function_info.valid_range_offset_start, sl); - unwind_plan.SetPlanValidAddressRange(func_range); + unwind_plan.SetPlanValidAddressRanges({func_range}); } } diff --git a/lldb/source/Symbol/DWARFCallFrameInfo.cpp b/lldb/source/Symbol/DWARFCallFrameInfo.cpp index 278d3a2c4a613..957818e8d077f 100644 --- a/lldb/source/Symbol/DWARFCallFrameInfo.cpp +++ b/lldb/source/Symbol/DWARFCallFrameInfo.cpp @@ -623,7 +623,7 @@ bool DWARFCallFrameInfo::FDEToUnwindPlan(dw_offset_t dwarf_offset, uint32_t code_align = cie->code_align; int32_t data_align = cie->data_align; - unwind_plan.SetPlanValidAddressRange(range); + unwind_plan.SetPlanValidAddressRanges({range}); UnwindPlan::Row row = cie->initial_row; unwind_plan.SetRegisterKind(GetRegisterKind()); diff --git a/lldb/source/Symbol/UnwindPlan.cpp b/lldb/source/Symbol/UnwindPlan.cpp index 4986802a4acdd..48089cbdecd97 100644 --- a/lldb/source/Symbol/UnwindPlan.cpp +++ b/lldb/source/Symbol/UnwindPlan.cpp @@ -15,6 +15,7 @@ #include "lldb/Utility/ConstString.h" #include "lldb/Utility/LLDBLog.h" #include "lldb/Utility/Log.h" +#include "llvm/ADT/STLExtras.h" #include "llvm/DebugInfo/DIContext.h" #include "llvm/DebugInfo/DWARF/DWARFExpression.h" #include <optional> @@ -396,34 +397,34 @@ void UnwindPlan::AppendRow(Row row) { *m_row_list.back() = std::move(row); } -void UnwindPlan::InsertRow(Row row, bool replace_existing) { - collection::iterator it = m_row_list.begin(); - while (it != m_row_list.end()) { - if ((*it)->GetOffset() >= row.GetOffset()) - break; - it++; +struct RowLess { + bool operator()(addr_t a, const UnwindPlan::RowSP &b) const { + return a < b->GetOffset(); } - if (it == m_row_list.end() || (*it)->GetOffset() != row.GetOffset()) + bool operator()(const UnwindPlan::RowSP &a, addr_t b) const { + return a->GetOffset() < b; + } +}; + +void UnwindPlan::InsertRow(Row row, bool replace_existing) { + auto it = llvm::lower_bound(m_row_list, row.GetOffset(), RowLess()); + if (it == m_row_list.end() || it->get()->GetOffset() > row.GetOffset()) m_row_list.insert(it, std::make_shared<Row>(std::move(row))); - else if (replace_existing) - **it = std::move(row); + else { + assert(it->get()->GetOffset() == row.GetOffset()); + if (replace_existing) + **it = std::move(row); + } } const UnwindPlan::Row *UnwindPlan::GetRowForFunctionOffset(int offset) const { - if (m_row_list.empty()) + auto it = offset == -1 ? m_row_list.end() + : llvm::upper_bound(m_row_list, offset, RowLess()); + if (it == m_row_list.begin()) return nullptr; - if (offset == -1) - return m_row_list.back().get(); - - RowSP row; - collection::const_iterator pos, end = m_row_list.end(); - for (pos = m_row_list.begin(); pos != end; ++pos) { - if ((*pos)->GetOffset() <= static_cast<lldb::offset_t>(offset)) - row = *pos; - else - break; - } - return row.get(); + // upper_bound returns the row strictly greater than our desired offset, which + // means that the row before it is a match. + return std::prev(it)->get(); } bool UnwindPlan::IsValidRowIndex(uint32_t idx) const { @@ -442,20 +443,13 @@ const UnwindPlan::Row *UnwindPlan::GetRowAtIndex(uint32_t idx) const { const UnwindPlan::Row *UnwindPlan::GetLastRow() const { if (m_row_list.empty()) { - Log *log = GetLog(LLDBLog::Unwind); - LLDB_LOGF(log, "UnwindPlan::GetLastRow() when rows are empty"); + LLDB_LOG(GetLog(LLDBLog::Unwind), + "UnwindPlan::GetLastRow() when rows are empty"); return nullptr; } return m_row_list.back().get(); } -int UnwindPlan::GetRowCount() const { return m_row_list.size(); } - -void UnwindPlan::SetPlanValidAddressRange(const AddressRange &range) { - if (range.GetBaseAddress().IsValid() && range.GetByteSize() != 0) - m_plan_valid_address_range = range; -} - bool UnwindPlan::PlanValidAtAddress(Address addr) { // If this UnwindPlan has no rows, it is an invalid UnwindPlan. if (GetRowCount() == 0) { @@ -479,9 +473,9 @@ bool UnwindPlan::PlanValidAtAddress(Address addr) { // If the 0th Row of unwind instructions is missing, or if it doesn't provide // a register to use to find the Canonical Frame Address, this is not a valid // UnwindPlan. - if (GetRowAtIndex(0) == nullptr || - GetRowAtIndex(0)->GetCFAValue().GetValueType() == - Row::FAValue::unspecified) { + const Row *row0 = GetRowForFunctionOffset(0); + if (!row0 || + row0->GetCFAValue().GetValueType() == Row::FAValue::unspecified) { Log *log = GetLog(LLDBLog::Unwind); if (log) { StreamString s; @@ -500,17 +494,15 @@ bool UnwindPlan::PlanValidAtAddress(Address addr) { return false; } - if (!m_plan_valid_address_range.GetBaseAddress().IsValid() || - m_plan_valid_address_range.GetByteSize() == 0) + if (m_plan_valid_ranges.empty()) return true; if (!addr.IsValid()) return true; - if (m_plan_valid_address_range.ContainsFileAddress(addr)) - return true; - - return false; + return llvm::any_of(m_plan_valid_ranges, [&](const AddressRange &range) { + return range.ContainsFileAddress(addr); + }); } 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 { s.Printf("not specified.\n"); break; } - if (m_plan_valid_address_range.GetBaseAddress().IsValid() && - m_plan_valid_address_range.GetByteSize() > 0) { + if (!m_plan_valid_ranges.empty()) { s.PutCString("Address range of this UnwindPlan: "); TargetSP target_sp(thread->CalculateTarget()); - m_plan_valid_address_range.Dump(&s, target_sp.get(), - Address::DumpStyleSectionNameOffset); + for (const AddressRange &range : m_plan_valid_ranges) + range.Dump(&s, target_sp.get(), Address::DumpStyleSectionNameOffset); s.EOL(); } - collection::const_iterator pos, begin = m_row_list.begin(), - end = m_row_list.end(); - for (pos = begin; pos != end; ++pos) { - s.Printf("row[%u]: ", (uint32_t)std::distance(begin, pos)); - (*pos)->Dump(s, this, thread, base_addr); - s.Printf("\n"); + for (const auto &[index, row_sp] : llvm::enumerate(m_row_list)) { + s.Format("row[{0}]: ", index); + row_sp->Dump(s, this, thread, base_addr); + s << "\n"; } } diff --git a/lldb/unittests/Symbol/CMakeLists.txt b/lldb/unittests/Symbol/CMakeLists.txt index ab5cecd101833..5664c21adbe3f 100644 --- a/lldb/unittests/Symbol/CMakeLists.txt +++ b/lldb/unittests/Symbol/CMakeLists.txt @@ -12,6 +12,7 @@ add_lldb_unittest(SymbolTests TestDWARFCallFrameInfo.cpp TestType.cpp TestLineEntry.cpp + UnwindPlanTest.cpp LINK_LIBS lldbCore diff --git a/lldb/unittests/Symbol/UnwindPlanTest.cpp b/lldb/unittests/Symbol/UnwindPlanTest.cpp new file mode 100644 index 0000000000000..fa8bb153e9247 --- /dev/null +++ b/lldb/unittests/Symbol/UnwindPlanTest.cpp @@ -0,0 +1,76 @@ +//===-- UnwindPlanTest.cpp ------------------------------------------------===// +// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#include "lldb/Symbol/UnwindPlan.h" +#include "gmock/gmock.h" +#include "gtest/gtest.h" + +using namespace lldb_private; +using namespace lldb; + +static UnwindPlan::Row make_simple_row(addr_t offset, uint64_t cfa_value) { + UnwindPlan::Row row; + row.SetOffset(offset); + row.GetCFAValue().SetIsConstant(cfa_value); + return row; +} + +TEST(UnwindPlan, InsertRow) { + UnwindPlan::Row row1 = make_simple_row(0, 42); + UnwindPlan::Row row2 = make_simple_row(0, 47); + + UnwindPlan plan(eRegisterKindGeneric); + plan.InsertRow(row1); + EXPECT_THAT(plan.GetRowForFunctionOffset(0), testing::Pointee(row1)); + + plan.InsertRow(row2, /*replace_existing=*/false); + EXPECT_THAT(plan.GetRowForFunctionOffset(0), testing::Pointee(row1)); + + plan.InsertRow(row2, /*replace_existing=*/true); + EXPECT_THAT(plan.GetRowForFunctionOffset(0), testing::Pointee(row2)); +} + +TEST(UnwindPlan, GetRowForFunctionOffset) { + UnwindPlan::Row row1 = make_simple_row(10, 42); + UnwindPlan::Row row2 = make_simple_row(20, 47); + + UnwindPlan plan(eRegisterKindGeneric); + plan.InsertRow(row1); + plan.InsertRow(row2); + + EXPECT_THAT(plan.GetRowForFunctionOffset(0), nullptr); + EXPECT_THAT(plan.GetRowForFunctionOffset(9), nullptr); + EXPECT_THAT(plan.GetRowForFunctionOffset(10), testing::Pointee(row1)); + EXPECT_THAT(plan.GetRowForFunctionOffset(19), testing::Pointee(row1)); + EXPECT_THAT(plan.GetRowForFunctionOffset(20), testing::Pointee(row2)); + EXPECT_THAT(plan.GetRowForFunctionOffset(99), testing::Pointee(row2)); +} + +TEST(UnwindPlan, PlanValidAtAddress) { + UnwindPlan::Row row1 = make_simple_row(0, 42); + UnwindPlan::Row row2 = make_simple_row(10, 47); + + UnwindPlan plan(eRegisterKindGeneric); + EXPECT_FALSE(plan.PlanValidAtAddress(Address(0))); + + plan.InsertRow(row2); + EXPECT_FALSE(plan.PlanValidAtAddress(Address(0))); + + plan.InsertRow(row1); + EXPECT_TRUE(plan.PlanValidAtAddress(Address(0))); + EXPECT_TRUE(plan.PlanValidAtAddress(Address(10))); + + plan.SetPlanValidAddressRanges({AddressRange(0, 5), AddressRange(15, 5)}); + EXPECT_TRUE(plan.PlanValidAtAddress(Address(0))); + EXPECT_FALSE(plan.PlanValidAtAddress(Address(5))); + EXPECT_FALSE(plan.PlanValidAtAddress(Address(10))); + EXPECT_TRUE(plan.PlanValidAtAddress(Address(15))); + EXPECT_FALSE(plan.PlanValidAtAddress(Address(20))); + EXPECT_FALSE(plan.PlanValidAtAddress(Address(25))); +} diff --git a/lldb/unittests/UnwindAssembly/x86/Testx86AssemblyInspectionEngine.cpp b/lldb/unittests/UnwindAssembly/x86/Testx86AssemblyInspectionEngine.cpp index db3a7d5b804ee..7ea35b99ef845 100644 --- a/lldb/unittests/UnwindAssembly/x86/Testx86AssemblyInspectionEngine.cpp +++ b/lldb/unittests/UnwindAssembly/x86/Testx86AssemblyInspectionEngine.cpp @@ -2247,7 +2247,7 @@ TEST_F(Testx86AssemblyInspectionEngine, TestSpArithx86_64Augmented) { sample_range = AddressRange(0x1000, sizeof(data)); unwind_plan.SetSourceName("unit testing hand-created unwind plan"); - unwind_plan.SetPlanValidAddressRange(sample_range); + unwind_plan.SetPlanValidAddressRanges({sample_range}); unwind_plan.SetRegisterKind(eRegisterKindLLDB); { @@ -2323,7 +2323,7 @@ TEST_F(Testx86AssemblyInspectionEngine, TestSimplex86_64Augmented) { sample_range = AddressRange(0x1000, sizeof(data)); unwind_plan.SetSourceName("unit testing hand-created unwind plan"); - unwind_plan.SetPlanValidAddressRange(sample_range); + unwind_plan.SetPlanValidAddressRanges({sample_range}); unwind_plan.SetRegisterKind(eRegisterKindLLDB); { @@ -2390,7 +2390,7 @@ TEST_F(Testx86AssemblyInspectionEngine, TestSimplei386ugmented) { sample_range = AddressRange(0x1000, sizeof(data)); unwind_plan.SetSourceName("unit testing hand-created unwind plan"); - unwind_plan.SetPlanValidAddressRange(sample_range); + unwind_plan.SetPlanValidAddressRanges({sample_range}); unwind_plan.SetRegisterKind(eRegisterKindLLDB); { _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
