llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-lldb Author: Pavel Labath (labath) <details> <summary>Changes</summary> Basically, disable everything except the eh_frame unwind plan, as that's the only one which supports this right now. The other plans are working with now trying the interpret everything in between the function parts as a part of the function, which is more likely to produce wrong results than correct ones. I changed the interface for object file plans, to give the implementations a chance to implement this correctly, but I haven't actually converted PECallFrameInfo (its only implementation) to handle that. (from the looks of things, it should be relatively easy to do, if it becomes necessary) I'm also deleting UnwindPlan::GetFirstNonPrologueInsn, as it's not used, and it doesn't work for discontinuous functions. --- Full diff: https://github.com/llvm/llvm-project/pull/140927.diff 7 Files Affected: - (modified) lldb/include/lldb/Symbol/CallFrameInfo.h (+4-2) - (modified) lldb/include/lldb/Symbol/FuncUnwinders.h (-6) - (modified) lldb/source/Plugins/ObjectFile/PECOFF/PECallFrameInfo.cpp (+17-18) - (modified) lldb/source/Plugins/ObjectFile/PECOFF/PECallFrameInfo.h (+10-4) - (modified) lldb/source/Symbol/FuncUnwinders.cpp (+38-67) - (modified) lldb/source/Target/RegisterContextUnwind.cpp (+3-4) - (modified) lldb/unittests/ObjectFile/PECOFF/TestPECallFrameInfo.cpp (+28-16) ``````````diff diff --git a/lldb/include/lldb/Symbol/CallFrameInfo.h b/lldb/include/lldb/Symbol/CallFrameInfo.h index 7db8722baaf5f..f4c5a5b374871 100644 --- a/lldb/include/lldb/Symbol/CallFrameInfo.h +++ b/lldb/include/lldb/Symbol/CallFrameInfo.h @@ -19,8 +19,10 @@ class CallFrameInfo { virtual bool GetAddressRange(Address addr, AddressRange &range) = 0; - virtual bool GetUnwindPlan(const Address &addr, UnwindPlan &unwind_plan) = 0; - virtual bool GetUnwindPlan(const AddressRange &range, UnwindPlan &unwind_plan) = 0; + virtual std::unique_ptr<UnwindPlan> + GetUnwindPlan(llvm::ArrayRef<AddressRange> ranges, const Address &addr) = 0; + + virtual std::unique_ptr<UnwindPlan> GetUnwindPlan(const Address &addr) = 0; }; } // namespace lldb_private diff --git a/lldb/include/lldb/Symbol/FuncUnwinders.h b/lldb/include/lldb/Symbol/FuncUnwinders.h index c21a1af5c56a2..abb0d8abbc21a 100644 --- a/lldb/include/lldb/Symbol/FuncUnwinders.h +++ b/lldb/include/lldb/Symbol/FuncUnwinders.h @@ -51,8 +51,6 @@ class FuncUnwinders { std::shared_ptr<const UnwindPlan> GetUnwindPlanArchitectureDefaultAtFunctionEntry(lldb_private::Thread &thread); - Address &GetFirstNonPrologueInsn(Target &target); - const Address &GetFunctionStartAddress() const; bool ContainsAddress(const Address &addr) const { @@ -114,10 +112,6 @@ class FuncUnwinders { /// The address ranges of the function. AddressRanges m_ranges; - /// The smallest address range covering the entire function. - /// DEPRECATED: Use m_ranges instead. - AddressRange m_range; - std::recursive_mutex m_mutex; std::shared_ptr<const UnwindPlan> m_unwind_plan_assembly_sp; diff --git a/lldb/source/Plugins/ObjectFile/PECOFF/PECallFrameInfo.cpp b/lldb/source/Plugins/ObjectFile/PECOFF/PECallFrameInfo.cpp index a7c2e4f0b8dbc..4b516b2e954bd 100644 --- a/lldb/source/Plugins/ObjectFile/PECOFF/PECallFrameInfo.cpp +++ b/lldb/source/Plugins/ObjectFile/PECOFF/PECallFrameInfo.cpp @@ -455,27 +455,26 @@ bool PECallFrameInfo::GetAddressRange(Address addr, AddressRange &range) { return true; } -bool PECallFrameInfo::GetUnwindPlan(const Address &addr, - UnwindPlan &unwind_plan) { - return GetUnwindPlan(AddressRange(addr, 1), unwind_plan); -} - -bool PECallFrameInfo::GetUnwindPlan(const AddressRange &range, - UnwindPlan &unwind_plan) { - unwind_plan.Clear(); - - unwind_plan.SetSourceName("PE EH info"); - unwind_plan.SetSourcedFromCompiler(eLazyBoolYes); - unwind_plan.SetRegisterKind(eRegisterKindLLDB); +std::unique_ptr<UnwindPlan> PECallFrameInfo::GetUnwindPlan( + llvm::ArrayRef<lldb_private::AddressRange> ranges, + const lldb_private::Address &addr) { + // Only continuous functions are supported. + if (ranges.size() != 1) + return nullptr; + const AddressRange &range = ranges[0]; const RuntimeFunction *runtime_function = FindRuntimeFunctionIntersectsWithRange(range); if (!runtime_function) - return false; + return nullptr; + + auto plan_up = std::make_unique<UnwindPlan>(eRegisterKindLLDB); + plan_up->SetSourceName("PE EH info"); + plan_up->SetSourcedFromCompiler(eLazyBoolYes); EHProgramBuilder builder(m_object_file, runtime_function->UnwindInfoOffset); if (!builder.Build()) - return false; + return nullptr; std::vector<UnwindPlan::Row> rows; @@ -493,14 +492,14 @@ bool PECallFrameInfo::GetUnwindPlan(const AddressRange &range, } for (auto it = rows.rbegin(); it != rows.rend(); ++it) - unwind_plan.AppendRow(std::move(*it)); + plan_up->AppendRow(std::move(*it)); - unwind_plan.SetPlanValidAddressRanges({AddressRange( + plan_up->SetPlanValidAddressRanges({AddressRange( m_object_file.GetAddress(runtime_function->StartAddress), runtime_function->EndAddress - runtime_function->StartAddress)}); - unwind_plan.SetUnwindPlanValidAtAllInstructions(eLazyBoolNo); + plan_up->SetUnwindPlanValidAtAllInstructions(eLazyBoolNo); - return true; + return plan_up; } const RuntimeFunction *PECallFrameInfo::FindRuntimeFunctionIntersectsWithRange( diff --git a/lldb/source/Plugins/ObjectFile/PECOFF/PECallFrameInfo.h b/lldb/source/Plugins/ObjectFile/PECOFF/PECallFrameInfo.h index 7c2a479ecddb5..07999aa7ccae0 100644 --- a/lldb/source/Plugins/ObjectFile/PECOFF/PECallFrameInfo.h +++ b/lldb/source/Plugins/ObjectFile/PECOFF/PECallFrameInfo.h @@ -9,7 +9,9 @@ #ifndef LLDB_SOURCE_PLUGINS_OBJECTFILE_PECOFF_PECALLFRAMEINFO_H #define LLDB_SOURCE_PLUGINS_OBJECTFILE_PECOFF_PECALLFRAMEINFO_H +#include "lldb/Core/AddressRange.h" #include "lldb/Symbol/CallFrameInfo.h" +#include "lldb/Symbol/UnwindPlan.h" #include "lldb/Utility/DataExtractor.h" class ObjectFilePECOFF; @@ -31,10 +33,14 @@ class PECallFrameInfo : public virtual lldb_private::CallFrameInfo { bool GetAddressRange(lldb_private::Address addr, lldb_private::AddressRange &range) override; - bool GetUnwindPlan(const lldb_private::Address &addr, - lldb_private::UnwindPlan &unwind_plan) override; - bool GetUnwindPlan(const lldb_private::AddressRange &range, - lldb_private::UnwindPlan &unwind_plan) override; + std::unique_ptr<lldb_private::UnwindPlan> + GetUnwindPlan(const lldb_private::Address &addr) override { + return GetUnwindPlan({lldb_private::AddressRange(addr, 1)}, addr); + } + + std::unique_ptr<lldb_private::UnwindPlan> + GetUnwindPlan(llvm::ArrayRef<lldb_private::AddressRange> ranges, + const lldb_private::Address &addr) override; private: const llvm::Win64EH::RuntimeFunction *FindRuntimeFunctionIntersectsWithRange( diff --git a/lldb/source/Symbol/FuncUnwinders.cpp b/lldb/source/Symbol/FuncUnwinders.cpp index 12a6d101d9930..fffc35d7f6177 100644 --- a/lldb/source/Symbol/FuncUnwinders.cpp +++ b/lldb/source/Symbol/FuncUnwinders.cpp @@ -31,30 +31,11 @@ using namespace lldb; using namespace lldb_private; -static AddressRange CollapseRanges(llvm::ArrayRef<AddressRange> ranges) { - if (ranges.empty()) - return AddressRange(); - if (ranges.size() == 1) - return ranges[0]; - - Address lowest_addr = ranges[0].GetBaseAddress(); - addr_t highest_addr = lowest_addr.GetFileAddress() + ranges[0].GetByteSize(); - for (const AddressRange &range : ranges.drop_front()) { - Address range_begin = range.GetBaseAddress(); - addr_t range_end = range_begin.GetFileAddress() + range.GetByteSize(); - if (range_begin.GetFileAddress() < lowest_addr.GetFileAddress()) - lowest_addr = range_begin; - if (range_end > highest_addr) - highest_addr = range_end; - } - return AddressRange(lowest_addr, highest_addr - lowest_addr.GetFileAddress()); -} - FuncUnwinders::FuncUnwinders(UnwindTable &unwind_table, Address addr, AddressRanges ranges) : m_unwind_table(unwind_table), m_addr(std::move(addr)), - m_ranges(std::move(ranges)), m_range(CollapseRanges(m_ranges)), - m_tried_unwind_plan_assembly(false), m_tried_unwind_plan_eh_frame(false), + m_ranges(std::move(ranges)), m_tried_unwind_plan_assembly(false), + m_tried_unwind_plan_eh_frame(false), m_tried_unwind_plan_object_file(false), m_tried_unwind_plan_debug_frame(false), m_tried_unwind_plan_object_file_augmented(false), @@ -106,8 +87,9 @@ FuncUnwinders::GetCompactUnwindUnwindPlan(Target &target) { return nullptr; m_tried_unwind_plan_compact_unwind = true; - if (m_range.GetBaseAddress().IsValid()) { - Address current_pc(m_range.GetBaseAddress()); + // Only continuous functions are supported. + if (m_ranges.size() == 1) { + Address current_pc(m_ranges[0].GetBaseAddress()); CompactUnwindInfo *compact_unwind = m_unwind_table.GetCompactUnwindInfo(); if (compact_unwind) { auto unwind_plan_sp = @@ -131,14 +113,10 @@ FuncUnwinders::GetObjectFileUnwindPlan(Target &target) { return m_unwind_plan_object_file_sp; m_tried_unwind_plan_object_file = true; - if (m_range.GetBaseAddress().IsValid()) { - CallFrameInfo *object_file_frame = m_unwind_table.GetObjectFileUnwindInfo(); - if (object_file_frame) { - auto plan_sp = std::make_shared<UnwindPlan>(lldb::eRegisterKindGeneric); - if (object_file_frame->GetUnwindPlan(m_range, *plan_sp)) - m_unwind_plan_object_file_sp = std::move(plan_sp); - } - } + if (CallFrameInfo *object_file_frame = + m_unwind_table.GetObjectFileUnwindInfo()) + m_unwind_plan_object_file_sp = + object_file_frame->GetUnwindPlan(m_ranges, m_addr); return m_unwind_plan_object_file_sp; } @@ -178,8 +156,9 @@ FuncUnwinders::GetArmUnwindUnwindPlan(Target &target) { return m_unwind_plan_arm_unwind_sp; m_tried_unwind_plan_arm_unwind = true; - if (m_range.GetBaseAddress().IsValid()) { - Address current_pc(m_range.GetBaseAddress()); + // Only continuous functions are supported. + if (m_ranges.size() == 1) { + Address current_pc = m_ranges[0].GetBaseAddress(); ArmUnwindInfo *arm_unwind_info = m_unwind_table.GetArmUnwindInfo(); if (arm_unwind_info) { auto plan_sp = std::make_shared<UnwindPlan>(lldb::eRegisterKindGeneric); @@ -215,9 +194,10 @@ FuncUnwinders::GetSymbolFileUnwindPlan(Thread &thread) { return m_unwind_plan_symbol_file_sp; m_tried_unwind_plan_symbol_file = true; - if (SymbolFile *symfile = m_unwind_table.GetSymbolFile()) { + if (SymbolFile *symfile = m_unwind_table.GetSymbolFile(); + symfile && m_ranges.size() == 1) { m_unwind_plan_symbol_file_sp = symfile->GetUnwindPlan( - m_range.GetBaseAddress(), + m_ranges[0].GetBaseAddress(), RegisterContextToInfo(*thread.GetRegisterContext())); } return m_unwind_plan_symbol_file_sp; @@ -242,10 +222,11 @@ FuncUnwinders::GetObjectFileAugmentedUnwindPlan(Target &target, // so the UnwindPlan can be used at any instruction in the function. UnwindAssemblySP assembly_profiler_sp(GetUnwindAssemblyProfiler(target)); - if (assembly_profiler_sp) { + // Only continuous functions are supported. + if (assembly_profiler_sp && m_ranges.size() == 1) { auto plan_sp = std::make_shared<UnwindPlan>(*object_file_unwind_plan); - if (assembly_profiler_sp->AugmentUnwindPlanFromCallSite(m_range, thread, + if (assembly_profiler_sp->AugmentUnwindPlanFromCallSite(m_ranges[0], thread, *plan_sp)) m_unwind_plan_object_file_augmented_sp = std::move(plan_sp); } @@ -280,9 +261,10 @@ FuncUnwinders::GetEHFrameAugmentedUnwindPlan(Target &target, Thread &thread) { // so the UnwindPlan can be used at any instruction in the function. UnwindAssemblySP assembly_profiler_sp(GetUnwindAssemblyProfiler(target)); - if (assembly_profiler_sp) { + // Only continuous functions are supported. + if (assembly_profiler_sp && m_ranges.size() == 1) { auto plan_sp = std::make_shared<UnwindPlan>(*eh_frame_plan); - if (assembly_profiler_sp->AugmentUnwindPlanFromCallSite(m_range, thread, + if (assembly_profiler_sp->AugmentUnwindPlanFromCallSite(m_ranges[0], thread, *plan_sp)) m_unwind_plan_eh_frame_augmented_sp = std::move(plan_sp); } @@ -319,10 +301,11 @@ FuncUnwinders::GetDebugFrameAugmentedUnwindPlan(Target &target, // function. UnwindAssemblySP assembly_profiler_sp(GetUnwindAssemblyProfiler(target)); - if (assembly_profiler_sp) { + // Only continuous functions are supported. + if (assembly_profiler_sp && m_ranges.size() == 1) { auto plan_sp = std::make_shared<UnwindPlan>(*debug_frame_plan); - if (assembly_profiler_sp->AugmentUnwindPlanFromCallSite(m_range, thread, + if (assembly_profiler_sp->AugmentUnwindPlanFromCallSite(m_ranges[0], thread, *plan_sp)) m_unwind_plan_debug_frame_augmented_sp = std::move(plan_sp); } @@ -339,18 +322,19 @@ FuncUnwinders::GetAssemblyUnwindPlan(Target &target, Thread &thread) { m_tried_unwind_plan_assembly = true; - // Don't analyze more than 10 megabytes of instructions, - // if a function is legitimately larger than that, we'll - // miss the epilogue instructions, but guard against a - // bogusly large function and analyzing large amounts of - // non-instruction data. - AddressRange range = m_range; - const addr_t func_size = - std::min(range.GetByteSize(), (addr_t)1024 * 10 * 10); - range.SetByteSize(func_size); - UnwindAssemblySP assembly_profiler_sp(GetUnwindAssemblyProfiler(target)); - if (assembly_profiler_sp) { + // Only continuous functions are supported. + if (assembly_profiler_sp && m_ranges.size() == 1) { + // Don't analyze more than 10 megabytes of instructions, + // if a function is legitimately larger than that, we'll + // miss the epilogue instructions, but guard against a + // bogusly large function and analyzing large amounts of + // non-instruction data. + AddressRange range = m_ranges[0]; + const addr_t func_size = + std::min(range.GetByteSize(), (addr_t)1024 * 10 * 10); + range.SetByteSize(func_size); + auto plan_sp = std::make_shared<UnwindPlan>(lldb::eRegisterKindGeneric); if (assembly_profiler_sp->GetNonCallSiteUnwindPlanFromAssembly( range, thread, *plan_sp)) @@ -457,9 +441,9 @@ FuncUnwinders::GetUnwindPlanFastUnwind(Target &target, Thread &thread) { m_tried_unwind_fast = true; UnwindAssemblySP assembly_profiler_sp(GetUnwindAssemblyProfiler(target)); - if (assembly_profiler_sp) { + if (assembly_profiler_sp && m_ranges.size() == 1) { auto plan_sp = std::make_shared<UnwindPlan>(lldb::eRegisterKindGeneric); - if (assembly_profiler_sp->GetFastUnwindPlan(m_range, thread, *plan_sp)) + if (assembly_profiler_sp->GetFastUnwindPlan(m_ranges[0], thread, *plan_sp)) m_unwind_plan_fast_sp = std::move(plan_sp); } return m_unwind_plan_fast_sp; @@ -503,19 +487,6 @@ FuncUnwinders::GetUnwindPlanArchitectureDefaultAtFunctionEntry(Thread &thread) { return m_unwind_plan_arch_default_at_func_entry_sp; } -Address &FuncUnwinders::GetFirstNonPrologueInsn(Target &target) { - std::lock_guard<std::recursive_mutex> guard(m_mutex); - if (m_first_non_prologue_insn.IsValid()) - return m_first_non_prologue_insn; - - ExecutionContext exe_ctx(target.shared_from_this(), false); - UnwindAssemblySP assembly_profiler_sp(GetUnwindAssemblyProfiler(target)); - if (assembly_profiler_sp) - assembly_profiler_sp->FirstNonPrologueInsn(m_range, exe_ctx, - m_first_non_prologue_insn); - return m_first_non_prologue_insn; -} - const Address &FuncUnwinders::GetFunctionStartAddress() const { return m_addr; } lldb::UnwindAssemblySP diff --git a/lldb/source/Target/RegisterContextUnwind.cpp b/lldb/source/Target/RegisterContextUnwind.cpp index 1884d8d53a7f6..32ed8e0384412 100644 --- a/lldb/source/Target/RegisterContextUnwind.cpp +++ b/lldb/source/Target/RegisterContextUnwind.cpp @@ -880,10 +880,9 @@ RegisterContextUnwind::GetFullUnwindPlanForFrame() { CallFrameInfo *object_file_unwind = pc_module_sp->GetUnwindTable().GetObjectFileUnwindInfo(); if (object_file_unwind) { - auto unwind_plan_sp = - std::make_shared<UnwindPlan>(lldb::eRegisterKindGeneric); - if (object_file_unwind->GetUnwindPlan(m_current_pc, *unwind_plan_sp)) - return unwind_plan_sp; + if (std::unique_ptr<UnwindPlan> plan_up = + object_file_unwind->GetUnwindPlan(m_current_pc)) + return plan_up; } return arch_default_unwind_plan_sp; diff --git a/lldb/unittests/ObjectFile/PECOFF/TestPECallFrameInfo.cpp b/lldb/unittests/ObjectFile/PECOFF/TestPECallFrameInfo.cpp index e842df5988867..264678e4c4ed3 100644 --- a/lldb/unittests/ObjectFile/PECOFF/TestPECallFrameInfo.cpp +++ b/lldb/unittests/ObjectFile/PECOFF/TestPECallFrameInfo.cpp @@ -24,12 +24,10 @@ using namespace lldb; class PECallFrameInfoTest : public testing::Test { SubsystemRAII<FileSystem, ObjectFilePECOFF> subsystems; - -protected: - void GetUnwindPlan(addr_t file_addr, UnwindPlan &plan) const; }; -void PECallFrameInfoTest::GetUnwindPlan(addr_t file_addr, UnwindPlan &plan) const { +static llvm::Expected<std::unique_ptr<UnwindPlan>> +GetUnwindPlan(addr_t file_addr) { llvm::Expected<TestFile> ExpectedFile = TestFile::fromYaml( R"( --- !COFF @@ -190,24 +188,34 @@ void PECallFrameInfoTest::GetUnwindPlan(addr_t file_addr, UnwindPlan &plan) cons symbols: [] ... )"); - ASSERT_THAT_EXPECTED(ExpectedFile, llvm::Succeeded()); + if (!ExpectedFile) + return ExpectedFile.takeError(); ModuleSP module_sp = std::make_shared<Module>(ExpectedFile->moduleSpec()); ObjectFile *object_file = module_sp->GetObjectFile(); - ASSERT_NE(object_file, nullptr); + if (!object_file) + return llvm::createStringError("object file is null"); std::unique_ptr<CallFrameInfo> cfi = object_file->CreateCallFrameInfo(); - ASSERT_NE(cfi.get(), nullptr); + if (!cfi) + return llvm::createStringError("call frame info is null"); SectionList *sect_list = object_file->GetSectionList(); - ASSERT_NE(sect_list, nullptr); - - EXPECT_TRUE(cfi->GetUnwindPlan(Address(file_addr, sect_list), plan)); + if (!sect_list) + return llvm::createStringError("section list is null"); + + std::unique_ptr<UnwindPlan> plan_up = + cfi->GetUnwindPlan(Address(file_addr, sect_list)); + if (!plan_up) + return llvm::createStringError("unwind plan is null"); + return plan_up; } TEST_F(PECallFrameInfoTest, Basic_eh) { - UnwindPlan plan(eRegisterKindLLDB); - GetUnwindPlan(0x1001080, plan); + llvm::Expected<std::unique_ptr<UnwindPlan>> expected_plan = + GetUnwindPlan(0x1001080); + ASSERT_THAT_EXPECTED(expected_plan, llvm::Succeeded()); + UnwindPlan &plan = **expected_plan; EXPECT_EQ(plan.GetRowCount(), 7); UnwindPlan::Row row; @@ -248,8 +256,10 @@ TEST_F(PECallFrameInfoTest, Basic_eh) { } TEST_F(PECallFrameInfoTest, Chained_eh) { - UnwindPlan plan(eRegisterKindLLDB); - GetUnwindPlan(0x1001180, plan); + llvm::Expected<std::unique_ptr<UnwindPlan>> expected_plan = + GetUnwindPlan(0x1001180); + ASSERT_THAT_EXPECTED(expected_plan, llvm::Succeeded()); + UnwindPlan &plan = **expected_plan; EXPECT_EQ(plan.GetRowCount(), 2); UnwindPlan::Row row; @@ -270,8 +280,10 @@ TEST_F(PECallFrameInfoTest, Chained_eh) { } TEST_F(PECallFrameInfoTest, Frame_reg_eh) { - UnwindPlan plan(eRegisterKindLLDB); - GetUnwindPlan(0x1001280, plan); + llvm::Expected<std::unique_ptr<UnwindPlan>> expected_plan = + GetUnwindPlan(0x1001280); + ASSERT_THAT_EXPECTED(expected_plan, llvm::Succeeded()); + UnwindPlan &plan = **expected_plan; EXPECT_EQ(plan.GetRowCount(), 11); UnwindPlan::Row row; `````````` </details> https://github.com/llvm/llvm-project/pull/140927 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
