https://github.com/JDevlieghere updated https://github.com/llvm/llvm-project/pull/215393
>From d1e68be2a8a60d4a63e265b6acb4b8cace6f4f60 Mon Sep 17 00:00:00 2001 From: Jonas Devlieghere <[email protected]> Date: Wed, 12 Aug 2026 22:06:40 -0700 Subject: [PATCH 1/3] [DWARFLinker] Constrain a function's high_pc to its own symbol Mach-O objects built with .subsections_via_symbols make every symbol an independently placeable atom, and the linker packs atoms without preserving the spacing they had in the object file. I have an example where the compiler describes such a subprogram as extending past its own atom. While it's debatable whether that's a good idea, it's not invalid in the object file. However, once linked, it is invalid. We can make dsymutil resilient against this by looking at the size of the symbol in the debug map and adjusting the end_pc. I'm doing so conservatively so that only a collision is repaired. Already overlapping/invalid ranges remain untouched. rdar://184768778 --- llvm/include/llvm/DWARFLinker/AddressesMap.h | 41 ++++++++-- llvm/lib/DWARFLinker/Classic/DWARFLinker.cpp | 23 +++++- .../Parallel/DIEAttributeCloner.cpp | 16 ++++ .../Parallel/DependencyTracker.cpp | 13 +++- .../subprogram-high-pc-past-symbol-dwarf2.s | 72 ++++++++++++++++++ .../Inputs/subprogram-high-pc-past-symbol.s | 76 +++++++++++++++++++ .../subprogram-high-pc-past-symbol.test | 57 ++++++++++++++ llvm/tools/dsymutil/DwarfLinkerForBinary.h | 31 ++++++-- 8 files changed, 309 insertions(+), 20 deletions(-) create mode 100644 llvm/test/tools/dsymutil/Inputs/subprogram-high-pc-past-symbol-dwarf2.s create mode 100644 llvm/test/tools/dsymutil/Inputs/subprogram-high-pc-past-symbol.s create mode 100644 llvm/test/tools/dsymutil/subprogram-high-pc-past-symbol.test diff --git a/llvm/include/llvm/DWARFLinker/AddressesMap.h b/llvm/include/llvm/DWARFLinker/AddressesMap.h index 38b9a67b124c1..443e6f5bb4186 100644 --- a/llvm/include/llvm/DWARFLinker/AddressesMap.h +++ b/llvm/include/llvm/DWARFLinker/AddressesMap.h @@ -83,22 +83,47 @@ class AddressesMap { /// Erases all data. virtual void clear() = 0; - /// This is used for assembly files where labels may not have high_pc - /// but the debug map has range information from symbols. - struct AssemblyRange { - AssemblyRange(uint64_t LowPC, uint64_t HighPC) + /// The extent the linker gave a symbol, in source address space. + struct SymbolRange { + SymbolRange(uint64_t LowPC, uint64_t HighPC) : LowPC(LowPC), HighPC(HighPC) {} uint64_t LowPC; uint64_t HighPC; }; - /// Returns the address range containing \p Addr if available. - /// \returns the range [LowPC, HighPC) containing Addr. - virtual std::optional<AssemblyRange> - getAssemblyRangeForAddress(uint64_t Addr) { + /// Returns the symbol range [LowPC, HighPC) containing \p Addr, if known. + virtual std::optional<SymbolRange> getSymbolRangeForAddress(uint64_t Addr) { return std::nullopt; } + /// Returns the linked address of the first symbol placed at or after + /// \p LinkedAddr, if one is known. + virtual std::optional<uint64_t> + getNextLinkedSymbolStart(uint64_t LinkedAddr) { + return std::nullopt; + } + + /// Constrains the source-space end of a code range, given its start \p LowPC, + /// its end \p HighPC as an address, and the \p Adjustment all of its + /// addresses shift by in the output. + /// + /// Neighbouring symbols shift by different amounts, so a range reaching past + /// the symbol holding its start can land inside the next symbol in the + /// output. Only that collision is repaired. Coverage that overlaps nothing is + /// left alone, and a symbol nested in the same extent is never a neighbour, + /// so it cannot shorten a range that legitimately spans it. + uint64_t constrainCodeRangeHighPC(uint64_t LowPC, uint64_t HighPC, + int64_t Adjustment) { + std::optional<SymbolRange> Symbol = getSymbolRangeForAddress(LowPC); + if (!Symbol) + return HighPC; + std::optional<uint64_t> NextStart = + getNextLinkedSymbolStart(Symbol->HighPC + Adjustment); + if (!NextStart) + return HighPC; + return std::min(HighPC, *NextStart - Adjustment); + } + /// This function checks whether variable has DWARF expression containing /// operation referencing live address(f.e. DW_OP_addr, DW_OP_addrx...). /// \returns first is true if the expression has an operation referencing an diff --git a/llvm/lib/DWARFLinker/Classic/DWARFLinker.cpp b/llvm/lib/DWARFLinker/Classic/DWARFLinker.cpp index 24289e0906586..5c8f718c8670f 100644 --- a/llvm/lib/DWARFLinker/Classic/DWARFLinker.cpp +++ b/llvm/lib/DWARFLinker/Classic/DWARFLinker.cpp @@ -672,7 +672,7 @@ unsigned DWARFLinker::shouldKeepSubprogramDIE( // function ranges when available, falling back to labels otherwise. if (Unit.getLanguage() == dwarf::DW_LANG_Mips_Assembler || Unit.getLanguage() == dwarf::DW_LANG_Assembly) { - if (auto Range = RelocMgr.getAssemblyRangeForAddress(*LowPc)) { + if (auto Range = RelocMgr.getSymbolRangeForAddress(*LowPc)) { Unit.addFunctionRange(Range->LowPC, Range->HighPC, MyInfo.AddrAdjust); } else { Unit.addLabelLowPc(*LowPc, MyInfo.AddrAdjust); @@ -698,7 +698,10 @@ unsigned DWARFLinker::shouldKeepSubprogramDIE( } // Replace the debug map range with a more accurate one. - Unit.addFunctionRange(*LowPc, *HighPc, MyInfo.AddrAdjust); + Unit.addFunctionRange( + *LowPc, + RelocMgr.constrainCodeRangeHighPC(*LowPc, *HighPc, MyInfo.AddrAdjust), + MyInfo.AddrAdjust); return Flags; } @@ -1471,6 +1474,14 @@ unsigned DWARFLinker::DIECloner::cloneAddressAttribute( else return 0; } else { + // A nested scope inherits the range its parent function overran, so every + // range is constrained, not just the subprogram's own. + if (AttrSpec.Attr == dwarf::DW_AT_high_pc) { + if (std::optional<uint64_t> LowPC = + dwarf::toAddress(InputDIE.find(dwarf::DW_AT_low_pc))) + Addr = ObjFile.Addresses->constrainCodeRangeHighPC(*LowPC, *Addr, + Info.PCOffset); + } *Addr += Info.PCOffset; } @@ -1629,6 +1640,14 @@ unsigned DWARFLinker::DIECloner::cloneScalarAttribute( return 0; } + if (AttrSpec.Attr == dwarf::DW_AT_high_pc) { + if (std::optional<uint64_t> LowPC = + dwarf::toAddress(InputDIE.find(dwarf::DW_AT_low_pc))) + Value = File.Addresses->constrainCodeRangeHighPC(*LowPC, *LowPC + Value, + Info.PCOffset) - + *LowPC; + } + DIE::value_iterator Patch = Die.addValue(DIEAlloc, dwarf::Attribute(AttrSpec.Attr), dwarf::Form(AttrSpec.Form), DIEInteger(Value)); diff --git a/llvm/lib/DWARFLinker/Parallel/DIEAttributeCloner.cpp b/llvm/lib/DWARFLinker/Parallel/DIEAttributeCloner.cpp index f58869d20291c..77a1d85edb772 100644 --- a/llvm/lib/DWARFLinker/Parallel/DIEAttributeCloner.cpp +++ b/llvm/lib/DWARFLinker/Parallel/DIEAttributeCloner.cpp @@ -525,6 +525,16 @@ size_t DIEAttributeCloner::cloneScalarAttr( !OutUnit.isCompileUnit()) return 0; + // A nested scope inherits the range its parent function overran, so every + // range is constrained, not just the subprogram's own. + if (AttrSpec.Attr == dwarf::DW_AT_high_pc && FuncAddressAdjustment) { + if (std::optional<uint64_t> LowPC = + dwarf::toAddress(InUnit.find(InputDieEntry, dwarf::DW_AT_low_pc))) + Value = InUnit.getContaingFile().Addresses->constrainCodeRangeHighPC( + *LowPC, *LowPC + Value, *FuncAddressAdjustment) - + *LowPC; + } + auto Result = Generator.addScalarAttribute(AttrSpec.Attr, ResultingForm, Value); // Record DW_AT_LLVM_stmt_sequence so the attribute value can be @@ -679,6 +689,12 @@ size_t DIEAttributeCloner::cloneAddressAttr( else return 0; } else { + if (AttrSpec.Attr == dwarf::DW_AT_high_pc && FuncAddressAdjustment) { + if (std::optional<uint64_t> LowPC = + dwarf::toAddress(InUnit.find(InputDieEntry, dwarf::DW_AT_low_pc))) + Addr = InUnit.getContaingFile().Addresses->constrainCodeRangeHighPC( + *LowPC, *Addr, *FuncAddressAdjustment); + } if (VarAddressAdjustment) *Addr += *VarAddressAdjustment; else if (FuncAddressAdjustment) diff --git a/llvm/lib/DWARFLinker/Parallel/DependencyTracker.cpp b/llvm/lib/DWARFLinker/Parallel/DependencyTracker.cpp index 124e73b1a15bd..0993497ff080b 100644 --- a/llvm/lib/DWARFLinker/Parallel/DependencyTracker.cpp +++ b/llvm/lib/DWARFLinker/Parallel/DependencyTracker.cpp @@ -952,14 +952,15 @@ bool DependencyTracker::isLiveSubprogramEntry(const UnitEntryPairTy &Entry) { // For assembly-language CUs there are typically no DW_TAG_subprogram // DIEs, so labels are the only addresses we see. Fall back to the - // assembly-range lookup to recover a function range for the line-table + // symbol-range lookup to recover a function range for the line-table // filter; otherwise the output line table would be empty. uint16_t Language = dwarf::toUnsigned( Entry.CU->getOrigUnit().getUnitDIE().find(dwarf::DW_AT_language), 0); if (Language == dwarf::DW_LANG_Mips_Assembler || Language == dwarf::DW_LANG_Assembly) { - if (auto Range = Entry.CU->getContaingFile() - .Addresses->getAssemblyRangeForAddress(*LowPc)) + if (auto Range = + Entry.CU->getContaingFile().Addresses->getSymbolRangeForAddress( + *LowPc)) Entry.CU->addFunctionRange(Range->LowPC, Range->HighPC, *RelocAdjustment); } @@ -974,6 +975,10 @@ bool DependencyTracker::isLiveSubprogramEntry(const UnitEntryPairTy &Entry) { if (!Info.getTrackLiveness() || DIE.getTag() == dwarf::DW_TAG_label) return true; - Entry.CU->addFunctionRange(*LowPc, *HighPc, *RelocAdjustment); + Entry.CU->addFunctionRange( + *LowPc, + Entry.CU->getContaingFile().Addresses->constrainCodeRangeHighPC( + *LowPc, *HighPc, *RelocAdjustment), + *RelocAdjustment); return true; } diff --git a/llvm/test/tools/dsymutil/Inputs/subprogram-high-pc-past-symbol-dwarf2.s b/llvm/test/tools/dsymutil/Inputs/subprogram-high-pc-past-symbol-dwarf2.s new file mode 100644 index 0000000000000..49933495d7dc6 --- /dev/null +++ b/llvm/test/tools/dsymutil/Inputs/subprogram-high-pc-past-symbol-dwarf2.s @@ -0,0 +1,72 @@ +; The DWARF 2 form of subprogram-high-pc-past-symbol.s, where DW_AT_high_pc is +; an address rather than a length. + + .text + .globl _a + .p2align 2 +_a: + ret +_filler: + nop + .globl _b + .p2align 2 +_b: + ret + + .section __DWARF,__debug_abbrev,regular,debug + .byte 1 ; abbrev 1: DW_TAG_compile_unit + .byte 0x11 + .byte 1 ; DW_CHILDREN_yes + .byte 0x25, 0x08 ; DW_AT_producer, DW_FORM_string + .byte 0x13, 0x0b ; DW_AT_language, DW_FORM_data1 + .byte 0x03, 0x08 ; DW_AT_name, DW_FORM_string + .byte 0x11, 0x01 ; DW_AT_low_pc, DW_FORM_addr + .byte 0x12, 0x01 ; DW_AT_high_pc, DW_FORM_addr + .byte 0, 0 + .byte 2 ; abbrev 2: DW_TAG_subprogram + .byte 0x2e + .byte 1 ; DW_CHILDREN_yes + .byte 0x03, 0x08 ; DW_AT_name, DW_FORM_string + .byte 0x11, 0x01 ; DW_AT_low_pc, DW_FORM_addr + .byte 0x12, 0x01 ; DW_AT_high_pc, DW_FORM_addr + .byte 0x3f, 0x0c ; DW_AT_external, DW_FORM_flag + .byte 0, 0 + .byte 3 ; abbrev 3: DW_TAG_lexical_block + .byte 0x0b + .byte 0 ; DW_CHILDREN_no + .byte 0x11, 0x01 ; DW_AT_low_pc, DW_FORM_addr + .byte 0x12, 0x01 ; DW_AT_high_pc, DW_FORM_addr + .byte 0, 0 + .byte 0 + + .section __DWARF,__debug_info,regular,debug +Lcu_begin: + .long Lcu_end-Lcu_version +Lcu_version: + .short 2 + .long 0 + .byte 8 + .byte 1 ; DW_TAG_compile_unit + .asciz "hand-written" + .byte 0x0c ; DW_LANG_C99 + .asciz "t.c" + .quad _a + .quad 0xc ; _a, _filler and _b together + .byte 2 ; DW_TAG_subprogram "a" + .asciz "a" + .quad _a + .quad 0x8 ; four bytes past the end of _a + .byte 1 + .byte 3 ; DW_TAG_lexical_block in "a" + .quad _a + .quad 0x8 ; reaches as far as its parent + .byte 0 ; end of "a"'s children + .byte 2 ; DW_TAG_subprogram "b" + .asciz "b" + .quad _b + .quad 0xc + .byte 1 + .byte 0 ; end of "b"'s children + .byte 0 ; end of the compile unit's children +Lcu_end: + .subsections_via_symbols diff --git a/llvm/test/tools/dsymutil/Inputs/subprogram-high-pc-past-symbol.s b/llvm/test/tools/dsymutil/Inputs/subprogram-high-pc-past-symbol.s new file mode 100644 index 0000000000000..f7d02714a2940 --- /dev/null +++ b/llvm/test/tools/dsymutil/Inputs/subprogram-high-pc-past-symbol.s @@ -0,0 +1,76 @@ +; _filler is an unreferenced atom between the two functions, so the linker drops +; it and places _b four bytes after _a. DW_AT_high_pc for _a reaches all the way +; to _b, past the code the linker keeps for it, and the lexical block inside _a +; ends with its parent. +; +; The DWARF is hand written because a producer normally agrees with the linker. + + .text + .globl _a + .p2align 2 +_a: + ret +_filler: + nop + .globl _b + .p2align 2 +_b: + ret + + .section __DWARF,__debug_abbrev,regular,debug + .byte 1 ; abbrev 1: DW_TAG_compile_unit + .byte 0x11 + .byte 1 ; DW_CHILDREN_yes + .byte 0x25, 0x08 ; DW_AT_producer, DW_FORM_string + .byte 0x13, 0x0b ; DW_AT_language, DW_FORM_data1 + .byte 0x03, 0x08 ; DW_AT_name, DW_FORM_string + .byte 0x11, 0x01 ; DW_AT_low_pc, DW_FORM_addr + .byte 0x12, 0x06 ; DW_AT_high_pc, DW_FORM_data4 + .byte 0, 0 + .byte 2 ; abbrev 2: DW_TAG_subprogram + .byte 0x2e + .byte 1 ; DW_CHILDREN_yes + .byte 0x03, 0x08 ; DW_AT_name, DW_FORM_string + .byte 0x11, 0x01 ; DW_AT_low_pc, DW_FORM_addr + .byte 0x12, 0x06 ; DW_AT_high_pc, DW_FORM_data4 + .byte 0x3f, 0x0c ; DW_AT_external, DW_FORM_flag + .byte 0, 0 + .byte 3 ; abbrev 3: DW_TAG_lexical_block + .byte 0x0b + .byte 0 ; DW_CHILDREN_no + .byte 0x11, 0x01 ; DW_AT_low_pc, DW_FORM_addr + .byte 0x12, 0x06 ; DW_AT_high_pc, DW_FORM_data4 + .byte 0, 0 + .byte 0 + + .section __DWARF,__debug_info,regular,debug +Lcu_begin: + .long Lcu_end-Lcu_version +Lcu_version: + .short 4 + .long 0 + .byte 8 + .byte 1 ; DW_TAG_compile_unit + .asciz "hand-written" + .byte 0x0c ; DW_LANG_C99 + .asciz "t.c" + .quad _a + .long 0xc ; _a, _filler and _b together + .byte 2 ; DW_TAG_subprogram "a" + .asciz "a" + .quad _a + .long 0x8 ; four bytes past the end of _a + .byte 1 + .byte 3 ; DW_TAG_lexical_block in "a" + .quad _a + .long 0x8 ; reaches as far as its parent + .byte 0 ; end of "a"'s children + .byte 2 ; DW_TAG_subprogram "b" + .asciz "b" + .quad _b + .long 0x4 + .byte 1 + .byte 0 ; end of "b"'s children + .byte 0 ; end of the compile unit's children +Lcu_end: + .subsections_via_symbols diff --git a/llvm/test/tools/dsymutil/subprogram-high-pc-past-symbol.test b/llvm/test/tools/dsymutil/subprogram-high-pc-past-symbol.test new file mode 100644 index 0000000000000..c58f75b0f9159 --- /dev/null +++ b/llvm/test/tools/dsymutil/subprogram-high-pc-past-symbol.test @@ -0,0 +1,57 @@ +# REQUIRES: aarch64-registered-target + +# A DW_AT_high_pc reaching past the code the linker kept for a function must be +# cut short at the function the linker placed next, otherwise the two overlap in +# the output and verification fails with "DIEs have overlapping address ranges". +# A scope nested in the function inherits the overrun, so it has to be cut short +# with its parent to stay inside it. + +# RUN: llvm-mc -triple arm64-apple-darwin -filetype=obj \ +# RUN: %p/Inputs/subprogram-high-pc-past-symbol.s -o %t.o +# RUN: echo '---' > %t.map +# RUN: echo "triple: 'arm64-apple-darwin'" >> %t.map +# RUN: echo 'objects:' >> %t.map +# RUN: echo " - filename: '%/t.o'" >> %t.map +# RUN: echo ' symbols:' >> %t.map +# RUN: echo ' - { sym: _a, objAddr: 0x0, binAddr: 0x1000, size: 0x4 }' >> %t.map +# RUN: echo ' - { sym: _b, objAddr: 0x8, binAddr: 0x1004, size: 0x4 }' >> %t.map + +# _inside starts within the code the linker kept for _a, so it is nested rather +# than adjacent and must not cut _a short. + +# RUN: echo ' - { sym: _inside, binAddr: 0x1002, size: 0x2 }' >> %t.map +# RUN: echo '...' >> %t.map + +# RUN: dsymutil --linker classic -y %t.map -f -o %t-classic.out +# RUN: llvm-dwarfdump -a %t-classic.out | FileCheck %s +# RUN: llvm-dwarfdump --verify %t-classic.out | FileCheck %s --check-prefix=VERIFY + +# RUN: dsymutil --linker parallel -y %t.map -f -o %t-parallel.out +# RUN: llvm-dwarfdump -a %t-parallel.out | FileCheck %s +# RUN: llvm-dwarfdump --verify %t-parallel.out | FileCheck %s --check-prefix=VERIFY + +# Same again with DW_AT_high_pc as an address rather than a length. + +# RUN: llvm-mc -triple arm64-apple-darwin -filetype=obj \ +# RUN: %p/Inputs/subprogram-high-pc-past-symbol-dwarf2.s -o %t.o +# RUN: dsymutil --linker classic -y %t.map -f -o %t-classic2.out +# RUN: llvm-dwarfdump -a %t-classic2.out | FileCheck %s +# RUN: llvm-dwarfdump --verify %t-classic2.out | FileCheck %s --check-prefix=VERIFY + +# RUN: dsymutil --linker parallel -y %t.map -f -o %t-parallel2.out +# RUN: llvm-dwarfdump -a %t-parallel2.out | FileCheck %s +# RUN: llvm-dwarfdump --verify %t-parallel2.out | FileCheck %s --check-prefix=VERIFY + +# CHECK: DW_TAG_subprogram +# CHECK: DW_AT_name{{.*}}"a" +# CHECK-NEXT: DW_AT_low_pc{{.*}}(0x0000000000001000) +# CHECK-NEXT: DW_AT_high_pc{{.*}}(0x0000000000001004) +# CHECK: DW_TAG_lexical_block +# CHECK-NEXT: DW_AT_low_pc{{.*}}(0x0000000000001000) +# CHECK-NEXT: DW_AT_high_pc{{.*}}(0x0000000000001004) +# CHECK: DW_TAG_subprogram +# CHECK: DW_AT_name{{.*}}"b" +# CHECK-NEXT: DW_AT_low_pc{{.*}}(0x0000000000001004) +# CHECK-NEXT: DW_AT_high_pc{{.*}}(0x0000000000001008) + +# VERIFY: No errors. diff --git a/llvm/tools/dsymutil/DwarfLinkerForBinary.h b/llvm/tools/dsymutil/DwarfLinkerForBinary.h index 446860abe150c..5b422e065a430 100644 --- a/llvm/tools/dsymutil/DwarfLinkerForBinary.h +++ b/llvm/tools/dsymutil/DwarfLinkerForBinary.h @@ -128,6 +128,9 @@ class DwarfLinkerForBinary { /// Address ranges for symbols with sizes (used for assembly file support). RangesTy AddressRanges; + /// Sorted linked start addresses of the symbols with a known size. + std::vector<uint64_t> LinkedSymbolStarts; + /// Returns list of valid relocations from \p Relocs, /// between \p StartOffset and \p NextOffset. /// @@ -172,15 +175,22 @@ class DwarfLinkerForBinary { } else { findValidRelocsInDebugSections(Obj, DMO); } - // Populate address ranges from debug map symbols that have sizes. - // This is used for assembly files where labels may not have high_pc. + // A sizeless symbol has no known extent, so it can bound neither a range + // of its own nor a neighbour's. The ranges stand in for the high_pc that + // assembly files lack. for (const auto &Entry : DMO.symbols()) { const auto &Mapping = Entry.getValue(); - if (Mapping.Size && Mapping.ObjectAddress) + if (!Mapping.Size) + continue; + LinkedSymbolStarts.push_back(Mapping.BinaryAddress); + if (Mapping.ObjectAddress) AddressRanges.insert( {*Mapping.ObjectAddress, *Mapping.ObjectAddress + Mapping.Size}, int64_t(Mapping.BinaryAddress) - *Mapping.ObjectAddress); } + llvm::sort(LinkedSymbolStarts); + LinkedSymbolStarts.erase(llvm::unique(LinkedSymbolStarts), + LinkedSymbolStarts.end()); } ~AddressManager() override { clear(); } @@ -241,14 +251,23 @@ class DwarfLinkerForBinary { ValidDebugInfoRelocs.clear(); ValidDebugAddrRelocs.clear(); AddressRanges.clear(); + LinkedSymbolStarts.clear(); } - std::optional<AssemblyRange> - getAssemblyRangeForAddress(uint64_t Addr) override { + std::optional<SymbolRange> + getSymbolRangeForAddress(uint64_t Addr) override { if (auto Range = AddressRanges.getRangeThatContains(Addr)) - return AssemblyRange(Range->Range.start(), Range->Range.end()); + return SymbolRange(Range->Range.start(), Range->Range.end()); return std::nullopt; } + + std::optional<uint64_t> + getNextLinkedSymbolStart(uint64_t LinkedAddr) override { + auto It = llvm::lower_bound(LinkedSymbolStarts, LinkedAddr); + if (It == LinkedSymbolStarts.end()) + return std::nullopt; + return *It; + } }; private: >From 6e4ba0a451d19ce508ae7eb4285a523f0293aaae Mon Sep 17 00:00:00 2001 From: Jonas Devlieghere <[email protected]> Date: Mon, 10 Aug 2026 12:52:58 -0700 Subject: [PATCH 2/3] [lldb] Search for a corefile's images before loading any of them A userland or kernel corefile can list hundreds of images, and searching for one can shell out to a symbol server or fetch over the network. Searching for them one at a time is where loading such a corefile spends its time. Add a batch form of SymbolLocator::Locate that runs the searches on the debugger's thread pool, gated on target.parallel-module-load. Results come back in the order the requests were given, since that order decides the Target's module order. Only the results are ordered, and anything a search reports to the user arrives in whatever order the searches finish in. Only the plugin searches run concurrently, so a platform hook does not have to be thread safe to take part, and reading a binary's UUID out of memory stays on the calling thread. Setting up a platform binary can replace the Target's platform and dynamic loader, and now happens for every image before any of them is searched for, so the platform a corefile asks for is the one all of its images are searched with. Previously the images listed ahead of a platform binary were searched with whatever platform preceded it. Assisted-by: Claude --- lldb/include/lldb/Symbol/SymbolLocator.h | 19 +++ lldb/source/Core/DynamicLoader.cpp | 52 ++++--- .../ObjectFile/Mach-O/ObjectFileMachO.cpp | 143 +++++++++-------- lldb/source/Symbol/SymbolLocator.cpp | 87 +++++++++-- .../TestMultipleBinaryCorefile.py | 25 +++ lldb/unittests/Symbol/SymbolLocatorTest.cpp | 145 +++++++++++++++++- 6 files changed, 372 insertions(+), 99 deletions(-) diff --git a/lldb/include/lldb/Symbol/SymbolLocator.h b/lldb/include/lldb/Symbol/SymbolLocator.h index 5fc9c161af50a..93915d2321c8a 100644 --- a/lldb/include/lldb/Symbol/SymbolLocator.h +++ b/lldb/include/lldb/Symbol/SymbolLocator.h @@ -15,10 +15,13 @@ #include "lldb/Utility/Status.h" #include "lldb/Utility/UUID.h" +#include "llvm/ADT/ArrayRef.h" #include "llvm/Support/Error.h" #include <optional> +#include <string> #include <system_error> +#include <vector> namespace lldb_private { @@ -49,6 +52,9 @@ class SymbolLocator : public PluginInterface { /// Allow contacting an external symbol server when the local searches come /// up empty. bool external_lookup = false; + + /// How to name this binary in a progress report. + std::string description; }; /// What a search found. @@ -82,12 +88,25 @@ class SymbolLocator : public PluginInterface { static llvm::Expected<Result> Locate(const Request &request, const FileSpecList &search_paths); + /// The platform hooks run on the calling thread, in order. Only the plugin + /// searches may run concurrently. + /// + /// \return One result per request, in the order the requests were given. + static std::vector<llvm::Expected<Result>> + Locate(llvm::ArrayRef<Request> requests, const FileSpecList &search_paths, + bool parallel); + /// Locate the symbol file for the given UUID on a background thread. This /// function returns immediately. Under the hood it uses the debugger's /// thread pool to call DownloadObjectAndSymbolFile. If a symbol file is /// found, this will notify all target which contain the module with the /// given UUID. static void DownloadSymbolFileAsync(const UUID &uuid); + +private: + /// Must stay callable concurrently. + static llvm::Expected<Result> + LocateWithPlugins(const Request &request, const FileSpecList &search_paths); }; } // namespace lldb_private diff --git a/lldb/source/Core/DynamicLoader.cpp b/lldb/source/Core/DynamicLoader.cpp index a948fefa4c83e..4178177a995a7 100644 --- a/lldb/source/Core/DynamicLoader.cpp +++ b/lldb/source/Core/DynamicLoader.cpp @@ -13,7 +13,6 @@ #include "lldb/Core/ModuleList.h" #include "lldb/Core/ModuleSpec.h" #include "lldb/Core/PluginManager.h" -#include "lldb/Core/Progress.h" #include "lldb/Core/Section.h" #include "lldb/Symbol/ObjectFile.h" #include "lldb/Symbol/SymbolLocator.h" @@ -26,10 +25,13 @@ #include "lldb/Utility/Log.h" #include "lldb/lldb-private-interfaces.h" +#include "llvm/ADT/STLExtras.h" +#include "llvm/ADT/SmallVector.h" #include "llvm/ADT/StringRef.h" #include "llvm/Support/Error.h" #include <memory> +#include <optional> #include <string> #include <cassert> @@ -245,13 +247,11 @@ GetBinaryNotFoundMessage(const DynamicLoader::BinarySpec &bin_spec) { return msg.GetString().str(); } -/// Search for a binary with a known UUID, and create a module for it. +/// Reads the Target, so it has to be called for one binary at a time. /// -/// Does not mutate the Target, but does read from it, and reaches the global -/// shared module list, the symbol locator plugins, and a locate module callback -/// the user may have installed. -static void SearchForBinary(Target &target, DynamicLoader::BinarySpec &bin_spec, - const FileSpecList &search_paths) { +/// \return What to search for, or nothing when the binary is already in hand. +static std::optional<SymbolLocator::Request> +PrepareSearch(Target &target, DynamicLoader::BinarySpec &bin_spec) { ModuleSpec module_spec; module_spec.SetTarget(target.shared_from_this()); module_spec.GetUUID() = bin_spec.uuid; @@ -266,19 +266,22 @@ static void SearchForBinary(Target &target, DynamicLoader::BinarySpec &bin_spec, /*invoke_locate_callback=*/true, /*invoke_symbol_locators=*/false); if (bin_spec.module_sp && bin_spec.module_sp->GetSymbolFileFileSpec()) - return; + return std::nullopt; - // Search for the binary and its symbols. SymbolLocator::Request request; request.module_spec = module_spec; request.platform = target.GetPlatform(); request.external_lookup = bin_spec.force_symbol_search; + request.description = GetBinaryDescription(bin_spec); + return request; +} - llvm::Expected<SymbolLocator::Result> located = - SymbolLocator::Locate(request, search_paths); +/// The module is not registered with the Target until LoadBinaryInTarget. +static void FinishSearch(DynamicLoader::BinarySpec &bin_spec, + llvm::Expected<SymbolLocator::Result> located) { if (!located) { - // This function's caller names the binary it could not find, so a plain - // miss needs nothing added to it. An explanation from a symbol server does. + // Only an explanation from a symbol server adds anything to the miss the + // caller already reports. llvm::Error error = located.takeError(); if (error.isA<SymbolLocator::NotFound>()) llvm::consumeError(std::move(error)); @@ -287,14 +290,9 @@ static void SearchForBinary(Target &target, DynamicLoader::BinarySpec &bin_spec, return; } - // A binary was found. Its symbols are another matter, and the caller reports - // that in its own order. if (located->symbol_error) bin_spec.error = Status::FromError(std::move(*located->symbol_error)); - // Create a module for what was found, sharing it with any other Target that - // asks for the same binary. The module is not registered with this Target - // until LoadBinaryInTarget. The locators have run, so don't run them again. ModuleSP located_module_sp; ModuleList::GetSharedModule(located->module_spec, located_module_sp, nullptr, nullptr, /*invoke_locate_callback=*/false, @@ -324,14 +322,28 @@ void DynamicLoader::LocateBinaries( Target &target = process->GetTarget(); const FileSpecList search_paths = Target::GetDefaultDebugFileSearchPaths(); + // Has to happen on this thread, and before any search, so that a binary whose + // UUID is not known yet still joins the batch. + llvm::SmallVector<BinarySpec *> to_search; + std::vector<SymbolLocator::Request> requests; for (BinarySpec &bin_spec : bin_specs) { if (!bin_spec.uuid.IsValid() && !bin_spec.value_is_offset) FindBinaryUUIDInMemory(process, bin_spec); if (!bin_spec.uuid.IsValid()) continue; - Progress progress("Locating binary", GetBinaryDescription(bin_spec)); - SearchForBinary(target, bin_spec, search_paths); + if (std::optional<SymbolLocator::Request> request = + PrepareSearch(target, bin_spec)) { + to_search.push_back(&bin_spec); + requests.push_back(std::move(*request)); + } } + + std::vector<llvm::Expected<SymbolLocator::Result>> located = + SymbolLocator::Locate(requests, search_paths, + target.GetParallelModuleLoad()); + + for (auto [bin_spec, result] : llvm::zip_equal(to_search, located)) + FinishSearch(*bin_spec, std::move(result)); } llvm::Expected<ModuleSP> diff --git a/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp b/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp index 5a31f16c9a729..01f56f0c3ee17 100644 --- a/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp +++ b/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp @@ -6674,17 +6674,21 @@ ObjectFileMachO::GetCorefileAllImageInfos() { bool ObjectFileMachO::LoadCoreFileImages(lldb_private::Process &process) { MachOCorefileAllImageInfos image_infos = GetCorefileAllImageInfos(); Log *log = GetLog(LLDBLog::Object | LLDBLog::DynamicLoader); - Status error; bool found_platform_binary = false; ModuleList added_modules; - for (MachOCorefileImageEntry &image : image_infos.all_image_infos) { - ModuleSP module_sp, local_filesystem_module_sp; + llvm::SmallVector<const MachOCorefileImageEntry *> pending_images; + std::vector<DynamicLoader::BinarySpec> pending_specs; + + for (MachOCorefileImageEntry &image : image_infos.all_image_infos) { // If this is a platform binary, it has been loaded (or registered with // the DynamicLoader to be loaded), we don't need to do any further // processing. We're not going to call ModulesDidLoad on this in this // method, so notify==true. + // + // Setting one up can replace the Target's platform and dynamic loader, so + // no image is searched for until this loop has run to the end. if (process.GetTarget() .GetDebugger() .GetPlatformList() @@ -6708,74 +6712,85 @@ bool ObjectFileMachO::LoadCoreFileImages(lldb_private::Process &process) { // We have either a UUID, or we have a load address which // and can try to read load commands and find a UUID. - if (image.uuid.IsValid() || - (!value_is_offset && value != LLDB_INVALID_ADDRESS)) { - DynamicLoader::BinarySpec bin_spec; - bin_spec.name = image.filename; - bin_spec.uuid = image.uuid; - bin_spec.value = value; - bin_spec.value_is_offset = value_is_offset; - bin_spec.force_symbol_search = image.currently_executing; - bin_spec.notify = false; - // Userland Darwin binaries will have segment load addresses via - // the `all image infos` LC_NOTE. - bin_spec.set_address_in_target = image.segment_load_addresses.empty(); - bin_spec.allow_memory_image_last_resort = - !image.segment_load_addresses.empty(); - if (llvm::Expected<ModuleSP> located = - DynamicLoader::LocateAndLoadBinary(&process, bin_spec)) { - module_sp = *located; - } else if (bin_spec.force_symbol_search) { - *process.GetTarget().GetDebugger().GetAsyncErrorStream() - << llvm::toString(located.takeError()) << "\n"; - } else { - // A corefile image that isn't on this machine is routine, and - // LocateAndLoadBinary has already logged it. - llvm::consumeError(located.takeError()); - } + if (!image.uuid.IsValid() && + (value_is_offset || value == LLDB_INVALID_ADDRESS)) + continue; + + DynamicLoader::BinarySpec bin_spec; + bin_spec.name = image.filename; + bin_spec.uuid = image.uuid; + bin_spec.value = value; + bin_spec.value_is_offset = value_is_offset; + bin_spec.force_symbol_search = image.currently_executing; + bin_spec.notify = false; + // Userland Darwin binaries will have segment load addresses via + // the `all image infos` LC_NOTE. + bin_spec.set_address_in_target = image.segment_load_addresses.empty(); + bin_spec.allow_memory_image_last_resort = + !image.segment_load_addresses.empty(); + + pending_images.push_back(&image); + pending_specs.push_back(std::move(bin_spec)); + } + + DynamicLoader::LocateBinaries(&process, pending_specs); + + for (auto [image, bin_spec] : + llvm::zip_equal(pending_images, pending_specs)) { + ModuleSP module_sp; + if (llvm::Expected<ModuleSP> loaded = + DynamicLoader::LoadBinaryInTarget(&process, bin_spec)) { + module_sp = *loaded; + } else if (bin_spec.force_symbol_search) { + *process.GetTarget().GetDebugger().GetAsyncErrorStream() + << llvm::toString(loaded.takeError()) << "\n"; + } else { + // A corefile image that isn't on this machine is routine, and has + // already been logged. + llvm::consumeError(loaded.takeError()); } - // We have a ModuleSP to load in the Target. Load it at the - // correct address/slide and notify/load scripting resources. - if (module_sp) { - added_modules.Append(module_sp, false /* notify */); - - // We have a list of segment load address - if (image.segment_load_addresses.size() > 0) { - if (log) { - std::string uuidstr = image.uuid.GetAsString(); - log->Printf("ObjectFileMachO::LoadCoreFileImages adding binary '%s' " - "UUID %s with section load addresses", - module_sp->GetFileSpec().GetPath().c_str(), - uuidstr.c_str()); - } - ObjectFile *objfile = module_sp->GetObjectFile(); - SectionList *sectlist = objfile ? objfile->GetSectionList() : nullptr; - for (auto name_vmaddr_tuple : image.segment_load_addresses) { - if (sectlist) { - SectionSP sect_sp = - sectlist->FindSectionByName(std::get<0>(name_vmaddr_tuple)); - if (sect_sp) { - process.GetTarget().SetSectionLoadAddress( - sect_sp, std::get<1>(name_vmaddr_tuple)); - } + if (!module_sp) + continue; + + added_modules.Append(module_sp, false /* notify */); + + // We have a list of segment load address + if (image->segment_load_addresses.size() > 0) { + if (log) { + std::string uuidstr = image->uuid.GetAsString(); + log->Printf("ObjectFileMachO::LoadCoreFileImages adding binary '%s' " + "UUID %s with section load addresses", + module_sp->GetFileSpec().GetPath().c_str(), + uuidstr.c_str()); + } + ObjectFile *objfile = module_sp->GetObjectFile(); + SectionList *sectlist = objfile ? objfile->GetSectionList() : nullptr; + for (auto name_vmaddr_tuple : image->segment_load_addresses) { + if (sectlist) { + SectionSP sect_sp = + sectlist->FindSectionByName(std::get<0>(name_vmaddr_tuple)); + if (sect_sp) { + process.GetTarget().SetSectionLoadAddress( + sect_sp, std::get<1>(name_vmaddr_tuple)); } } - } else { - if (log) { - std::string uuidstr = image.uuid.GetAsString(); - log->Printf("ObjectFileMachO::LoadCoreFileImages adding binary '%s' " - "UUID %s with %s 0x%" PRIx64, - module_sp->GetFileSpec().GetPath().c_str(), - uuidstr.c_str(), - value_is_offset ? "slide" : "load address", value); - } - bool changed; - module_sp->SetLoadAddress(process.GetTarget(), value, value_is_offset, - changed); } + } else { + if (log) { + std::string uuidstr = image->uuid.GetAsString(); + log->Printf("ObjectFileMachO::LoadCoreFileImages adding binary '%s' " + "UUID %s with %s 0x%" PRIx64, + module_sp->GetFileSpec().GetPath().c_str(), uuidstr.c_str(), + bin_spec.value_is_offset ? "slide" : "load address", + bin_spec.value); + } + bool changed; + module_sp->SetLoadAddress(process.GetTarget(), bin_spec.value, + bin_spec.value_is_offset, changed); } } + if (added_modules.GetSize() > 0) { process.GetTarget().ModulesDidLoad(added_modules); process.Flush(); diff --git a/lldb/source/Symbol/SymbolLocator.cpp b/lldb/source/Symbol/SymbolLocator.cpp index 4b8bc7405fdbb..58304aad24100 100644 --- a/lldb/source/Symbol/SymbolLocator.cpp +++ b/lldb/source/Symbol/SymbolLocator.cpp @@ -10,10 +10,12 @@ #include "lldb/Core/Debugger.h" #include "lldb/Core/PluginManager.h" +#include "lldb/Core/Progress.h" #include "lldb/Host/FileSystem.h" #include "lldb/Host/Host.h" #include "lldb/Target/Platform.h" +#include "llvm/ADT/STLExtras.h" #include "llvm/ADT/SmallSet.h" #include "llvm/Support/ThreadPool.h" @@ -31,22 +33,13 @@ std::error_code SymbolLocator::NotFound::convertToErrorCode() const { } llvm::Expected<SymbolLocator::Result> -SymbolLocator::Locate(const Request &request, - const FileSpecList &search_paths) { +SymbolLocator::LocateWithPlugins(const Request &request, + const FileSpecList &search_paths) { FileSystem &fs = FileSystem::Instance(); Result result; ModuleSpec &module_spec = result.module_spec; module_spec = request.module_spec; - // The locator plugins have no Platform to consult, so ask it here. - if (request.platform) { - if (std::optional<ModuleSpec> found = request.platform->FindModuleFiles( - module_spec, search_paths, result.statistics)) { - result.module_spec = *found; - return result; - } - } - // Can lldb's symbol and executable location schemes find them locally? module_spec.GetSymbolFileSpec() = PluginManager::LocateExecutableSymbolFile( module_spec, search_paths, result.statistics); @@ -77,6 +70,78 @@ SymbolLocator::Locate(const Request &request, return result; } +static std::optional<SymbolLocator::Result> +AskPlatform(const SymbolLocator::Request &request, + const FileSpecList &search_paths) { + if (!request.platform) + return std::nullopt; + + SymbolLocator::Result result; + std::optional<ModuleSpec> found = request.platform->FindModuleFiles( + request.module_spec, search_paths, result.statistics); + if (!found) + return std::nullopt; + + result.module_spec = *found; + return result; +} + +llvm::Expected<SymbolLocator::Result> +SymbolLocator::Locate(const Request &request, + const FileSpecList &search_paths) { + if (std::optional<Result> answer = AskPlatform(request, search_paths)) + return std::move(*answer); + return LocateWithPlugins(request, search_paths); +} + +std::vector<llvm::Expected<SymbolLocator::Result>> +SymbolLocator::Locate(llvm::ArrayRef<Request> requests, + const FileSpecList &search_paths, bool parallel) { + // One slot per request, so concurrent searches never contend. + std::vector<std::optional<llvm::Expected<Result>>> slots(requests.size()); + std::vector<size_t> remaining; + + if (!requests.empty()) { + // Throttled because every search reports through this from its own thread. + Progress progress("Locating binaries", "", requests.size(), + /*debugger=*/nullptr, + Progress::kDefaultHighFrequencyReportTime); + + for (auto [i, request] : llvm::enumerate(requests)) { + if (std::optional<Result> answer = AskPlatform(request, search_paths)) { + slots[i] = std::move(*answer); + progress.Increment(1, request.description); + } else { + remaining.push_back(i); + } + } + + auto locate = [&](size_t i) { + slots[i] = LocateWithPlugins(requests[i], search_paths); + progress.Increment(1, requests[i].description); + }; + + // One search has nothing to overlap with. + if (parallel && remaining.size() > 1) { + llvm::ThreadPoolTaskGroup task_group(Debugger::GetThreadPool()); + for (size_t i : remaining) + task_group.async(locate, i); + task_group.wait(); + } else { + for (size_t i : remaining) + locate(i); + } + } + + std::vector<llvm::Expected<Result>> results; + results.reserve(slots.size()); + for (std::optional<llvm::Expected<Result>> &slot : slots) { + assert(slot && "every request has a result"); + results.emplace_back(std::move(*slot)); + } + return results; +} + void SymbolLocator::DownloadSymbolFileAsync(const UUID &uuid) { static llvm::SmallSet<UUID, 8> g_seen_uuids; static std::mutex g_mutex; diff --git a/lldb/test/API/macosx/lc-note/multiple-binary-corefile/TestMultipleBinaryCorefile.py b/lldb/test/API/macosx/lc-note/multiple-binary-corefile/TestMultipleBinaryCorefile.py index 8a35536b27301..96553a866bb43 100644 --- a/lldb/test/API/macosx/lc-note/multiple-binary-corefile/TestMultipleBinaryCorefile.py +++ b/lldb/test/API/macosx/lc-note/multiple-binary-corefile/TestMultipleBinaryCorefile.py @@ -196,6 +196,31 @@ def test_corefile_binaries_dsymforuuid(self): self.load_corefile_and_test() + @skipIf(archs=no_match(["x86_64", "arm64", "arm64e", "aarch64"])) + @skipIfRemote + @requireDarwin + def test_corefile_binaries_serial_search(self): + """The corefile's binaries are searched for in parallel by default. + Searching for them one at a time has to give the same answer, in the + same order, since load_corefile_and_test indexes into the module + list.""" + self.initial_setup() + + self.runCmd("settings set target.parallel-module-load false") + self.addTearDownHook( + lambda: self.runCmd("settings clear target.parallel-module-load") + ) + + # Register the binaries in lldb's global module cache, as + # test_corefile_binaries_preloaded does, so the corefile's images can + # be found without a symbol server. + target = self.dbg.CreateTarget(self.aout_exe, "", "", False, lldb.SBError()) + self.dbg.DeleteTarget(target) + target = self.dbg.CreateTarget(self.libtwo_exe, "", "", False, lldb.SBError()) + self.dbg.DeleteTarget(target) + + self.load_corefile_and_test() + @skipIf(archs=no_match(["x86_64", "arm64", "arm64e", "aarch64"])) @skipIfRemote @requireDarwin diff --git a/lldb/unittests/Symbol/SymbolLocatorTest.cpp b/lldb/unittests/Symbol/SymbolLocatorTest.cpp index d86808702da5e..cc7e8854cf1b7 100644 --- a/lldb/unittests/Symbol/SymbolLocatorTest.cpp +++ b/lldb/unittests/Symbol/SymbolLocatorTest.cpp @@ -7,27 +7,42 @@ //===----------------------------------------------------------------------===// #include "lldb/Symbol/SymbolLocator.h" +#include "TestingSupport/TestUtilities.h" +#include "lldb/Core/Debugger.h" #include "lldb/Core/PluginManager.h" #include "lldb/Host/FileSystem.h" #include "lldb/Host/HostInfo.h" #include "lldb/Target/Platform.h" #include "lldb/Utility/FileSpecList.h" +#include "llvm/ADT/STLExtras.h" +#include "llvm/Support/ThreadPool.h" #include "llvm/Support/VirtualFileSystem.h" #include "llvm/Testing/Support/Error.h" #include "gtest/gtest.h" +#include <atomic> +#include <condition_variable> +#include <mutex> + using namespace lldb; using namespace lldb_private; namespace { /// Which steps of the search ran, so a test can tell where an answer came from. +/// Written from every thread of a batch, so the flags have to be atomic. struct LocatorCalls { - bool located_symbol_file = false; - bool located_object_file = false; - bool downloaded = false; + std::atomic<bool> located_symbol_file = false; + std::atomic<bool> located_object_file = false; + std::atomic<bool> downloaded = false; + + void Clear() { + located_symbol_file = false; + located_object_file = false; + downloaded = false; + } }; LocatorCalls g_calls; @@ -42,6 +57,13 @@ std::optional<FileSpec> g_symbol_file; /// an errno rather than a message. bool g_symbol_server_errno = false; +/// When set, the fake locator only answers for requests carrying a UUID. +bool g_only_with_uuid = false; + +/// Run by the fake locator, to let a test hold every search of a batch open at +/// once. +std::function<void()> g_barrier; + std::optional<FileSpec> LocateExecutableSymbolFile(const ModuleSpec &, const FileSpecList &) { g_calls.located_symbol_file = true; @@ -50,8 +72,12 @@ std::optional<FileSpec> LocateExecutableSymbolFile(const ModuleSpec &, std::optional<ModuleSpec> LocateExecutableObjectFile(const ModuleSpec &spec) { g_calls.located_object_file = true; + if (g_barrier) + g_barrier(); if (!g_object_file) return {}; + if (g_only_with_uuid && !spec.GetUUID().IsValid()) + return {}; ModuleSpec located(spec); located.GetFileSpec() = *g_object_file; return located; @@ -113,6 +139,11 @@ class SymbolLocatorTest : public testing::Test { m_fs(new llvm::vfs::InMemoryFileSystem()) {} void SetUp() override { + // The batch runs on the debugger's thread pool. Debugger::Initialize takes + // an argument, so SubsystemRAII cannot call it. + std::call_once(TestUtilities::g_debugger_initialize_flag, + []() { Debugger::Initialize(nullptr); }); + // Locate reports a binary it cannot find as an error, so a test that wants // a hit has to point the fake locator at a file that exists. FileSystem::Initialize(m_fs); @@ -120,10 +151,12 @@ class SymbolLocatorTest : public testing::Test { m_fs->addFileNoOwn(m_binary.GetPath(), 0, m_empty_buffer); m_fs->addFileNoOwn(m_symbols.GetPath(), 0, m_empty_buffer); - g_calls = LocatorCalls(); + g_calls.Clear(); g_object_file = std::nullopt; g_symbol_file = std::nullopt; g_symbol_server_errno = false; + g_only_with_uuid = false; + g_barrier = nullptr; ASSERT_TRUE(PluginManager::RegisterPlugin( "test", "test symbol locator", CreateSymbolLocator, LocateExecutableObjectFile, LocateExecutableSymbolFile, @@ -131,6 +164,7 @@ class SymbolLocatorTest : public testing::Test { } void TearDown() override { + g_barrier = nullptr; PluginManager::UnregisterPlugin(CreateSymbolLocator); HostInfo::Terminate(); FileSystem::Terminate(); @@ -144,6 +178,10 @@ class SymbolLocatorTest : public testing::Test { FileSpec m_symbols = FileSpec("/binary.dSYM", FileSpec::Style::posix); }; +std::vector<SymbolLocator::Request> MakeRequests(size_t count) { + return std::vector<SymbolLocator::Request>(count); +} + } // namespace TEST_F(SymbolLocatorTest, MissRunsEveryStep) { @@ -269,3 +307,102 @@ TEST_F(SymbolLocatorTest, ThePluginsRunWhenThePlatformHasNothingToSay) { EXPECT_EQ(1u, platform->find_module_files_calls); EXPECT_TRUE(g_calls.located_object_file); } + +TEST_F(SymbolLocatorTest, TheBatchSearchesEveryRequest) { + g_object_file = m_binary; + std::vector<SymbolLocator::Request> requests = MakeRequests(8); + + std::vector<llvm::Expected<SymbolLocator::Result>> results = + SymbolLocator::Locate(requests, FileSpecList(), /*parallel=*/true); + + ASSERT_EQ(requests.size(), results.size()); + for (llvm::Expected<SymbolLocator::Result> &result : results) { + ASSERT_THAT_EXPECTED(result, llvm::Succeeded()); + EXPECT_EQ(m_binary, result->module_spec.GetFileSpec()); + } +} + +TEST_F(SymbolLocatorTest, TheBatchKeepsResultsInRequestOrder) { + // Every other request is one the locator will answer, so the results can only + // line up with the requests if the order is kept. + g_object_file = m_binary; + g_only_with_uuid = true; + std::vector<SymbolLocator::Request> requests = MakeRequests(6); + for (auto [i, request] : llvm::enumerate(requests)) + if (i % 2 == 0) + request.module_spec.GetUUID() = UUID("0123456789ABCDEF", 16); + + std::vector<llvm::Expected<SymbolLocator::Result>> results = + SymbolLocator::Locate(requests, FileSpecList(), /*parallel=*/true); + + ASSERT_EQ(requests.size(), results.size()); + for (auto [i, result] : llvm::enumerate(results)) { + if (i % 2 == 0) + EXPECT_THAT_EXPECTED(result, llvm::Succeeded()) << "request " << i; + else + EXPECT_THAT_EXPECTED(result, llvm::Failed()) << "request " << i; + } +} + +TEST_F(SymbolLocatorTest, TheBatchSearchesConcurrently) { + // A serial batch could never get every task inside the locator at once. No + // more tasks than the pool can run, or the ones left queued would hang it. + const size_t num_requests = + std::min<size_t>(4, Debugger::GetThreadPool().getMaxConcurrency()); + if (num_requests < 2) + GTEST_SKIP() << "the thread pool runs one task at a time"; + + std::mutex mutex; + std::condition_variable cv; + size_t arrived = 0; + bool everyone_arrived = false; + + g_barrier = [&] { + std::unique_lock<std::mutex> lock(mutex); + if (++arrived == num_requests) { + everyone_arrived = true; + cv.notify_all(); + return; + } + // Assert on what the waiter observed, not on the count: a late arrival + // would set the flag either way. + bool released = cv.wait_for(lock, std::chrono::seconds(10), + [&] { return everyone_arrived; }); + EXPECT_TRUE(released) << "the batch did not run concurrently"; + }; + + std::vector<SymbolLocator::Request> requests = MakeRequests(num_requests); + std::vector<llvm::Expected<SymbolLocator::Result>> results = + SymbolLocator::Locate(requests, FileSpecList(), /*parallel=*/true); + for (llvm::Expected<SymbolLocator::Result> &result : results) + if (!result) + llvm::consumeError(result.takeError()); + + EXPECT_EQ(num_requests, arrived); +} + +TEST_F(SymbolLocatorTest, TheSerialBatchGivesTheSameAnswers) { + g_object_file = m_binary; + std::vector<SymbolLocator::Request> requests = MakeRequests(4); + + std::vector<llvm::Expected<SymbolLocator::Result>> parallel = + SymbolLocator::Locate(requests, FileSpecList(), /*parallel=*/true); + std::vector<llvm::Expected<SymbolLocator::Result>> serial = + SymbolLocator::Locate(requests, FileSpecList(), /*parallel=*/false); + + ASSERT_EQ(parallel.size(), serial.size()); + for (auto [p, s] : llvm::zip_equal(parallel, serial)) { + EXPECT_THAT_EXPECTED(p, llvm::Succeeded()); + EXPECT_THAT_EXPECTED(s, llvm::Succeeded()); + if (p && s) + EXPECT_EQ(p->module_spec.GetFileSpec(), s->module_spec.GetFileSpec()); + } +} + +TEST_F(SymbolLocatorTest, AnEmptyBatchIsNoWork) { + std::vector<llvm::Expected<SymbolLocator::Result>> results = + SymbolLocator::Locate({}, FileSpecList(), /*parallel=*/true); + + EXPECT_TRUE(results.empty()); + EXPECT_FALSE(g_calls.located_object_file); +} >From d2f67bc3c3b840166b2b8bd015f27576697eab5c Mon Sep 17 00:00:00 2001 From: Jonas Devlieghere <[email protected]> Date: Thu, 13 Aug 2026 16:45:09 -0700 Subject: [PATCH 3/3] Address Jason's feedback --- lldb/source/Core/DynamicLoader.cpp | 10 ++++++---- .../Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp | 5 +++-- 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/lldb/source/Core/DynamicLoader.cpp b/lldb/source/Core/DynamicLoader.cpp index 4178177a995a7..9420c5fd27def 100644 --- a/lldb/source/Core/DynamicLoader.cpp +++ b/lldb/source/Core/DynamicLoader.cpp @@ -280,8 +280,9 @@ PrepareSearch(Target &target, DynamicLoader::BinarySpec &bin_spec) { static void FinishSearch(DynamicLoader::BinarySpec &bin_spec, llvm::Expected<SymbolLocator::Result> located) { if (!located) { - // Only an explanation from a symbol server adds anything to the miss the - // caller already reports. + // Loading a binary that was never found already reports that, so a bare + // not-found error would only say it a second time. Any other error says + // something that report cannot. llvm::Error error = located.takeError(); if (error.isA<SymbolLocator::NotFound>()) llvm::consumeError(std::move(error)); @@ -322,8 +323,9 @@ void DynamicLoader::LocateBinaries( Target &target = process->GetTarget(); const FileSpecList search_paths = Target::GetDefaultDebugFileSearchPaths(); - // Has to happen on this thread, and before any search, so that a binary whose - // UUID is not known yet still joins the batch. + // Reading a binary's UUID out of memory has to happen on this thread, and + // before any search, so that a binary whose UUID is not known yet still joins + // the batch. llvm::SmallVector<BinarySpec *> to_search; std::vector<SymbolLocator::Request> requests; for (BinarySpec &bin_spec : bin_specs) { diff --git a/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp b/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp index 01f56f0c3ee17..9d2fe2d321407 100644 --- a/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp +++ b/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp @@ -6687,8 +6687,9 @@ bool ObjectFileMachO::LoadCoreFileImages(lldb_private::Process &process) { // processing. We're not going to call ModulesDidLoad on this in this // method, so notify==true. // - // Setting one up can replace the Target's platform and dynamic loader, so - // no image is searched for until this loop has run to the end. + // Setting up a platform binary can replace the Target's platform and + // dynamic loader, so no image is searched for until this loop has run to + // the end. if (process.GetTarget() .GetDebugger() .GetPlatformList() _______________________________________________ lldb-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
