[lld-macho] Group undefined symbol diagnostics by symbol

[BertalanD]

Summary

ld64.lld used to print the "undefined symbol" line for each reference to
an undefined symbol previously:

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

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

Now they are deduplicated:

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

As with the other lld ports, only the first 3 references are printed.

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

[int3]

lld/MachO/SymbolTable.cpp

349–355

Something like -u isn't really a location. It makes more sense to me to use Loc to mean section+offset, like LLD-ELF is doing.

How about calling this something like namedSources, given that treatUndefinedSymbol already calls the same value source?

360

371

codebase convention is to use explicit types instead of auto unless the type is already mentioned on the same line, or if it's some complicated e.g. iterator type

[BertalanD]

Spelled out type names, changed code to use just the macho namespace and tweaked namedLocs's name.

[BertalanD]

Thank you for the review. I hope I fixed everything you pointed out.

lld/MachO/SymbolTable.cpp

349–355

That's a much better name, thanks for the suggestion!

Is relocLocs fine as is, or do you have a better idea for it? I know it doesn't have the best name.

360

Changed the other two functions that had the incorrect namespace. I'm touching them anyways, so I hope it's fine to include that change in this patch.

[thakis]

Looks great!

Note to self, for reference: ELF was D63344, COFF was D63646.

[thakis]

lld/MachO/SymbolTable.cpp

349–355

Maybe codeReferences for the relocs and otherReferences for the named ones? But the current names work for me personally too. Let's see if Jez has a preference.

[int3]

lld/MachO/SymbolTable.cpp

349–355

I like @thakis' idea :)

360

looks good, thanks!

[BertalanD]

Applied naming suggestions.

[kstoimenov]

This might be breaking sanitizer builds: https://lab.llvm.org/buildbot/#/builders/5/builds/24787/steps/19/logs/stdio

Could you please take a look?
Thanks!

FAIL: lld :: MachO/arch-multiple.s (66137 of 66440)
******************** TEST 'lld :: MachO/arch-multiple.s' FAILED ********************
Script:
--
: 'RUN: at line 3';   /b/sanitizer-x86_64-linux-fast/build/llvm_build_ubsan/bin/llvm-mc -filetype=obj -triple=x86_64-apple-macos -o /b/sanitizer-x86_64-linux-fast/build/llvm_build_ubsan/tools/lld/test/MachO/Output/arch-multiple.s.tmp.o /b/sanitizer-x86_64-linux-fast/build/llvm-project/lld/test/MachO/arch-multiple.s
: 'RUN: at line 4';   not ld64.lld -arch x86_64 -platform_version macos 11.0 11.0 -syslibroot /b/sanitizer-x86_64-linux-fast/build/llvm-project/lld/test/MachO/Inputs/MacOSX.sdk -fatal_warnings -o /b/sanitizer-x86_64-linux-fast/build/llvm_build_ubsan/tools/lld/test/MachO/Output/arch-multiple.s.tmp.out -arch_multiple /b/sanitizer-x86_64-linux-fast/build/llvm_build_ubsan/tools/lld/test/MachO/Output/arch-multiple.s.tmp.o 2>&1 | /b/sanitizer-x86_64-linux-fast/build/llvm_build_ubsan/bin/FileCheck /b/sanitizer-x86_64-linux-fast/build/llvm-project/lld/test/MachO/arch-multiple.s
--
Exit Code: 1
Command Output (stderr):
--
/b/sanitizer-x86_64-linux-fast/build/llvm-project/lld/test/MachO/arch-multiple.s:6:10: error: CHECK: expected string not found in input
# CHECK: error: undefined symbol for arch x86_64: _foo

[BertalanD]

@kstoimenov

I had a quick look at it, but I couldn't yet figure out what went wrong just by looking at the source code.

I'm going to try to set up a similar ASan bootstrapping build overnight, and thakis (who's mentoring me for GSoC) is aware of it as well and said that he will investigate too.

I don't have commit access, so feel free to make the revert commit for me in the meantime.

edit: d'oh, I meant to say UBSan

[thakis]

I can reproduce the failures with use_ubsan=true (…after D127906). (I can't repro them with use_asan=true fwiw.) Investigating.

(Even with this reverted locally, ELF/version-script-complex-wildcards.s still fails for me under ubsan. That's due to a different change. Or maybe my local setup is somehow weird.)

[thakis]

This is due to an ODR violation: Both lld/ELF/Relocations.cpp and lld/MachO/SymbolTable.cpp now define a ::UndefinedDiag type, and so they both have the same name for the implicit move ctor etc. Since those are inline, the linker just picks one of the two UndefinedDiag::UndefinedDiag() move ctors at random and uses it to move both types, even though they are distinct.

This fixes it, for example:

diff --git a/lld/MachO/SymbolTable.cpp b/lld/MachO/SymbolTable.cpp
index 7705fbd5f667..5bc622126071 100644
--- a/lld/MachO/SymbolTable.cpp
+++ b/lld/MachO/SymbolTable.cpp
@@ -345,6 +345,7 @@ static bool recoverFromUndefinedSymbol(const Undefined &sym) {
   return false;
 }
 
+namespace {
 struct UndefinedDiag {
   struct SectionAndOffset {
     const InputSection *isec;
@@ -355,7 +356,8 @@ struct UndefinedDiag {
   std::vector<std::string> otherReferences;
 };
 
-static MapVector<const Undefined *, UndefinedDiag> undefs;
+MapVector<const Undefined *, UndefinedDiag> undefs;
+}
 
 void macho::reportPendingUndefinedSymbols() {
   for (const auto &undef : undefs) {

I'll reland with that fix.

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 reportPendingUndefinedSymbols();
+
+// Call reportPendingUndefinedSymbols() to emit diagnostics.
 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 bool recoverFromUndefinedSymbol(const Undefined &sym) {
 
   // 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) {
+struct UndefinedDiag {
+  struct SectionAndOffset {
+    const InputSection *isec;
+    uint64_t offset;
+  };
+
+  std::vector<std::string> namedLocs;
+  std::vector<SectionAndOffset> relocLocs;
+};
+
+static MapVector<const Undefined *, UndefinedDiag> undefs;
+
+void lld::macho::reportPendingUndefinedSymbols() {
+  for (const auto &undef : undefs) {
+    const auto &locations = undef.second;
+
     std::string message = "undefined symbol";
     if (config->archMultiple)
       message += (" for arch " + getArchitectureName(config->arch())).str();
-  message += (": " + name + "\n>>> referenced by " + source).str();
+    message += ": " + toString(*undef.first);
+
+    const size_t maxUndefinedReferences = 3;
+    size_t i = 0;
+    for (const auto &named : locations.namedLocs) {
+      if (i >= maxUndefinedReferences)
+        break;
+      message += "\n>>> referenced by " + named;
+      ++i;
+    }
+
+    for (const auto &ref : locations.relocLocs) {
+      if (i >= maxUndefinedReferences)
+        break;
+      // TODO: Get source file/line from debug information.
+      message += "\n>>> referenced by " + ref.isec->getLocation(ref.offset);
+      ++i;
+    }
+
+    size_t totalReferences =
+        locations.namedLocs.size() + locations.relocLocs.size();
+    if (totalReferences > i)
+      message +=
+          ("\n>>> referenced " + Twine(totalReferences - i) + " more times")
+              .str();
 
     if (config->undefinedSymbolTreatment == UndefinedSymbolTreatment::error)
       error(message);
     else if (config->undefinedSymbolTreatment ==
              UndefinedSymbolTreatment::warning)
       warn(message);
     else
-    assert(false && "diagnostics make sense for -undefined error|warning only");
+      assert(false &&
+             "diagnostics make sense for -undefined error|warning only");
+  }
+
+  // This function is called multiple times during execution. Clear the printed
+  // diagnostics to avoid printing the same things again the next time.
+  undefs.clear();
 }
 
 void lld::macho::treatUndefinedSymbol(const Undefined &sym, StringRef source) {
   if (recoverFromUndefinedSymbol(sym))
     return;
-  printUndefinedDiagnostic(sym.getName(), source);
+
+  undefs[&sym].namedLocs.push_back(source.str());
 }
 
 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));
+  undefs[&sym].relocLocs.push_back({isec, offset});
 }
 
 std::unique_ptr<SymbolTable> macho::symtab;

lld/MachO/Writer.cpp

 void Writer::writeCodeSignature() {
   if (codeSignatureSection)
     codeSignatureSection->writeHashes(buffer->getBufferStart());
 }
 
 void Writer::writeOutputFile() {
   TimeTraceScope timeScope("Write output file");
   openFile();
+  reportPendingUndefinedSymbols();
   if (errorCount())
     return;
   writeSections();
   writeUuid();
   writeCodeSignature();
 
   if (auto e = buffer->commit())
     error("failed to write to the output file: " + toString(std::move(e)));
 }
 
 template <class LP> void Writer::run() {
   treatSpecialUndefineds();
   if (config->entry && !isa<Undefined>(config->entry))
     prepareBranchTarget(config->entry);
 
   // Canonicalization of all pointers to InputSections should be handled by
   // these two scan* methods. I.e. from this point onward, for all live
   // InputSections, we should have `isec->canonical() == isec`.
   scanSymbols();
   scanRelocations();
 
   // Do not proceed if there was an undefined symbol.
+  reportPendingUndefinedSymbols();
   if (errorCount())
     return;
 
   if (in.stubHelper->isNeeded())
     in.stubHelper->setup();
   // At this point, we should know exactly which output sections are needed,
   // courtesy of scanSymbols() and scanRelocations().
   createOutputSections<LP>();

lld/test/MachO/invalid/undef-multi.s

+# REQUIRES: aarch64
+
+# RUN: llvm-mc -filetype=obj -triple=arm64-apple-darwin %s -o %t.o
+# RUN: not %lld -arch arm64 %t.o -o /dev/null 2>&1 | FileCheck -DFILE=%t.o %s
+
+# CHECK: error: undefined symbol: _undef
+# CHECK-NEXT: >>> referenced by [[FILE]]:(symbol _main+0x0)
+# CHECK-NEXT: >>> referenced by [[FILE]]:(symbol _foo+0x0)
+# CHECK-NEXT: >>> referenced by [[FILE]]:(symbol _bar+0x0)
+# CHECK-NEXT: >>> referenced 1 more times
+
+.globl _main
+_main:
+    b _undef
+
+.globl _foo
+_foo:
+    b _undef
+
+.global _bar
+_bar:
+    b _undef
+
+.globl _baz
+_baz:
+    b _undef
+
+.subsections_via_symbols