[lld-macho] Print the name of functions containing undefined references

[BertalanD]

Summary

The error used to look like this:

ld64.lld: error: undefined symbol: _foo
>>> referenced by /path/to/bar.o

Now it displays the name of the function that contains the undefined
reference as well:

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

I plan to use debug information to provide even more context in a future
commit.

Please commit this diff with the following author info:
Daniel Bertalan <[e-mail]>

[thakis]

This looks excellent.

The ELF and COFF ports as well as ld64 don't print the offset:

% out/gn/bin/clang -fuse-ld=lld test.cc -isysroot $(xcrun -show-sdk-path)  --target=x86_64-pc-linux -nostdlib
ld.lld: error: undefined symbol: f()
>>> referenced by test.cc
>>>               /var/folders/w6/wpbtszrs7jl9dc9l5qtdkvg00000gn/T/test-d7614d.o:(main)
clang: error: linker command failed with exit code 1 (use -v to see invocation)

% out/gn/bin/clang-cl -fuse-ld=lld test.cc  /link /nodefaultlib                                     
lld-link: error: <root>: undefined symbol: mainCRTStartup
lld-link: error: undefined symbol: void __cdecl f(void)
>>> referenced by /var/folders/w6/wpbtszrs7jl9dc9l5qtdkvg00000gn/T/test-611b03.obj:(main)
clang: error: linker command failed with exit code 1 (use -v to see invocation)

% out/gn/bin/clang test.o -isysroot $(xcrun -show-sdk-path)              
Undefined symbols for architecture arm64:
  "f()", referenced from:
      _main in test.o
ld: symbol(s) not found for architecture arm64

Should we omit it too, at least for undefined symbol diags?

[int3]

Should we omit it too, at least for undefined symbol diags?

IMO we are strictly providing more useful info here so I think it makes more sense for us to keep it this way and maybe have the other ports changed to match at some point

[thakis]

In D127696#3581100, @int3 wrote:

Should we omit it too, at least for undefined symbol diags?

IMO we are strictly providing more useful info here so I think it makes more sense for us to keep it this way and maybe have the other ports changed to match at some point

What's useful about an offset about a function?

I think for almost everyone, the offset is at best noise they'll ignore or at worst something that confuses them.

And even for people who know what it means and can read assembly, what's the added value of knowing which offset in a function has the reference?

[thakis]

Let's land this as-is for now. I'm sure once Jez reads my reply he will be swayed by my flawless reasoning, but implementing this suggestion will require more code on top of the code that is already here, so might as well do that suggestion in a follow-up and keep this one smaller. (And everyone agrees that this patch as-is is a big step forward already.)

[int3]

And even for people who know what it means and can read assembly, what's the added value of knowing which offset in a function has the reference?

Okay fair. I guess if we printed out the actual debug line number, that might be more useful (since the user might want to delete that reference in the source.)

It appears that LLD-ELF only emits the address offset when reporting on an invalid relocation, which seems like a reasonable approach to follow, since in that case we actually do need the address to diagnose the error

[int3]

One minute apart :) but yeah we can address that in a followup

lld/MachO/SymbolTable.h

public:
   Symbol *find(StringRef name) { return find(llvm::CachedHashStringRef(name)); }
 
 private:
   std::pair<Symbol *, bool> insert(StringRef name, const InputFile *);
   llvm::DenseMap<llvm::CachedHashStringRef, int> symMap;
   std::vector<Symbol *> symVector;
 };
 
-void treatUndefinedSymbol(const Undefined &, StringRef source = "");
+void treatUndefinedSymbol(const Undefined &, StringRef source);
+void treatUndefinedSymbol(const Undefined &, const InputSection *,
+                          uint64_t offset);
 
 extern std::unique_ptr<SymbolTable> symtab;
 
 } // namespace macho
 } // namespace lld
 
 #endif

lld/MachO/SymbolTable.cpp

static void handleSegmentBoundarySymbol(const Undefined &sym, StringRef segName,
                                         Boundary which) {
   OutputSegment *seg = getOrCreateOutputSegment(segName);
   if (which == Boundary::Start)
     seg->segmentStartSymbols.push_back(createBoundarySymbol(sym));
   else
     seg->segmentEndSymbols.push_back(createBoundarySymbol(sym));
 }
 
-void lld::macho::treatUndefinedSymbol(const Undefined &sym, StringRef source) {
+// Try to find a definition for an undefined symbol.
+// Returns true if a definition was found and no diagnostics are needed.
+static bool recoverFromUndefinedSymbol(const Undefined &sym) {
   // Handle start/end symbols.
   StringRef name = sym.getName();
-  if (name.consume_front("section$start$"))
-    return handleSectionBoundarySymbol(sym, name, Boundary::Start);
-  if (name.consume_front("section$end$"))
-    return handleSectionBoundarySymbol(sym, name, Boundary::End);
-  if (name.consume_front("segment$start$"))
-    return handleSegmentBoundarySymbol(sym, name, Boundary::Start);
-  if (name.consume_front("segment$end$"))
-    return handleSegmentBoundarySymbol(sym, name, Boundary::End);
+  if (name.consume_front("section$start$")) {
+    handleSectionBoundarySymbol(sym, name, Boundary::Start);
+    return true;
+  }
+  if (name.consume_front("section$end$")) {
+    handleSectionBoundarySymbol(sym, name, Boundary::End);
+    return true;
+  }
+  if (name.consume_front("segment$start$")) {
+    handleSegmentBoundarySymbol(sym, name, Boundary::Start);
+    return true;
+  }
+  if (name.consume_front("segment$end$")) {
+    handleSegmentBoundarySymbol(sym, name, Boundary::End);
+    return true;
+  }
 
   // Handle -U.
   if (config->explicitDynamicLookups.count(sym.getName())) {
     symtab->addDynamicLookup(sym.getName());
-    return;
+    return true;
   }
 
   // Handle -undefined.
-  auto message = [source, &sym]() {
-    std::string message = "undefined symbol";
-    if (config->archMultiple)
-      message += (" for arch " + getArchitectureName(config->arch())).str();
-    message += ": " + toString(sym);
-    if (!source.empty())
-      message += "\n>>> referenced by " + source.str();
-    else
-      message += "\n>>> referenced by " + toString(sym.getFile());
-    return message;
-  };
-  switch (config->undefinedSymbolTreatment) {
-  case UndefinedSymbolTreatment::error:
-    error(message());
-    break;
-  case UndefinedSymbolTreatment::warning:
-    warn(message());
-    LLVM_FALLTHROUGH;
-  case UndefinedSymbolTreatment::dynamic_lookup:
-  case UndefinedSymbolTreatment::suppress:
-    symtab->addDynamicLookup(sym.getName());
-    break;
-  case UndefinedSymbolTreatment::unknown:
-    llvm_unreachable("unknown -undefined TREATMENT");
-  }
+  if (config->undefinedSymbolTreatment ==
+          UndefinedSymbolTreatment::dynamic_lookup ||
+      config->undefinedSymbolTreatment == UndefinedSymbolTreatment::suppress) {
+    symtab->addDynamicLookup(sym.getName());
+    return true;
+  }
+
+  // We do not return true here, as we still need to print diagnostics.
+  if (config->undefinedSymbolTreatment == UndefinedSymbolTreatment::warning)
+    symtab->addDynamicLookup(sym.getName());
+
+  return false;
+}
+
+static void printUndefinedDiagnostic(StringRef name, StringRef source) {
+  message += (": " + name + "\n>>> referenced by " + source).str();
+
+  if (config->undefinedSymbolTreatment == UndefinedSymbolTreatment::error)
+    error(message);
+  else if (config->undefinedSymbolTreatment ==
+           UndefinedSymbolTreatment::warning)
+    warn(message);
+    assert(false && "diagnostics make sense for -undefined error|warning only");
+
+void lld::macho::treatUndefinedSymbol(const Undefined &sym, StringRef source) {
+  if (recoverFromUndefinedSymbol(sym))
+    return;
+  printUndefinedDiagnostic(sym.getName(), source);
+}
+
+void lld::macho::treatUndefinedSymbol(const Undefined &sym,
+                                      const InputSection *isec,
+                                      uint64_t offset) {
+  if (recoverFromUndefinedSymbol(sym))
+    return;
+
+  // TODO: Get source file/line from debug information.
+  printUndefinedDiagnostic(toString(sym), isec->getLocation(offset));
 }
 
 std::unique_ptr<SymbolTable> macho::symtab;

lld/MachO/UnwindInfoSection.cpp

if (auto *s = r.referent.dyn_cast<Symbol *>()) {
         //
         // (See discussions/alternatives already considered on D107533)
         if (!defined->isExternal())
           if (Symbol *sym = symtab->find(defined->getName()))
             if (!sym->isLazy())
               r.referent = s = sym;
       }
       if (auto *undefined = dyn_cast<Undefined>(s)) {
-        treatUndefinedSymbol(*undefined);
+        treatUndefinedSymbol(*undefined, isec, r.offset);
         // treatUndefinedSymbol() can replace s with a DylibSymbol; re-check.
         if (isa<Undefined>(s))
           continue;
       }
 
       if (auto *defined = dyn_cast<Defined>(s)) {
         // Check if we have created a synthetic symbol at the same address.
         Symbol *&personality =

lld/MachO/Writer.cpp

for (auto it = isec->relocs.begin(); it != isec->relocs.end(); ++it) {
         // Skip over the following UNSIGNED relocation -- it's just there as the
         // minuend, and doesn't have the usual UNSIGNED semantics. We don't want
         // to emit rebase opcodes for it.
         it++;
         continue;
       }
       if (auto *sym = r.referent.dyn_cast<Symbol *>()) {
         if (auto *undefined = dyn_cast<Undefined>(sym))
-          treatUndefinedSymbol(*undefined);
+          treatUndefinedSymbol(*undefined, isec, r.offset);
         // treatUndefinedSymbol() can replace sym with a DylibSymbol; re-check.
         if (!isa<Undefined>(sym) && validateSymbolRelocation(sym, isec, r))
           prepareSymbolRelocation(sym, isec, r);
       } else {
         // Canonicalize the referent so that later accesses in Writer won't
         // have to worry about it. Perhaps we should do this for Defined::isec
         // too...
         auto *referentIsec = r.referent.get<InputSection *>();

lld/test/MachO/invalid/undefined-symbol.s

 # REQUIRES: x86
 # RUN: rm -rf %t; split-file %s %t
 # RUN: llvm-mc -filetype=obj -triple=x86_64-apple-darwin %t/main.s -o %t/main.o
 # RUN: llvm-mc -filetype=obj -triple=x86_64-apple-darwin %t/foo.s -o %t/foo.o
 # RUN: llvm-ar crs %t/foo.a %t/foo.o
 # RUN: not %lld --icf=all -o /dev/null %t/main.o 2>&1 | \
-# RUN:     FileCheck %s -DSYM=_foo -DFILENAME=%t/main.o
+# RUN:     FileCheck %s -DSYM=_foo -DLOC='%t/main.o:(symbol _main+0x1)'
 # RUN: not %lld -o /dev/null %t/main.o %t/foo.a 2>&1 | \
-# RUN:     FileCheck %s -DSYM=_bar -DFILENAME='%t/foo.a(foo.o)'
+# RUN:     FileCheck %s -DSYM=_bar -DLOC='%t/foo.a(foo.o):(symbol _foo+0x1)'
 # RUN: not %lld -o /dev/null %t/main.o -force_load %t/foo.a 2>&1 | \
-# RUN:     FileCheck %s -DSYM=_bar -DFILENAME='%t/foo.a(foo.o)'
+# RUN:     FileCheck %s -DSYM=_bar -DLOC='%t/foo.a(foo.o):(symbol _foo+0x1)'
 # CHECK: error: undefined symbol: [[SYM]]
-# CHECK-NEXT: >>> referenced by [[FILENAME]]
+# CHECK-NEXT: >>> referenced by [[LOC]]
 
 #--- foo.s
 .globl _foo
 .text
 _foo:
   callq _bar
   retq