[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

1143

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

1143

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

1140–1141

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

190 ↗

(On Diff #343964)

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.)

198 ↗

(On Diff #343964)

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

190 ↗

(On Diff #343964)

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

194 ↗

(On Diff #344104)

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

190 ↗

(On Diff #343964)

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

lld/MachO/Writer.cpp

1055

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

[int3]

lld/MachO/SymbolTable.cpp

194 ↗

(On Diff #344104)

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

190 ↗

(On Diff #343964)

fair point...

lld/MachO/Writer.cpp

1055

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;
-    }
     // 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;
       }
     }
     // Literal exported-symbol names must be defined, but glob
     // patterns need not match.

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);
+    // treatUndefinedSymbol() can replace sym with a DylibSymbol; re-check.
+    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