llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-lldb Author: Pavel Labath (labath) <details> <summary>Changes</summary> AddressFunctionScope was always returning the first address range of the function (assuming it was the only one). This doesn't work for RegisterContextUnwind (it's only caller), when the function doesn't start at the lowest address because it throws off the 'how many bytes "into" a function I am' computation. This patch replaces the result with a call to (recently introduced) SymbolContext::GetFunctionOrSymbolAddress. Note to reviewers: This patch consists of two commits. The first one is equivalent to #<!-- -->137006. The two PRs are functionally independent, but without the other PR, unwinding would be so broken there wouldn't be anything to test. When reviewing, I recommend viewing only the second commit in the PR. --- Patch is 27.83 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/137155.diff 10 Files Affected: - (modified) lldb/include/lldb/Core/Address.h (+3-10) - (modified) lldb/include/lldb/Symbol/DWARFCallFrameInfo.h (+6-3) - (modified) lldb/source/Core/Address.cpp (+2-13) - (modified) lldb/source/Symbol/DWARFCallFrameInfo.cpp (+34-30) - (modified) lldb/source/Symbol/FuncUnwinders.cpp (+7-14) - (modified) lldb/source/Symbol/UnwindTable.cpp (+15-12) - (modified) lldb/source/Target/RegisterContextUnwind.cpp (+25-35) - (modified) lldb/test/Shell/Unwind/Inputs/basic-block-sections-with-dwarf.s (+52-43) - (modified) lldb/test/Shell/Unwind/basic-block-sections-with-dwarf-static.test (+25-14) - (modified) lldb/unittests/Symbol/TestDWARFCallFrameInfo.cpp (+7-6) ``````````diff diff --git a/lldb/include/lldb/Core/Address.h b/lldb/include/lldb/Core/Address.h index 9b5874f8b1fbe..85b2ab7bb3cfe 100644 --- a/lldb/include/lldb/Core/Address.h +++ b/lldb/include/lldb/Core/Address.h @@ -371,22 +371,15 @@ class Address { bool ResolveAddressUsingFileSections(lldb::addr_t addr, const SectionList *sections); - /// Resolve this address to its containing function and optionally get - /// that function's address range. + /// Resolve this address to its containing function. /// /// \param[out] sym_ctx /// The symbol context describing the function in which this address lies /// - /// \parm[out] addr_range_ptr - /// Pointer to the AddressRange to fill in with the function's address - /// range. Caller may pass null if they don't need the address range. - /// /// \return - /// Returns \b false if the function/symbol could not be resolved - /// or if the address range was requested and could not be resolved; + /// Returns \b false if the function/symbol could not be resolved; /// returns \b true otherwise. - bool ResolveFunctionScope(lldb_private::SymbolContext &sym_ctx, - lldb_private::AddressRange *addr_range_ptr = nullptr); + bool ResolveFunctionScope(lldb_private::SymbolContext &sym_ctx); /// Set the address to represent \a load_addr. /// diff --git a/lldb/include/lldb/Symbol/DWARFCallFrameInfo.h b/lldb/include/lldb/Symbol/DWARFCallFrameInfo.h index 679f652c7f2e0..c214ed1f60919 100644 --- a/lldb/include/lldb/Symbol/DWARFCallFrameInfo.h +++ b/lldb/include/lldb/Symbol/DWARFCallFrameInfo.h @@ -47,12 +47,15 @@ class DWARFCallFrameInfo { /// Return an UnwindPlan based on the call frame information encoded in the /// FDE of this DWARFCallFrameInfo section. The returned plan will be valid /// (at least) for the given address. - bool GetUnwindPlan(const Address &addr, UnwindPlan &unwind_plan); + std::unique_ptr<UnwindPlan> GetUnwindPlan(const Address &addr); /// Return an UnwindPlan based on the call frame information encoded in the /// FDE of this DWARFCallFrameInfo section. The returned plan will be valid - /// (at least) for some address in the given range. - bool GetUnwindPlan(const AddressRange &range, UnwindPlan &unwind_plan); + /// (at least) for some address in the given ranges. If no unwind information + /// is found, nullptr is returned. \a addr represents the entry point of the + /// function. It corresponds to the offset zero in the returned UnwindPlan. + std::unique_ptr<UnwindPlan> GetUnwindPlan(llvm::ArrayRef<AddressRange> ranges, + const Address &addr); typedef RangeVector<lldb::addr_t, uint32_t> FunctionAddressAndSizeVector; diff --git a/lldb/source/Core/Address.cpp b/lldb/source/Core/Address.cpp index 1dab874a96583..a967bf5491211 100644 --- a/lldb/source/Core/Address.cpp +++ b/lldb/source/Core/Address.cpp @@ -263,22 +263,11 @@ bool Address::ResolveAddressUsingFileSections(addr_t file_addr, return false; // Failed to resolve this address to a section offset value } -/// if "addr_range_ptr" is not NULL, then fill in with the address range of the function. -bool Address::ResolveFunctionScope(SymbolContext &sym_ctx, - AddressRange *addr_range_ptr) { +bool Address::ResolveFunctionScope(SymbolContext &sym_ctx) { constexpr SymbolContextItem resolve_scope = eSymbolContextFunction | eSymbolContextSymbol; - if (!(CalculateSymbolContext(&sym_ctx, resolve_scope) & resolve_scope)) { - if (addr_range_ptr) - addr_range_ptr->Clear(); - return false; - } - - if (!addr_range_ptr) - return true; - - return sym_ctx.GetAddressRange(resolve_scope, 0, false, *addr_range_ptr); + return CalculateSymbolContext(&sym_ctx, resolve_scope) & resolve_scope; } ModuleSP Address::GetModule() const { diff --git a/lldb/source/Symbol/DWARFCallFrameInfo.cpp b/lldb/source/Symbol/DWARFCallFrameInfo.cpp index a763acb1fdf9e..cb8aa8a26c3f1 100644 --- a/lldb/source/Symbol/DWARFCallFrameInfo.cpp +++ b/lldb/source/Symbol/DWARFCallFrameInfo.cpp @@ -151,53 +151,57 @@ DWARFCallFrameInfo::DWARFCallFrameInfo(ObjectFile &objfile, SectionSP §ion_sp, Type type) : m_objfile(objfile), m_section_sp(section_sp), m_type(type) {} -bool DWARFCallFrameInfo::GetUnwindPlan(const Address &addr, - UnwindPlan &unwind_plan) { - return GetUnwindPlan(AddressRange(addr, 1), unwind_plan); +std::unique_ptr<UnwindPlan> +DWARFCallFrameInfo::GetUnwindPlan(const Address &addr) { + return GetUnwindPlan({AddressRange(addr, 1)}, addr); } -bool DWARFCallFrameInfo::GetUnwindPlan(const AddressRange &range, - UnwindPlan &unwind_plan) { +std::unique_ptr<UnwindPlan> +DWARFCallFrameInfo::GetUnwindPlan(llvm::ArrayRef<AddressRange> ranges, + const Address &addr) { FDEEntryMap::Entry fde_entry; - Address addr = range.GetBaseAddress(); // Make sure that the Address we're searching for is the same object file as // this DWARFCallFrameInfo, we only store File offsets in m_fde_index. ModuleSP module_sp = addr.GetModule(); if (module_sp.get() == nullptr || module_sp->GetObjectFile() == nullptr || module_sp->GetObjectFile() != &m_objfile) - return false; + return nullptr; - std::optional<FDEEntryMap::Entry> entry = GetFirstFDEEntryInRange(range); - if (!entry) - return false; + std::vector<AddressRange> valid_ranges; - std::optional<FDE> fde = ParseFDE(entry->data, addr); - if (!fde) - return false; - - unwind_plan.SetSourceName(m_type == EH ? "eh_frame CFI" : "DWARF CFI"); + auto result = std::make_unique<UnwindPlan>(GetRegisterKind()); + result->SetSourceName(m_type == EH ? "eh_frame CFI" : "DWARF CFI"); // In theory the debug_frame info should be valid at all call sites // ("asynchronous unwind info" as it is sometimes called) but in practice // gcc et al all emit call frame info for the prologue and call sites, but // not for the epilogue or all the other locations during the function // reliably. - unwind_plan.SetUnwindPlanValidAtAllInstructions(eLazyBoolNo); - unwind_plan.SetSourcedFromCompiler(eLazyBoolYes); - unwind_plan.SetRegisterKind(GetRegisterKind()); - - unwind_plan.SetPlanValidAddressRanges({fde->range}); - unwind_plan.SetUnwindPlanForSignalTrap(fde->for_signal_trap ? eLazyBoolYes - : eLazyBoolNo); - unwind_plan.SetReturnAddressRegister(fde->return_addr_reg_num); - int64_t slide = - fde->range.GetBaseAddress().GetFileAddress() - addr.GetFileAddress(); - for (UnwindPlan::Row &row : fde->rows) { - row.SlideOffset(slide); - unwind_plan.AppendRow(std::move(row)); + result->SetUnwindPlanValidAtAllInstructions(eLazyBoolNo); + result->SetSourcedFromCompiler(eLazyBoolYes); + result->SetUnwindPlanForSignalTrap(eLazyBoolNo); + for (const AddressRange &range : ranges) { + std::optional<FDEEntryMap::Entry> entry = GetFirstFDEEntryInRange(range); + if (!entry) + continue; + std::optional<FDE> fde = ParseFDE(entry->data, addr); + if (!fde) + continue; + int64_t slide = + fde->range.GetBaseAddress().GetFileAddress() - addr.GetFileAddress(); + valid_ranges.push_back(std::move(fde->range)); + if (fde->for_signal_trap) + result->SetUnwindPlanForSignalTrap(eLazyBoolYes); + result->SetReturnAddressRegister(fde->return_addr_reg_num); + for (UnwindPlan::Row &row : fde->rows) { + row.SlideOffset(slide); + result->AppendRow(std::move(row)); + } } - - return true; + result->SetPlanValidAddressRanges(std::move(valid_ranges)); + if (result->GetRowCount() == 0) + return nullptr; + return result; } bool DWARFCallFrameInfo::GetAddressRange(Address addr, AddressRange &range) { diff --git a/lldb/source/Symbol/FuncUnwinders.cpp b/lldb/source/Symbol/FuncUnwinders.cpp index 11600825e8e38..faec24cde7fdd 100644 --- a/lldb/source/Symbol/FuncUnwinders.cpp +++ b/lldb/source/Symbol/FuncUnwinders.cpp @@ -149,13 +149,9 @@ FuncUnwinders::GetEHFrameUnwindPlan(Target &target) { return m_unwind_plan_eh_frame_sp; m_tried_unwind_plan_eh_frame = true; - if (m_range.GetBaseAddress().IsValid()) { - DWARFCallFrameInfo *eh_frame = m_unwind_table.GetEHFrameInfo(); - if (eh_frame) { - auto plan_sp = std::make_shared<UnwindPlan>(lldb::eRegisterKindGeneric); - if (eh_frame->GetUnwindPlan(m_range, *plan_sp)) - m_unwind_plan_eh_frame_sp = std::move(plan_sp); - } + if (m_addr.IsValid()) { + if (DWARFCallFrameInfo *eh_frame = m_unwind_table.GetEHFrameInfo()) + m_unwind_plan_eh_frame_sp = eh_frame->GetUnwindPlan(m_ranges, m_addr); } return m_unwind_plan_eh_frame_sp; } @@ -167,13 +163,10 @@ FuncUnwinders::GetDebugFrameUnwindPlan(Target &target) { return m_unwind_plan_debug_frame_sp; m_tried_unwind_plan_debug_frame = true; - if (m_range.GetBaseAddress().IsValid()) { - DWARFCallFrameInfo *debug_frame = m_unwind_table.GetDebugFrameInfo(); - if (debug_frame) { - auto plan_sp = std::make_shared<UnwindPlan>(lldb::eRegisterKindGeneric); - if (debug_frame->GetUnwindPlan(m_range, *plan_sp)) - m_unwind_plan_debug_frame_sp = std::move(plan_sp); - } + if (!m_ranges.empty()) { + if (DWARFCallFrameInfo *debug_frame = m_unwind_table.GetDebugFrameInfo()) + m_unwind_plan_debug_frame_sp = + debug_frame->GetUnwindPlan(m_ranges, m_addr); } return m_unwind_plan_debug_frame_sp; } diff --git a/lldb/source/Symbol/UnwindTable.cpp b/lldb/source/Symbol/UnwindTable.cpp index 21ecd434d7212..3aca495696c84 100644 --- a/lldb/source/Symbol/UnwindTable.cpp +++ b/lldb/source/Symbol/UnwindTable.cpp @@ -122,6 +122,13 @@ AddressRanges UnwindTable::GetAddressRanges(const Address &addr, return {}; } +static Address GetFunctionOrSymbolAddress(const Address &addr, + const SymbolContext &sc) { + if (Address result = sc.GetFunctionOrSymbolAddress(); result.IsValid()) + return result; + return addr; +} + FuncUnwindersSP UnwindTable::GetFuncUnwindersContainingAddress(const Address &addr, const SymbolContext &sc) { @@ -131,25 +138,20 @@ UnwindTable::GetFuncUnwindersContainingAddress(const Address &addr, // There is an UnwindTable per object file, so we can safely use file handles addr_t file_addr = addr.GetFileAddress(); - iterator end = m_unwinds.end(); - iterator insert_pos = end; - if (!m_unwinds.empty()) { - insert_pos = m_unwinds.lower_bound(file_addr); - iterator pos = insert_pos; - if ((pos == m_unwinds.end()) || - (pos != m_unwinds.begin() && - pos->second->GetFunctionStartAddress() != addr)) - --pos; - + iterator insert_pos = m_unwinds.upper_bound(file_addr); + if (insert_pos != m_unwinds.begin()) { + auto pos = std::prev(insert_pos); if (pos->second->ContainsAddress(addr)) return pos->second; } + Address start_addr = GetFunctionOrSymbolAddress(addr, sc); AddressRanges ranges = GetAddressRanges(addr, sc); if (ranges.empty()) return nullptr; - auto func_unwinder_sp = std::make_shared<FuncUnwinders>(*this, addr, ranges); + auto func_unwinder_sp = + std::make_shared<FuncUnwinders>(*this, start_addr, ranges); for (const AddressRange &range : ranges) m_unwinds.emplace_hint(insert_pos, range.GetBaseAddress().GetFileAddress(), func_unwinder_sp); @@ -164,11 +166,12 @@ FuncUnwindersSP UnwindTable::GetUncachedFuncUnwindersContainingAddress( const Address &addr, const SymbolContext &sc) { Initialize(); + Address start_addr = GetFunctionOrSymbolAddress(addr, sc); AddressRanges ranges = GetAddressRanges(addr, sc); if (ranges.empty()) return nullptr; - return std::make_shared<FuncUnwinders>(*this, addr, std::move(ranges)); + return std::make_shared<FuncUnwinders>(*this, start_addr, std::move(ranges)); } void UnwindTable::Dump(Stream &s) { diff --git a/lldb/source/Target/RegisterContextUnwind.cpp b/lldb/source/Target/RegisterContextUnwind.cpp index 3ed49e12476dd..c7185962ae8ea 100644 --- a/lldb/source/Target/RegisterContextUnwind.cpp +++ b/lldb/source/Target/RegisterContextUnwind.cpp @@ -160,8 +160,7 @@ void RegisterContextUnwind::InitializeZerothFrame() { UnwindLogMsg("using architectural default unwind method"); } - AddressRange addr_range; - m_sym_ctx_valid = m_current_pc.ResolveFunctionScope(m_sym_ctx, &addr_range); + m_sym_ctx_valid = m_current_pc.ResolveFunctionScope(m_sym_ctx); if (m_sym_ctx.symbol) { UnwindLogMsg("with pc value of 0x%" PRIx64 ", symbol name is '%s'", @@ -185,15 +184,9 @@ void RegisterContextUnwind::InitializeZerothFrame() { // If we were able to find a symbol/function, set addr_range to the bounds of // that symbol/function. else treat the current pc value as the start_pc and // record no offset. - if (addr_range.GetBaseAddress().IsValid()) { - m_start_pc = addr_range.GetBaseAddress(); - if (m_current_pc.GetSection() == m_start_pc.GetSection()) { - m_current_offset = m_current_pc.GetOffset() - m_start_pc.GetOffset(); - } else if (m_current_pc.GetModule() == m_start_pc.GetModule()) { - // This means that whatever symbol we kicked up isn't really correct --- - // we should not cross section boundaries ... We really should NULL out - // the function/symbol in this case unless there is a bad assumption here - // due to inlined functions? + if (m_sym_ctx_valid) { + m_start_pc = m_sym_ctx.GetFunctionOrSymbolAddress(); + if (m_current_pc.GetModule() == m_start_pc.GetModule()) { m_current_offset = m_current_pc.GetFileAddress() - m_start_pc.GetFileAddress(); } @@ -498,8 +491,7 @@ void RegisterContextUnwind::InitializeNonZerothFrame() { return; } - AddressRange addr_range; - m_sym_ctx_valid = m_current_pc.ResolveFunctionScope(m_sym_ctx, &addr_range); + m_sym_ctx_valid = m_current_pc.ResolveFunctionScope(m_sym_ctx); if (m_sym_ctx.symbol) { UnwindLogMsg("with pc value of 0x%" PRIx64 ", symbol name is '%s'", pc, @@ -523,9 +515,8 @@ void RegisterContextUnwind::InitializeNonZerothFrame() { // Don't decrement if we're "above" an asynchronous event like // sigtramp. decr_pc_and_recompute_addr_range = false; - } else if (!addr_range.GetBaseAddress().IsValid() || - addr_range.GetBaseAddress().GetSection() != m_current_pc.GetSection() || - addr_range.GetBaseAddress().GetOffset() != m_current_pc.GetOffset()) { + } else if (Address addr = m_sym_ctx.GetFunctionOrSymbolAddress(); + addr != m_current_pc) { // If our "current" pc isn't the start of a function, decrement the pc // if we're up the stack. if (m_behaves_like_zeroth_frame) @@ -558,7 +549,7 @@ void RegisterContextUnwind::InitializeNonZerothFrame() { Address temporary_pc; temporary_pc.SetLoadAddress(pc - 1, &process->GetTarget()); m_sym_ctx.Clear(false); - m_sym_ctx_valid = temporary_pc.ResolveFunctionScope(m_sym_ctx, &addr_range); + m_sym_ctx_valid = temporary_pc.ResolveFunctionScope(m_sym_ctx); UnwindLogMsg("Symbol is now %s", GetSymbolOrFunctionName(m_sym_ctx).AsCString("")); @@ -567,8 +558,8 @@ void RegisterContextUnwind::InitializeNonZerothFrame() { // If we were able to find a symbol/function, set addr_range_ptr to the // bounds of that symbol/function. else treat the current pc value as the // start_pc and record no offset. - if (addr_range.GetBaseAddress().IsValid()) { - m_start_pc = addr_range.GetBaseAddress(); + if (m_sym_ctx_valid) { + m_start_pc = m_sym_ctx.GetFunctionOrSymbolAddress(); m_current_offset = pc - m_start_pc.GetLoadAddress(&process->GetTarget()); m_current_offset_backed_up_one = m_current_offset; if (decr_pc_and_recompute_addr_range && @@ -868,13 +859,11 @@ RegisterContextUnwind::GetFullUnwindPlanForFrame() { // Even with -fomit-frame-pointer, we can try eh_frame to get back on // track. - DWARFCallFrameInfo *eh_frame = - pc_module_sp->GetUnwindTable().GetEHFrameInfo(); - if (eh_frame) { - auto unwind_plan_sp = - std::make_shared<UnwindPlan>(lldb::eRegisterKindGeneric); - if (eh_frame->GetUnwindPlan(m_current_pc, *unwind_plan_sp)) - return unwind_plan_sp; + if (DWARFCallFrameInfo *eh_frame = + pc_module_sp->GetUnwindTable().GetEHFrameInfo()) { + if (std::unique_ptr<UnwindPlan> plan_up = + eh_frame->GetUnwindPlan(m_current_pc)) + return plan_up; } ArmUnwindInfo *arm_exidx = @@ -1345,9 +1334,9 @@ RegisterContextUnwind::SavedLocationForRegister( // value instead of the Return Address register. // If $pc is not available, fall back to the RA reg. UnwindPlan::Row::AbstractRegisterLocation scratch; - if (m_frame_type == eTrapHandlerFrame && - active_row->GetRegisterInfo - (pc_regnum.GetAsKind (unwindplan_registerkind), scratch)) { + if (m_frame_type == eTrapHandlerFrame && active_row && + active_row->GetRegisterInfo( + pc_regnum.GetAsKind(unwindplan_registerkind), scratch)) { UnwindLogMsg("Providing pc register instead of rewriting to " "RA reg because this is a trap handler and there is " "a location for the saved pc register value."); @@ -1377,7 +1366,7 @@ RegisterContextUnwind::SavedLocationForRegister( } } - if (regnum.IsValid() && + if (regnum.IsValid() && active_row && active_row->GetRegisterInfo(regnum.GetAsKind(unwindplan_registerkind), unwindplan_regloc)) { have_unwindplan_regloc = true; @@ -1941,8 +1930,7 @@ void RegisterContextUnwind::PropagateTrapHandlerFlagFromUnwindPlan( GetSymbolOrFunctionName(m_sym_ctx).AsCString("")); m_current_offset_backed_up_one = m_current_offset; - AddressRange addr_range; - m_sym_ctx_valid = m_current_pc.ResolveFunctionScope(m_sym_ctx, &addr_range); + m_sym_ctx_valid = m_current_pc.ResolveFunctionScope(m_sym_ctx); UnwindLogMsg("Symbol is now %s", GetSymbolOrFunctionName(m_sym_ctx).AsCString("")); @@ -1951,9 +1939,11 @@ void RegisterContextUnwind::PropagateTrapHandlerFlagFromUnwindPlan( Process *process = exe_ctx.GetProcessPtr(); Target *target = &process->GetTarget(); - m_start_pc = addr_range.GetBaseAddress(); - m_current_offset = - m_current_pc.GetLoadAddress(target) - m_start_pc.GetLoadAddress(target); + if (m_sym_ctx_valid) { + m_start_pc = m_sym_ctx.GetFunctionOrSymbolAddress(); + m_current_offset = m_current_pc.GetLoadAddress(target) - + m_start_pc.GetLoadAddress(target); + } } } diff --git a/lldb/test/Shell/Unwind/Inputs/basic-block-sections-with-dwarf.s b/lldb/test/Shell/Unwind/Inputs/basic-block-sections-with-dwarf.s index c405e51c227cb..a7b5431a7afaf 100644 --- a/lldb/test/Shell/Unwind/Inputs/basic-block-sections-with-dwarf.s +++ b/lldb/test/Shell/Unwind/Inputs/basic-block-sections-with-dwarf.s @@ -4,7 +4,9 @@ # int bar() { return foo(0); } # int foo(int flag) { return flag ? bar() : baz(); } # int main() { return foo(1); } -# The function bar has been placed "in the middle" of foo. +# The function bar has been placed "in the middle" of foo. The functions are not +# using the frame pointer register and the are deliberately adjusting the stack +# pointer to test that we're using the correct unwind row. .text @@ -17,29 +19,31 @@ baz: .Lbaz_end: .size baz, .Lbaz_end-baz - .type foo,@function -foo: +foo.__part.3: .cfi_startproc - pushq %rbp - .cfi_def_cfa_offset 16 - .cfi_offset %rbp, -16 - movq %rsp, %rbp - .cfi_def_cfa_register %rbp - subq $16, %rsp - movl %edi, -8(%rbp) - cmpl $0, -8(%rbp) - je foo.__part.2 - jmp foo.__part.1 + .cfi_def_cfa_offset 32 + .cfi_offset %rbx, -16 + addq $24, %rsp + .cfi_def_cfa %rsp, 8 + ... [truncated] `````````` </details> https://github.com/llvm/llvm-project/pull/137155 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
