This is an archive of the discontinued LLVM Phabricator instance.

[lld-macho] Show source information for undefined references
ClosedPublic

Authored by BertalanD on Jun 20 2022, 4:38 AM.

Details

Summary

The error used to look like this:

ld64.lld: error: undefined symbol: _foo
>>> referenced by /path/to/bar.o:(symbol _baz+0x4)

If DWARF line information is available, we now show where in the source
the references are coming from:

ld64.lld: error: unreferenced symbol: _foo
>>> referenced by: bar.cpp:42 (/path/to/bar.cpp:42)
>>>                /path/to/bar.o:(symbol _baz+0x4)

Diff Detail

Event Timeline

BertalanD created this revision.Jun 20 2022, 4:38 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJun 20 2022, 4:38 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
BertalanD requested review of this revision.Jun 20 2022, 4:38 AM
Herald added a project: Restricted Project. · View Herald Transcript

This looks excellent. Only inconsequential comments :)

lld/MachO/InputSection.cpp
111

nit: the verb has a space, "look up". ("lookup" is a noun. same for "set up" / "setup".)

116

nit: same

124

Do we have test coverage of all 3 scenarios (fun, var, file) in this function?

lld/MachO/SyntheticSections.h
438

const ref?

BertalanD updated this revision to Diff 438415.Jun 20 2022, 8:35 AM
BertalanD removed a reviewer: jdoerfert.
  • Added a test for references originating from variables (couldn't produce a non-convoluted example that covers the third case
  • Take emitBeginSourceStab's argument as a StringRef
  • Fixed the spelling errors
BertalanD marked an inline comment as done.Jun 20 2022, 8:35 AM
This revision is now accepted and ready to land.Jun 20 2022, 3:56 PM
This revision was landed with ongoing or failed builds.Jun 20 2022, 3:57 PM
This revision was automatically updated to reflect the committed changes.

Apologies, only after landing this did I realize that this makes ld64.lld crash on the repro in comment 0 of https://github.com/llvm/llvm-project/issues/56121

I'll revert this again for now, to keep HEAD green.

Lengthy stack below:

(lldb) r
Process 58952 launched: '/Users/thakis/src/llvm-project/out/gn/bin/ld64.lld' (arm64)
ld64.lld was compiled with optimization - stepping may behave oddly; variables may not be available.
Process 58952 stopped
* thread #16, stop reason = EXC_BAD_ACCESS (code=1, address=0xa00000129)
    frame #0: 0x0000000100606a80 ld64.lld`llvm::DWARFUnit::tryExtractDIEsIfNeeded(bool) [inlined] std::__1::vector<llvm::DWARFDebugInfoEntry, std::__1::allocator<llvm::DWARFDebugInfoEntry> >::empty(this=0x0000000a00000129 size=0) const at vector:524:23 [opt]
   521 	        {return static_cast<size_type>(__end_cap() - this->__begin_);}
   522 	    _LIBCPP_NODISCARD_AFTER_CXX17 _LIBCPP_INLINE_VISIBILITY
   523 	    bool empty() const _NOEXCEPT
-> 524 	        {return this->__begin_ == this->__end_;}
   525 	    size_type max_size() const _NOEXCEPT;
   526 	    void reserve(size_type __n);
   527 	    void shrink_to_fit() _NOEXCEPT;
Target 0: (ld64.lld) stopped.
(lldb) bt
* thread #16, stop reason = EXC_BAD_ACCESS (code=1, address=0xa00000129)
  * frame #0: 0x0000000100606a80 ld64.lld`llvm::DWARFUnit::tryExtractDIEsIfNeeded(bool) [inlined] std::__1::vector<llvm::DWARFDebugInfoEntry, std::__1::allocator<llvm::DWARFDebugInfoEntry> >::empty(this=0x0000000a00000129 size=0) const at vector:524:23 [opt]
    frame #1: 0x0000000100606a80 ld64.lld`llvm::DWARFUnit::tryExtractDIEsIfNeeded(this=0x0000000a00000009, CUDieOnly=true) at DWARFUnit.cpp:487:31 [opt]
    frame #2: 0x000000010060699c ld64.lld`llvm::DWARFUnit::extractDIEsIfNeeded(this=0x0000000a00000009, CUDieOnly=<unavailable>) at DWARFUnit.cpp:482:17 [opt]
    frame #3: 0x000000010060614c ld64.lld`llvm::DWARFUnit::getCompilationDir() [inlined] llvm::DWARFUnit::getUnitDIE(this=0x0000000a00000009, ExtractUnitDIEOnly=true) at DWARFUnit.h:403:5 [opt]
    frame #4: 0x0000000100606144 ld64.lld`llvm::DWARFUnit::getCompilationDir(this=0x0000000a00000009) at DWARFUnit.cpp:390:26 [opt]
    frame #5: 0x000000010025305c ld64.lld`lld::macho::ObjFile::sourceFile(this=0x00000001587fc698) const at InputFiles.cpp:1379:37 [opt]
    frame #6: 0x000000010027e280 ld64.lld`lld::macho::SymtabSection::emitStabs(this=0x000000028cdc4a00) at SyntheticSections.cpp:934:33 [opt]
    frame #7: 0x000000010027e850 ld64.lld`lld::macho::SymtabSection::finalizeContents(this=0x000000028cdc4a00) at SyntheticSections.cpp:1027:3 [opt]
    frame #8: 0x00000001002a4d74 ld64.lld`std::__1::__function::__func<llvm::ThreadPool::createTaskAndFuture(std::__1::function<void ()>)::'lambda'(), std::__1::allocator<llvm::ThreadPool::createTaskAndFuture(std::__1::function<void ()>)::'lambda'()>, void ()>::operator()() [inlined] std::__1::__function::__value_func<void ()>::operator(this=0x0000600001809218)() const at function.h:509:16 [opt]
    frame #9: 0x00000001002a4d60 ld64.lld`std::__1::__function::__func<llvm::ThreadPool::createTaskAndFuture(std::__1::function<void ()>)::'lambda'(), std::__1::allocator<llvm::ThreadPool::createTaskAndFuture(std::__1::function<void ()>)::'lambda'()>, void ()>::operator()() [inlined] std::__1::function<void ()>::operator(this=0x0000600001809218)() const at function.h:1186:12 [opt]
    frame #10: 0x00000001002a4d60 ld64.lld`std::__1::__function::__func<llvm::ThreadPool::createTaskAndFuture(std::__1::function<void ()>)::'lambda'(), std::__1::allocator<llvm::ThreadPool::createTaskAndFuture(std::__1::function<void ()>)::'lambda'()>, void ()>::operator()() [inlined] llvm::ThreadPool::createTaskAndFuture(this=0x0000600001809208)>)::'lambda'()::operator()() const at ThreadPool.h:135:15 [opt]
    frame #11: 0x00000001002a4d60 ld64.lld`std::__1::__function::__func<llvm::ThreadPool::createTaskAndFuture(std::__1::function<void ()>)::'lambda'(), std::__1::allocator<llvm::ThreadPool::createTaskAndFuture(std::__1::function<void ()>)::'lambda'()>, void ()>::operator()() [inlined] decltype(__f=0x0000600001809208)>)::'lambda'()&>(fp)()) std::__1::__invoke<llvm::ThreadPool::createTaskAndFuture(std::__1::function<void ()>)::'lambda'()&>(llvm::ThreadPool::createTaskAndFuture(std::__1::function<void ()>)::'lambda'()&) at type_traits:2435:23 [opt]
    frame #12: 0x00000001002a4d60 ld64.lld`std::__1::__function::__func<llvm::ThreadPool::createTaskAndFuture(std::__1::function<void ()>)::'lambda'(), std::__1::allocator<llvm::ThreadPool::createTaskAndFuture(std::__1::function<void ()>)::'lambda'()>, void ()>::operator()() [inlined] void std::__1::__invoke_void_return_wrapper<void, true>::__call<llvm::ThreadPool::createTaskAndFuture(__args=0x0000600001809208)>)::'lambda'()&>(llvm::ThreadPool::createTaskAndFuture(std::__1::function<void ()>)::'lambda'()&) at invoke.h:61:9 [opt]
    frame #13: 0x00000001002a4d60 ld64.lld`std::__1::__function::__func<llvm::ThreadPool::createTaskAndFuture(std::__1::function<void ()>)::'lambda'(), std::__1::allocator<llvm::ThreadPool::createTaskAndFuture(std::__1::function<void ()>)::'lambda'()>, void ()>::operator()() [inlined] std::__1::__function::__alloc_func<llvm::ThreadPool::createTaskAndFuture(std::__1::function<void ()>)::'lambda'(), std::__1::allocator<llvm::ThreadPool::createTaskAndFuture(std::__1::function<void ()>)::'lambda'()>, void ()>::operator(this=0x0000600001809208)() at function.h:182:16 [opt]
    frame #14: 0x00000001002a4d60 ld64.lld`std::__1::__function::__func<llvm::ThreadPool::createTaskAndFuture(std::__1::function<void ()>)::'lambda'(), std::__1::allocator<llvm::ThreadPool::createTaskAndFuture(std::__1::function<void ()>)::'lambda'()>, void ()>::operator(this=0x0000600001809200)() at function.h:356:12 [opt]
    frame #15: 0x000000010035dd54 ld64.lld`llvm::ThreadPool::processTasks(llvm::ThreadPoolTaskGroup*) [inlined] std::__1::__function::__value_func<void ()>::operator(this=0x00000002da436f18)() const at function.h:509:16 [opt]
    frame #16: 0x000000010035dd40 ld64.lld`llvm::ThreadPool::processTasks(llvm::ThreadPoolTaskGroup*) [inlined] std::__1::function<void ()>::operator(this=0x00000002da436f18)() const at function.h:1186:12 [opt]
    frame #17: 0x000000010035dd40 ld64.lld`llvm::ThreadPool::processTasks(this=0x000000016fdfe1a0, WaitingForGroup=0x0000000000000000) at ThreadPool.cpp:99:5 [opt]
    frame #18: 0x000000010035e694 ld64.lld`void* llvm::thread::ThreadProxy<std::__1::tuple<llvm::ThreadPool::grow(int)::$_0> >(void*) [inlined] llvm::ThreadPool::grow(int)::$_0::operator()() const at ThreadPool.cpp:47:7 [opt]
    frame #19: 0x000000010035e680 ld64.lld`void* llvm::thread::ThreadProxy<std::__1::tuple<llvm::ThreadPool::grow(int)::$_0> >(void*) [inlined] void llvm::thread::Apply<llvm::ThreadPool::grow(int)::$_0>(std::__1::tuple<llvm::ThreadPool::grow(int)::$_0>&, std::__1::integer_sequence<unsigned long>) at thread.h:42:5 [opt]
    frame #20: 0x000000010035e680 ld64.lld`void* llvm::thread::ThreadProxy<std::__1::tuple<llvm::ThreadPool::grow(int)::$_0> >(void*) [inlined] void llvm::thread::GenericThreadProxy<std::__1::tuple<llvm::ThreadPool::grow(int)::$_0> >(Ptr=0x0000600000121490) at thread.h:50:5 [opt]
    frame #21: 0x000000010035e678 ld64.lld`void* llvm::thread::ThreadProxy<std::__1::tuple<llvm::ThreadPool::grow(int)::$_0> >(Ptr=0x0000600000121490) at thread.h:60:5 [opt]
    frame #22: 0x000000018c02626c libsystem_pthread.dylib`_pthread_start + 148

Looks like ObjFile::parseDebugInfo wasn't checking if there were any compilation units before assigning the iterator's value to compileUnit. I'm not sure why that didn't cause issues previously, it's maybe because we were taking slightly different code paths as we did not parse __debug_line. The following change makes lld not crash when linking the Chromium reproducer.

diff --git a/lld/MachO/InputFiles.cpp b/lld/MachO/InputFiles.cpp
index 2db8dae75d42..724e8f64d2ab 100644
--- a/lld/MachO/InputFiles.cpp
+++ b/lld/MachO/InputFiles.cpp
@@ -1015,7 +1015,10 @@ void ObjFile::parseDebugInfo() {
   // FIXME: There can be more than one compile unit per object file. See
   // PR48637.
   auto it = units.begin();
-  compileUnit = it->get();
+  if (it == units.end())
+    compileUnit = nullptr;
+  else
+    compileUnit = it->get();
 }
 
 ArrayRef<data_in_code_entry> ObjFile::getDataInCode() const {
@@ -1376,6 +1379,8 @@ void ObjFile::registerEhFrames(Section &ehFrameSection) {
 }
 
 std::string ObjFile::sourceFile() const {
+  if (!compileUnit)
+    return {};
   SmallString<261> dir(compileUnit->getCompilationDir());
   StringRef sep = sys::path::get_separator();
   // We don't use `path::append` here because we want an empty `dir` to result

I'll set up a build on Linux to check if the failures there go away as well.

Fix crash if no DWARF compile unit info is present.

It looks like lots of tests failed on the expensive-check bots (eg here https://lab.llvm.org/buildbot/#/builders/42/builds/5914) so we do have test coverage. Do you think it's easy to make a test that fails without expensive checks? (If not, it's fine as-is, given that some of the bots did catch this.)

Looks like ObjFile::parseDebugInfo wasn't checking if there were any compilation units before assigning the iterator's value to compileUnit. I'm not sure why that didn't cause issues previously, it's maybe because we were taking slightly different code paths as we did not parse __debug_line.

I think your __debug_line theory has legs. If I take the original patch and change just this:

 % git diff
diff --git a/lld/MachO/Dwarf.cpp b/lld/MachO/Dwarf.cpp
index 357503a655cd..24a6530795dd 100644
--- a/lld/MachO/Dwarf.cpp
+++ b/lld/MachO/Dwarf.cpp
@@ -29,7 +29,7 @@ std::unique_ptr<DwarfObject> DwarfObject::create(ObjFile *obj) {
     if (StringRef *s =
             StringSwitch<StringRef *>(isec->getName())
                 .Case(section_names::debugInfo, &dObj->infoSection.Data)
-                .Case(section_names::debugLine, &dObj->lineSection.Data)
+                //.Case(section_names::debugLine, &dObj->lineSection.Data)
                 .Case(section_names::debugAbbrev, &dObj->abbrevSection)
                 .Case(section_names::debugStr, &dObj->strSection)
                 .Default(nullptr)) {

…then my repro case no longer crashes.

So dereferencing end() happens to produce nullptr before by dumb luck, but doesn't after. And scarily, we don't have anything that catches us derefing end() in this scenario.

I checked the object in Chromium that caused the crash, and it indeed only has __debug_line but not the other DWARF sections. I'll have a think about crafting an object that has only one of the three sections that are currently parsed on trunk, and see if we can produce the same crash with it. I suppose if that works, I'll submit a separate diff with a fix for trunk and the test case.

BertalanD updated this revision to Diff 438807.Jun 21 2022, 1:06 PM

Rebased onto D128294, which should fix the segmentation fault.

thakis added inline comments.Jun 21 2022, 1:45 PM
lld/MachO/InputFiles.cpp
1379

Do we need this check? All current callers of sourceFile() check compileUnit before calling this.

BertalanD added inline comments.Jun 21 2022, 2:09 PM
lld/MachO/InputFiles.cpp
1379

This sounds like one of those narrow vs wide contract API questions. Should I remove the check from here, replace it with an assert, or remove the check from getSourceLocation? I don't really have a preference.

thakis added inline comments.Jun 21 2022, 2:42 PM
lld/MachO/InputFiles.cpp
1379

I'd just remove the check. Adding an assert is fine, but it's kind of implied be the (then) unguarded deref

BertalanD updated this revision to Diff 438841.Jun 21 2022, 2:46 PM

remove unnecessary null check

BertalanD marked 2 inline comments as done.Jun 21 2022, 2:46 PM