[lld/mac] Implement support for .weak_def_can_be_hidden

[thakis]

Summary

I first had a more invasive patch for this (D101069), but while trying
to get that polished for review I realized that lld's current symbol
merging semantics mean that only a very small code change is needed.
So this goes with the smaller patch for now.

This has no effect on projects that build with -fvisibility=hidden
(e.g. chromium), since these see .private_extern symbols instead.

It does have an effect on projects that build with -fvisibility-inlines-hidden
(e.g. llvm) in -O2 builds, where LLVM's GlobalOpt pass will promote most inline
functions from .weak_definition to .weak_def_can_be_hidden.

Before this patch:

% ls -l out/gn/bin/clang out/gn/lib/libclang.dylib
-rwxr-xr-x  1 thakis  staff  113059936 Apr 22 11:51 out/gn/bin/clang
-rwxr-xr-x  1 thakis  staff   86370064 Apr 22 11:51 out/gn/lib/libclang.dylib
% out/gn/bin/llvm-objdump --macho --weak-bind out/gn/bin/clang | wc -l
    8291
% out/gn/bin/llvm-objdump --macho --weak-bind out/gn/lib/libclang.dylib | wc -l
    5698

With this patch:

% ls -l out/gn/bin/clang out/gn/lib/libclang.dylib
-rwxr-xr-x  1 thakis  staff  111721096 Apr 22 11:55 out/gn/bin/clang
-rwxr-xr-x  1 thakis  staff   85291208 Apr 22 11:55 out/gn/lib/libclang.dylib
thakis@MBP llvm-project % out/gn/bin/llvm-objdump --macho --weak-bind out/gn/bin/clang | wc -l
     725
thakis@MBP llvm-project % out/gn/bin/llvm-objdump --macho --weak-bind out/gn/lib/libclang.dylib | wc -l
     542

Linking clang becomes a tiny bit faster with this patch:

x 100    0.67263818    0.77847815    0.69430709    0.69877208   0.017715892
+ 100    0.67209601    0.73323393    0.68600798    0.68917346   0.012824377
Difference at 95.0% confidence
        -0.00959861 +/- 0.00428661
        -1.37364% +/- 0.613449%
        (Student's t, pooled s = 0.0154648)

This only happens if lld with the patch and lld without the patch are both
linked with an lld with the patch or both linked with an lld without the patch
(...or with ld64). I accidentally linked the lld with the patch with an lld
without the patch and the other way round at first. In that setup, no
difference is found. That makese sense, since having fewer weak imports will
make the linked output a bit faster too. So not only does this make linking
binaries such as clang a bit faster (since fewer exports need to be written to
the export trie by lld), the linked output binary binary is also a bit faster
(since dyld needs to process fewer dynamic imports).

This also happens to fix the one check-clang failure when using lld as host
linker, but mostly for silly reasons: See crbug.com/1183336, mostly comment 26.
The real bug here is that c-index-test links all of LLVM both statically and
dynamically, which is an ODR violation. Things just happen to work with this
patch.

So after this patch, check-clang, check-lld, check-llvm all pass with lld as
host linker :)

[int3]

love the comments!

lld/MachO/InputFiles.cpp

450

457–459

nit: I assume you aligned the paragraph this way so that the various references to isWeakDefCanBeHidden would line up vertically for easy comparison. To make it clearer, how about changing "and isWeakDefCanBeHidden" to "but isWeakDefCanBeHidden" so it's clearly identical to line 457?

460

Did I understand it correctly?

461

I think the "weak and" is redundant?

lld/test/MachO/weak-def-can-be-hidden.s

20–25

(same for other non-filecheck comments)

I'm wondering if it would be clearer to have this context in InputFiles.cpp 🤔but either seems fine

57

s/arbitary/arbitrary/

but also: maybe "you have to remove the HEADERS check" would be clearer? I had to think for a second to make the link between adding a suffix and ensuring that a -NOT check wouldn't match

[thakis]

Thanks!

lld/MachO/InputFiles.cpp

460

Yes :)

lld/test/MachO/weak-def-can-be-hidden.s

57

There are 3 HEADERS check spread over the file. Adding a suffix is faster :)

lld/MachO/InputFiles.cpp

void ObjFile::parseRelocations(ArrayRef<Section> sectionHeaders,
   }
 }
 
 template <class NList>
 static macho::Symbol *createDefined(const NList &sym, StringRef name,
                                     InputSection *isec, uint64_t value,
                                     uint64_t size) {
   // Symbol scope is determined by sym.n_type & (N_EXT | N_PEXT):
-  // N_EXT: Global symbols
-  // N_EXT | N_PEXT: Linkage unit (think: dylib) scoped
+  // N_EXT: Global symbols. These go in the symbol table during the link,
+  //        and also in the export table of the output so that the dynamic
+  //        linker sees them.
+  // N_EXT | N_PEXT: Linkage unit (think: dylib) scoped. These go in the
+  //                 symbol table during the link so that duplicates are
+  //                 either reported (for non-weak symbols) or merged
+  //                 (for weak symbols), but they do not go in the export
+  //                 table of the output.
   // N_PEXT: Does not occur in input files in practice,
   //         a private extern must be external.
-  // 0: Translation-unit scoped. These are not in the symbol table.
+  // 0: Translation-unit scoped. These are not in the symbol table during
+  //    link, and not in the export table of the output either.
+
+  bool isWeakDefCanBeHidden =
+      (sym.n_desc & (N_WEAK_DEF | N_WEAK_REF)) == (N_WEAK_DEF | N_WEAK_REF);
 
   if (sym.n_type & (N_EXT | N_PEXT)) {
     assert((sym.n_type & N_EXT) && "invalid input");
+    bool isPrivateExtern = sym.n_type & N_PEXT;
+
+    // lld's behavior for merging symbols is slightly different from ld64:
+    // ld64 picks the winning symbol based on several criteria (see
+    // pickBetweenRegularAtoms() in ld64's SymbolTable.cpp), while lld
+    // just merges metadata and keeps the contents of the first symbol
+    // with that name (see SymbolTable::addDefined). For:
+    // * inline function F in a TU built with -fvisibility-inlines-hidden
+    // * and inline function F in another TU built without that flag
+    // ld64 will pick the one from the file built without
+    // -fvisibility-inlines-hidden.
+    // lld will instead pick the one listed first on the link command line and
+    // give it visibility as if the function was built without
+    // -fvisibility-inlines-hidden.
+    // If both functions have the same contents, this will have the same
+    // behavior. If not, it won't, but the input had an ODR violation in
+    // that case.
+    //
+    // Similarly, merging a symbol
+    // that's isPrivateExtern and not isWeakDefCanBeHidden with one
+    // that's not isPrivateExtern but isWeakDefCanBeHidden technically
+    // should produce one
+    // that's not isPrivateExtern but isWeakDefCanBeHidden. That matters
+    // with ld64's semantics, because it means the non-private-extern
+    // definition will continue to take priority if more private extern
+    // definitions are encountered. With lld's semantics there's no observable
+    // difference between a symbol that's isWeakDefCanBeHidden or one that's
+    // privateExtern -- neither makes it into the dynamic symbol table. So just
+    // promote isWeakDefCanBeHidden to isPrivateExtern here.
+    if (isWeakDefCanBeHidden)
+      isPrivateExtern = true;
+
     return symtab->addDefined(name, isec->file, isec, value, size,
-                              sym.n_desc & N_WEAK_DEF, sym.n_type & N_PEXT);
+                              sym.n_desc & N_WEAK_DEF, isPrivateExtern);
   }
+
+  assert(!isWeakDefCanBeHidden &&
+         "weak_def_can_be_hidden on already-hidden symbol?");
   return make<Defined>(name, isec->file, isec, value, size,
                        sym.n_desc & N_WEAK_DEF,
                        /*isExternal=*/false, /*isPrivateExtern=*/false);
 }
 
 // Absolute symbols are defined symbols that do not have an associated
 // InputSection. They cannot be weak.
 template <class NList>

lld/test/MachO/weak-def-can-be-hidden.s

+# REQUIRES: x86
+# RUN: rm -rf %t; split-file %s %t
+
+# RUN: llvm-mc -filetype=obj -triple=x86_64-apple-macos \
+# RUN:     %t/weak-foo.s -o %t/weak-foo.o
+
+# RUN: llvm-mc -filetype=obj -triple=x86_64-apple-macos \
+# RUN:     %t/weak-autohide-foo.s -o %t/weak-autohide-foo.o
+
+# RUN: llvm-mc -filetype=obj -triple=x86_64-apple-macos \
+# RUN:     %t/weak-foo-pe.s -o %t/weak-foo-pe.o
+
+# RUN: llvm-mc -filetype=obj -triple=x86_64-apple-macos \
+# RUN:     %t/weak-autohide-foo-pe.s -o %t/weak-autohide-foo-pe.o
+
+# RUN: llvm-mc -filetype=obj -triple=x86_64-apple-macos \
+# RUN:     %t/ref-foo.s -o %t/ref-foo.o
+
+## Basics: A weak_def_can_be_hidden symbol should not be in the output's
+## export table, nor in the weak bind table, and references to it from
+## within the dylib should not use weak indirect lookups.
+## Think: Inline function compiled without any -fvisibility flags, inline
+## function does not have its address taken. In -O2 compiles, GlobalOpt will
+## upgrade the inline function from .weak_definition to .weak_def_can_be_hidden.
+# RUN: %lld -dylib -o %t/weak-autohide-foo.dylib \
+# RUN:     %t/weak-autohide-foo.o %t/ref-foo.o
+# RUN: llvm-objdump --syms --exports-trie %t/weak-autohide-foo.dylib | \
+# RUN:     FileCheck --check-prefix=EXPORTS %s
+# RUN: llvm-nm -m %t/weak-autohide-foo.dylib | \
+# RUN:     FileCheck --check-prefix=EXPORTS-NM %s
+# RUN: llvm-objdump --macho --bind --weak-bind %t/weak-autohide-foo.dylib | \
+# RUN:     FileCheck --check-prefix=WEAKBIND %s
+# RUN: llvm-objdump --macho --private-header %t/weak-autohide-foo.dylib | \
+# RUN:     FileCheck --check-prefix=HEADERS %s
+
+## ld64 doesn't treat .weak_def_can_be_hidden as weak symbols in the
+## exports trie or bind tables, but it claims they are weak in the symbol
+## table. lld marks them as local in the symbol table, see also test
+## weak-private-extern.s. These FileCheck lines match both outputs.
+# EXPORTS-LABEL: SYMBOL TABLE:
+# EXPORTS-DAG:   [[#%x, FOO_ADDR:]] {{.*}} _foo
+# EXPORTS-LABEL: Exports trie:
+# EXPORTS-NOT:   0x{{0*}}[[#%X, FOO_ADDR]] _foo
+
+## nm output for .weak_def_can_be_hidden says "was a private external" even
+## though it wasn't .private_extern: It was just .weak_def_can_be_hidden.
+## This matches ld64.
+# EXPORTS-NM: (__TEXT,__text) non-external (was a private external) _foo
+
+# WEAKBIND-NOT: __got
+# WEAKBIND-NOT: __la_symbol_ptr
+
+## ld64 sets WEAKBIND and BINDS_TO_WEAK in the mach-o header even though there
+## are no weak bindings or weak definitions after processing the autohide. That
+## looks like a bug in ld64 (?) If you change lit.local.cfg to set %lld to ld to
+## test compatibility, you have to add some arbitrary suffix to these two lines:
+# HEADERS-NOT: WEAK_DEFINES
+# HEADERS-NOT: BINDS_TO_WEAK
+
+## Same behavior for a symbol that's both .weak_def_can_be_hidden and
+## .private_extern. Think: Inline function compiled with
+## -fvisibility-inlines-hidden.
+# RUN: %lld -dylib -o %t/weak-autohide-foo-pe.dylib \
+# RUN:     %t/weak-autohide-foo-pe.o %t/ref-foo.o
+# RUN: llvm-objdump --syms --exports-trie %t/weak-autohide-foo-pe.dylib | \
+# RUN:     FileCheck --check-prefix=EXPORTS %s
+# RUN: llvm-nm -m %t/weak-autohide-foo-pe.dylib | \
+# RUN:     FileCheck --check-prefix=EXPORTS-NM %s
+# RUN: llvm-objdump --macho --bind --weak-bind %t/weak-autohide-foo-pe.dylib | \
+# RUN:     FileCheck --check-prefix=WEAKBIND %s
+# RUN: llvm-objdump --macho --private-header %t/weak-autohide-foo-pe.dylib | \
+# RUN:     FileCheck --check-prefix=HEADERS %s
+
+## In fact, a regular weak symbol that's .private_extern behaves the same
+## as well.
+# RUN: %lld -dylib -o %t/weak-foo-pe.dylib %t/weak-foo-pe.o %t/ref-foo.o
+# RUN: llvm-objdump --syms --exports-trie %t/weak-foo-pe.dylib | \
+# RUN:     FileCheck --check-prefix=EXPORTS %s
+# RUN: llvm-nm -m %t/weak-foo-pe.dylib | \
+# RUN:     FileCheck --check-prefix=EXPORTS-NM %s
+# RUN: llvm-objdump --macho --bind --weak-bind %t/weak-foo-pe.dylib | \
+# RUN:     FileCheck --check-prefix=WEAKBIND %s
+# RUN: llvm-objdump --macho --private-header %t/weak-foo-pe.dylib | \
+# RUN:     FileCheck --check-prefix=HEADERS %s
+
+## Combining a regular weak_definition with a weak_def_can_be_hidden produces
+## a regular weak external.
+# RUN: %lld -dylib -o %t/weak-foo.dylib -lSystem \
+# RUN:     %t/weak-autohide-foo.o %t/weak-foo.o %t/ref-foo.o
+# RUN: llvm-objdump --syms --exports-trie %t/weak-foo.dylib | \
+# RUN:     FileCheck --check-prefix=WEAK %s
+# RUN: llvm-nm -m %t/weak-foo.dylib | \
+# RUN:     FileCheck --check-prefix=WEAK-NM %s
+# RUN: llvm-objdump --macho --bind --weak-bind %t/weak-foo.dylib | \
+# RUN:     FileCheck --check-prefix=WEAK-WEAKBIND %s
+# RUN: llvm-objdump --macho --private-header %t/weak-foo.dylib | \
+# RUN:     FileCheck --check-prefix=WEAK-HEADERS %s
+# WEAK-LABEL: SYMBOL TABLE:
+# WEAK-DAG:   [[#%x, FOO_ADDR:]] w {{.*}} _foo
+# WEAK-LABEL: Exports trie:
+# WEAK-DAG:   0x{{0*}}[[#%X, FOO_ADDR]] _foo
+# WEAK-NM: (__TEXT,__text) weak external _foo
+# WEAK-WEAKBIND: __la_symbol_ptr 0x{{.*}} pointer 0           _foo
+# WEAK-HEADERS: WEAK_DEFINES
+# WEAK-HEADERS: BINDS_TO_WEAK
+
+#--- weak-foo.s
+.globl _foo
+.weak_definition _foo
+_foo:
+  retq
+
+#--- weak-autohide-foo.s
+.globl _foo
+.weak_def_can_be_hidden _foo
+_foo:
+  retq
+
+#--- weak-foo-pe.s
+.private_extern _foo
+.globl _foo
+.weak_definition _foo
+_foo:
+  retq
+
+#--- weak-autohide-foo-pe.s
+.private_extern _foo
+.globl _foo
+.weak_def_can_be_hidden _foo
+_foo:
+  retq
+
+#--- ref-foo.s
+.globl _bar
+_bar:
+  callq _foo
+  retq