[lld-macho] Dylib symbols should always replace undefined symbols

[int3]

Summary

Otherwise we get undefined symbol errors depending on the order of
arguments on the command line.

Depends on D78270.

[int3]

update

[int3]

update

[smeenai]

lld/test/MachO/resolution.s

28–29

Where are _bar and _baz defined in this assembly file?

[smeenai]

Not this diff, but it appears ld64 always prefers symbols from object files over symbols from dylibs. For example:

$ cat f1.c
int f() { return 1; }

$ cat f2.c:
int f() { return 2; }

$ cat main.c
int f(void);
int main() { return f(); }

$ clang -fPIC -dynamiclib -o libf1.dylib f1.c
$ clang -c f2.c
$ clang -c main.c

$ ld libf1.dylib main.o f2.o -lSystem
$ ./a.out; echo $?
2

$ ld main.o libf1.dylib f2.o -lSystem
$ ./a.out; echo $?
2

# sanity check
$ ld main.o libf1.dylib -lSystem

I believe we should already be implementing this behavior, courtesy of a Defined replacing any other symbol, but it's worth adding a test.

[int3]

lld/test/MachO/resolution.s

28–29

ah, I was under the impression that I didn't need to define them because they were already appearing in the symtab, but I realize they were appearing as undefined symbols instead of the global/local defined symbols that I wanted. Will fix, thanks

I believe we should already be implementing this behavior, courtesy of a Defined replacing any other symbol, but it's worth adding a test.

Yeah this is what _bar and _baz are supposed to test :)

[int3]

fix test

[smeenai]

lld/test/MachO/resolution.s

28–29

For the particular case I was thinking of, you'd need to have the dylib appear in the link line before the object file, to confirm that a Defined symbol replaces a DylibSymbol.

Also, it's not too relevant for this particular test, but it's an interesting question of what it means to have a GOTPCREL relocation against a symbol in your own binary (especially a symbol that's not even external, like _baz here). ld64 creates GOT entries for those symbols. I believe we won't right now, since our GOT creation is based on whether the relocation is targeting a dylib symbol vs. what the type of the relocation is.

[int3]

lld/test/MachO/resolution.s

28–29

Oh gotcha. And yeah, you're right, we're not handling those GOT relocations quite correctly right now...

[int3]

extend test, also simplify libresolution.s

[MaskRay]

This behavior is reasonable to me but where can we find authoritative symbol resolution rules? (You may be aware of https://lld.llvm.org/NewLLD.html "Efficient archive file handling". The ELF port has different resolution rules regarding archives.)

lld/test/MachO/resolution.s

3

Consider inlining short Inputs/* file:

# RUN: echo '.globl _foo, _bar, _baz; _foo: _bar: _baz' | \
# RUN:   llvm-mc -filetype=obj -triple=x86_64-apple-darwin - -o %t/libresolution.o

[int3]

where can we find authoritative symbol resolution rules?

I guess the authoritative source is whatever ld64 is doing. I took a brief look at their SymbolTable.cpp but it seems rather complex, handling many kinds of symbols. The test behavior in this diff does emulate the observed behavior of ld64 though.

Thanks for the pointer to the ELF resolution rule design, will keep that in mind as we expand support for symbol kinds.

[int3]

define libresolution inline

[smeenai]

LGTM.

In D79114#2021626, @int3 wrote:

where can we find authoritative symbol resolution rules?

I guess the authoritative source is whatever ld64 is doing. I took a brief look at their SymbolTable.cpp but it seems rather complex, handling many kinds of symbols. The test behavior in this diff does emulate the observed behavior of ld64 though.

Thanks for the pointer to the ELF resolution rule design, will keep that in mind as we expand support for symbol kinds.

I added some commentary on ld64's behavior in https://reviews.llvm.org/D78342#inline-728182.

lld/MachO/SymbolTable.cpp

Symbol *SymbolTable::addUndefined(StringRef name) {
   return s;
 }
 
 Symbol *SymbolTable::addDylib(StringRef name, DylibFile *file) {
   Symbol *s;
   bool wasInserted;
   std::tie(s, wasInserted) = insert(name);
 
-  if (wasInserted)
+  if (wasInserted || isa<Undefined>(s))
     replaceSymbol<DylibSymbol>(s, file, name);
   return s;
 }
 
 SymbolTable *macho::symtab;

lld/test/MachO/resolution.s

+# REQUIRES: x86
+# RUN: mkdir -p %t
+# RUN: echo '.globl _foo, _bar, _baz; _foo: _bar: _baz:' | \
+# RUN:   llvm-mc -filetype=obj -triple=x86_64-apple-darwin -o %t/libresolution.o
+# RUN: lld -flavor darwinnew -dylib -install_name \
+# RUN:   @executable_path/libresolution.dylib %t/libresolution.o -o %t/libresolution.dylib
+# RUN: lld -flavor darwinnew -dylib -install_name \
+# RUN:   @executable_path/libresolution2.dylib %t/libresolution.o -o %t/libresolution2.dylib
+# RUN: llvm-mc -filetype=obj -triple=x86_64-apple-darwin %s -o %t/resolution.o
+
+## Check that we select the symbol defined in the first dylib passed on the
+## command line.
+# RUN: lld -flavor darwinnew -o %t/dylib-first -Z -L%t -lresolution -lresolution2 %t/resolution.o
+# RUN: llvm-objdump --macho --bind %t/dylib-first | FileCheck %s --check-prefix=DYLIB-FIRST
+# DYLIB-FIRST:     libresolution _foo
+
+# RUN: lld -flavor darwinnew -o %t/dylib2-first -Z -L%t -lresolution2 -lresolution %t/resolution.o
+# RUN: llvm-objdump --macho --bind %t/dylib2-first | FileCheck %s --check-prefix=DYLIB2-FIRST
+# DYLIB2-FIRST: libresolution2 _foo
+
+## Also check that defined symbols take precedence over dylib symbols.
+# DYLIB-FIRST-NOT: libresolution _bar
+# DYLIB-FIRST-NOT: libresolution _baz
+
+## Check that we pick the dylib symbol over the undefined symbol in the object
+## file, even if the object file appears first on the command line.
+# RUN: lld -flavor darwinnew -o %t/obj-first -Z -L%t %t/resolution.o -lresolution
+# RUN: llvm-objdump --macho --bind %t/obj-first | FileCheck %s --check-prefix=OBJ-FIRST
+# OBJ-FIRST: libresolution _foo
+## But defined symbols should still take precedence.
+# OBJ-FIRST-NOT: libresolution _bar
+# OBJ-FIRST-NOT: libresolution _baz
+
+.globl _main, _bar
+# Global defined symbol
+_bar:
+# Local defined symbol
+_baz:
+
+_main:
+  movq _foo@GOTPCREL(%rip), %rsi
+  movq _bar@GOTPCREL(%rip), %rsi
+  movq _baz@GOTPCREL(%rip), %rsi
+  ret