[lld-macho] Treat undefined symbols uniformly

[int3]

Summary

In particular, we should apply the -undefined behavior to all
such symbols, include those that are specified via the command line
(i.e. -e, -u, and -exported_symbol). ld64 supports this too.

[int3]

[thakis]

Interesting! Do you know of anything that actually does this? Why? Every use case I can think of would better use -U main instead of -undefined dynamic_lookup.

(Change lgtm in any case.)

[thakis]

lld/MachO/Driver.cpp

1152–1153

I think this needs basically the same change. -u _foo -undefined dynamic_lookup forces a dynamically-looked-up symbol foo to exist at load time with ld64. (Can't really think of a use case for that either, but since it's next to the entry point right now and behaves the same way right now, this should probably receive the same treatment in this change.)

[int3]

handle -u and -exported_symbol similarly

[int3]

Bah, I accidentally removed you as the reviewer when doing arc diff --verbatim. I've made a bunch of changes since you stamped it though, might be worth to have another quick look...

Interesting! Do you know of anything that actually does this?

Nope, it was just something that occurred to me to test as I was reviewing our entry symbol handling...

lld/MachO/Driver.cpp

1152–1153

good point... looks like -exported_symbol needs the same treatment too. I've verified that ld64 does apply the same behavior to them, and this diff has been expanded accordingly.

[thakis]

Thanks! Still lg, but more suggestions:

lld/MachO/Driver.cpp

1146–1150

Could we keep this here so all the treatUndefinedSymbol()s are in one place? Or is there something that forces this to move around?

if (config->outputType == MH_EXECUTE && isa<Undefined>(config->entry)) {
   treatUndefinedSymbol(cast<Undefined(config->entry), "the entry point");
   if (isa<Undefined>(config->entry))
     return false;
 }

lld/MachO/SymbolTable.cpp

193

nit: s/&/source/

And if we're doing capturing, do [source, sym] and remove the arg, to make things self-consistent.

(& works here, but with & it's easy to accidentally capture a stack variable, and if the closure escapes that's a recipe for subtle bugs, and compilers don't yet warn on default-capturing locals in escaping closures. So I try to not use default capture.)

201

nit: this reads imho easier like so:

std::string message = "undefined symbol: " + toString(sym);
if (!source.empty())
  message +=  "\n>>> referenced by " + source.str();
else
  message +=  "\n>>> referenced by " + toString(sym.getFile());
return message;

That was source doesn't point to different things based on control flow, and it's fairly linear and straight-forward. (The "referenced by" string is duplicated, but right on top of each other, so it's obvious it's the same)

(It allocates more, but only in a code path that emits diags, so it doesn't normally run anyways.)

[int3]

• move undefined check for entry back into Driver.cpp
• return all undefined symbol errors from -u, not just the first one, similar to what we do for -exported_symbol

lld/MachO/SymbolTable.cpp

193

fair enough. while at it I renamed message to msg so it doesn't shadow the message defined in lld/Common.

[thakis]

lg with writer.cpp change undone

lld/MachO/SymbolTable.cpp

193

meh, the local message string shadows too. i'd keep the old name (but up to you)

194

Since you're doing returns in the conditional now: style guide says no else after return :P

lld/MachO/Writer.cpp

1061

...this change is no longer needed now that this is done in driver, rigth?

[int3]

lld/MachO/SymbolTable.cpp

193

fair point...

194

I don't like this part of the style guide :p Guess I'll go with your original suggestion to preserve symmetry

lld/MachO/Writer.cpp

1061

I swore I deleted it... oops

[int3]

update

lld/MachO/Driver.cpp

for (const Arg *arg : args.filtered(OPT_mllvm))
       parseClangOption(arg->getValue(), arg->getSpelling());
 
     compileBitcodeFiles();
     replaceCommonSymbols();
 
     StringRef orderFile = args.getLastArgValue(OPT_order_file);
     if (!orderFile.empty())
       parseOrderFile(orderFile);
 
-    if (config->outputType == MH_EXECUTE && isa<Undefined>(config->entry)) {
-      error("undefined symbol: " + toString(*config->entry));
-      return false;
-    }
+    if (config->entry)
+      if (auto *undefined = dyn_cast<Undefined>(config->entry))
+        treatUndefinedSymbol(*undefined, "the entry point");
+
     // FIXME: This prints symbols that are undefined both in input files and
     // via -u flag twice.
-    for (const Symbol *undefined : config->explicitUndefineds) {
-      if (isa<Undefined>(undefined)) {
-        error("undefined symbol: " + toString(*undefined) +
-              "\n>>> referenced by flag -u " + toString(*undefined));
-        return false;
-      }
+    for (const Symbol *sym : config->explicitUndefineds) {
+      if (const auto *undefined = dyn_cast<Undefined>(sym))
+        treatUndefinedSymbol(*undefined, "-u");
     }
     // Literal exported-symbol names must be defined, but glob
     // patterns need not match.
     for (const CachedHashStringRef &cachedName :
          config->exportedSymbols.literals) {
       if (const Symbol *sym = symtab->find(cachedName))
-        if (isa<Defined>(sym))
-          continue;
-      error("undefined symbol: " + cachedName.val() +
-            "\n>>> referenced from option -exported_symbol(s_list)");
+        if (const auto *undefined = dyn_cast<Undefined>(sym))
+          treatUndefinedSymbol(*undefined, "-exported_symbol(s_list)");
     }
 
+    // FIXME: should terminate the link early based on errors encountered so
+    // far?
+
     createSyntheticSections();
     createSyntheticSymbols();
 
     for (const Arg *arg : args.filtered(OPT_sectcreate)) {
       StringRef segName = arg->getValue(0);
       StringRef sectName = arg->getValue(1);
       StringRef fileName = arg->getValue(2);
       Optional<MemoryBufferRef> buffer = readFile(fileName);

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 &);
+void treatUndefinedSymbol(const Undefined &, StringRef source = "");
 
 extern SymbolTable *symtab;
 
 } // namespace macho
 } // namespace lld
 
 #endif

lld/MachO/SymbolTable.cpp

Defined *SymbolTable::addSynthetic(StringRef name, InputSection *isec,
                                    bool includeInSymtab) {
   Defined *s = addDefined(name, nullptr, isec, value, /*size=*/0,
                           /*isWeakDef=*/false, isPrivateExtern,
                           /*isThumb=*/false);
   s->includeInSymtab = includeInSymtab;
   return s;
 }
 
-void lld::macho::treatUndefinedSymbol(const Undefined &sym) {
-  auto message = [](const Undefined &sym) {
+void lld::macho::treatUndefinedSymbol(const Undefined &sym, StringRef source) {
+  auto msg = [source, &sym]() {
     std::string message = "undefined symbol: " + toString(sym);
-    std::string fileName = toString(sym.getFile());
-    if (!fileName.empty())
-      message += "\n>>> referenced by " + fileName;
-    return message;
+    if (!source.empty())
+      return message + "\n>>> referenced by " + source.str();
+    else
+      return message + "\n>>> referenced by " + toString(sym.getFile());
   };
   switch (config->undefinedSymbolTreatment) {
   case UndefinedSymbolTreatment::error:
-    error(message(sym));
+    error(msg());
     break;
   case UndefinedSymbolTreatment::warning:
-    warn(message(sym));
+    warn(msg());
     LLVM_FALLTHROUGH;
   case UndefinedSymbolTreatment::dynamic_lookup:
   case UndefinedSymbolTreatment::suppress:
     symtab->addDynamicLookup(sym.getName());
     break;
   case UndefinedSymbolTreatment::unknown:
     llvm_unreachable("unknown -undefined TREATMENT");
   }
 }
 
 SymbolTable *macho::symtab;

lld/MachO/Writer.cpp

void Writer::writeOutputFile() {
   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() {
-  if (config->entry)
+  if (config->entry) {
+    if (auto *undefined = dyn_cast<Undefined>(config->entry))
+      treatUndefinedSymbol(*undefined, "the entry point");
+    if (!isa<Undefined>(config->entry))
       prepareBranchTarget(config->entry);
+  }
+
   scanRelocations();
   if (in.stubHelper->isNeeded())
     in.stubHelper->setup();
   scanSymbols();
   createOutputSections<LP>();
   // No more sections nor segments are created beyond this point.
   sortSegmentsAndSections();
   createLoadCommands<LP>();

lld/test/MachO/entry-symbol.s

 # CHECK-NEXT: addr 0x{{0*}}[[#ENTRY_ADDR]]
 # CHECK-NEXT: size
 # CHECK-NEXT: offset [[#ENTRYOFF:]]
 # CHECK:      cmd  LC_MAIN
 # CHECK-NEXT: cmdsize  24
 # CHECK-NEXT: entryoff [[#ENTRYOFF]]
 
 # RUN: %lld -lSystem -o %t/dysym-main %t/not-main.o %t/libfoo.dylib -e _dysym_main
-# RUN: llvm-objdump --macho --all-headers --indirect-symbols --lazy-bind %t/dysym-main | FileCheck %s --check-prefix=DYSYM
+# RUN: llvm-objdump --macho --all-headers --indirect-symbols --lazy-bind %t/dysym-main | FileCheck %s --check-prefix=DYSYM -DDYLIB=libfoo
+
+# RUN: %lld -lSystem -o %t/dyn-lookup %t/not-main.o -e _dysym_main -undefined dynamic_lookup
+# RUN: llvm-objdump --macho --all-headers --indirect-symbols --lazy-bind %t/dyn-lookup | FileCheck %s --check-prefix=DYSYM -DDYLIB=flat-namespace
+
 # DYSYM-LABEL: Indirect symbols for (__TEXT,__stubs) 1 entries
 # DYSYM-NEXT:  address                      index  name
 # DYSYM-NEXT:  0x[[#%x,DYSYM_ENTRY_ADDR:]]  [[#]]  _dysym_main
 # DYSYM-LABEL: cmd  LC_MAIN
 # DYSYM-NEXT:  cmdsize  24
 # DYSYM-NEXT:  entryoff [[#%u, DYSYM_ENTRY_ADDR - 0x100000000]]
 # DYSYM-LABEL: Lazy bind table:
-# DYSYM-NEXT:  segment  section            address     dylib            symbol
-# DYSYM-NEXT:  __DATA   __la_symbol_ptr    {{.*}}      libfoo       _dysym_main
+# DYSYM-NEXT:  segment  section            address     dylib        symbol
+# DYSYM-NEXT:  __DATA   __la_symbol_ptr    {{.*}}      [[DYLIB]]    _dysym_main
 
 # RUN: %lld -lSystem -o %t/weak-dysym-main %t/not-main.o %t/libfoo.dylib -e _weak_dysym_main
 # RUN: llvm-objdump --macho --all-headers --indirect-symbols --bind --weak-bind %t/weak-dysym-main | FileCheck %s --check-prefix=WEAK-DYSYM
 # WEAK-DYSYM-LABEL: Indirect symbols for (__TEXT,__stubs) 1 entries
 # WEAK-DYSYM-NEXT:  address                      index  name
 # WEAK-DYSYM-NEXT:  0x[[#%x,DYSYM_ENTRY_ADDR:]]  [[#]]  _weak_dysym_main
 # WEAK-DYSYM:       cmd  LC_MAIN
 # WEAK-DYSYM-NEXT:  cmdsize  24
 # WEAK-DYSYM-NEXT:  entryoff [[#%u, DYSYM_ENTRY_ADDR - 0x100000000]]
 # WEAK-DYSYM-LABEL: Bind table:
-# WEAK-DYSYM-NEXT:  segment  section          address  type     addend  dylib      symbol
-# WEAK-DYSYM:       __DATA   __la_symbol_ptr  {{.*}}   pointer       0  libfoo _weak_dysym_main
+# WEAK-DYSYM-NEXT:  segment  section          address  type     addend  dylib   symbol
+# WEAK-DYSYM:       __DATA   __la_symbol_ptr  {{.*}}   pointer       0  libfoo  _weak_dysym_main
 # WEAK-DYSYM-LABEL: Weak bind table:
 # WEAK-DYSYM-NEXT:  segment  section          address  type     addend  symbol
 # WEAK-DYSYM-NEXT:  __DATA   __la_symbol_ptr  {{.*}}   pointer       0  _weak_dysym_main
 
 # RUN: %lld -dylib -lSystem -o %t/dysym-main.dylib %t/not-main.o %t/libfoo.dylib -e _dysym_main
 # RUN: llvm-objdump --macho --indirect-symbols --lazy-bind %t/dysym-main.dylib | FileCheck %s --check-prefix=DYLIB-NO-MAIN
 # DYLIB-NO-MAIN-NOT: _dysym_main
 
 # RUN: not %lld -o /dev/null %t/not-main.o -e _missing 2>&1 | FileCheck %s --check-prefix=UNDEFINED
 # UNDEFINED: error: undefined symbol: _missing
 # RUN: not %lld -o /dev/null %t/not-main.o 2>&1 | FileCheck %s --check-prefix=DEFAULT-ENTRY
-# DEFAULT-ENTRY: error: undefined symbol: _main
+# DEFAULT-ENTRY:      error: undefined symbol: _main
+# DEFAULT-ENTRY-NEXT: >>> referenced by the entry point
 
 #--- libfoo.s
 .text
 .global _dysym_main, _weak_dysym_main
 .weak_definition _weak_dysym_main
 _dysym_main:
   ret
 
 _weak_dysym_main:
   ret
 
 #--- not-main.s
 .text
 .global _not_main
 _not_main:
   ret

lld/test/MachO/export-options.s

 # REQUIRES: x86
 
 # RUN: rm -rf %t; split-file %s %t
 # RUN: llvm-mc -filetype=obj -triple=x86_64-apple-macos %t/default.s -o %t/default.o
 # RUN: llvm-mc -filetype=obj -triple=x86_64-apple-macos %t/lazydef.s -o %t/lazydef.o
 # RUN: llvm-ar --format=darwin rcs %t/lazydef.a %t/lazydef.o
 
-## Check that mixing exported and unexported symbol options yields an error
-# RUN: not %lld -dylib %t/default.o -o /dev/null \
-# RUN:         -exported_symbol a -unexported_symbol b 2>&1 | \
-# RUN:     FileCheck --check-prefix=CONFLICT %s
-
-# CONFLICT: error: cannot use both -exported_symbol* and -unexported_symbol* options
-# CONFLICT-NEXT: >>> ignoring unexports
-
 ## Check that an exported literal name with no symbol definition yields an error
 ## but that an exported glob-pattern with no matching symbol definition is OK
 # RUN: not %lld -dylib %t/default.o -o /dev/null \
 # RUN:         -exported_symbol absent_literal \
 # RUN:         -exported_symbol absent_gl?b 2>&1 | \
 # RUN:     FileCheck --check-prefix=UNDEF %s
 
 # UNDEF: error: undefined symbol: absent_literal
-# UNDEF-NEXT: >>> referenced from option -exported_symbol(s_list)
+# UNDEF-NEXT: >>> referenced by -exported_symbol(s_list)
 # UNDEF-NOT: error: {{.*}} absent_gl{{.}}b
 
+## Check that dynamic_lookup suppresses the error
+# RUN: %lld -dylib %t/default.o -undefined dynamic_lookup -o %t/dyn-lookup \
+# RUN:      -exported_symbol absent_literal
+# RUN: llvm-objdump --macho --syms %t/dyn-lookup | FileCheck %s --check-prefix=DYN
+# DYN: *UND* absent_literal
+
 ## Check that exported literal symbols are present in output's
 ## symbol table, even lazy symbols which would otherwise be omitted
 # RUN: %lld -dylib %t/default.o %t/lazydef.a -o %t/lazydef \
 # RUN:         -exported_symbol _keep_globl \
 # RUN:         -exported_symbol _keep_lazy
 # RUN: llvm-objdump --syms %t/lazydef | \
 # RUN:     FileCheck --check-prefix=EXPORT %s
 

lld/test/MachO/u.s

 # REQUIRES: x86
 # RUN: rm -rf %t; split-file %s %t
 
 # RUN: llvm-mc -filetype=obj -triple=x86_64-apple-macos -o %t/foo.o %t/foo.s
 # RUN: llvm-ar csr %t/lib.a %t/foo.o
 
 # RUN: llvm-mc -filetype=obj -triple=x86_64-apple-macos -o %t/main.o %t/main.s
 
-# RUN: %lld %t/main.o %t/lib.a -o /dev/null -why_load | \
+# RUN: %lld -lSystem %t/main.o %t/lib.a -o /dev/null -why_load | \
 # RUN:     FileCheck %s --check-prefix=NOFOO --allow-empty
 
-# RUN: %lld %t/main.o %t/lib.a -u _foo -o /dev/null -why_load | \
+# RUN: %lld -lSystem %t/main.o %t/lib.a -u _foo -o /dev/null -why_load | \
 # RUN:     FileCheck %s --check-prefix=FOO
 
-# RUN: not %lld %t/main.o %t/lib.a -u _asdf -o /dev/null 2>&1 | \
+# RUN: not %lld %t/main.o %t/lib.a -u _asdf -u _fdsa -o /dev/null 2>&1 | \
 # RUN:     FileCheck %s --check-prefix=UNDEF
 
+# RUN: %lld -lSystem %t/main.o %t/lib.a -u _asdf -undefined dynamic_lookup -o %t/dyn-lookup
+# RUN: llvm-objdump --macho --syms %t/dyn-lookup | FileCheck %s --check-prefix=DYN
+
 # NOFOO-NOT: _foo forced load of {{.+}}lib.a(foo.o)
 # FOO: _foo forced load of {{.+}}lib.a(foo.o)
-# UNDEF: error: undefined symbol: _asdf
+# UNDEF:      error: undefined symbol: _asdf
+# UNDEF-NEXT: >>> referenced by -u
+# UNDEF:      error: undefined symbol: _fdsa
+# UNDEF-NEXT: >>> referenced by -u
+# DYN: *UND* _asdf
 
 #--- foo.s
 .globl _foo
 _foo:
   ret
 
 #--- main.s
 .globl _main
 _main:
   ret