[lld/mac] Emit only one LC_LOAD_DYLIB per dylib

[thakis]

Summary

In some cases, we end up with several distinct DylibFiles that
have the same install name. Only emit a single LC_LOAD_DYLIB in
those cases.

This happens in 3 cases I know of:

1. Some tbd files are symlinks. libpthread.tbd is a symlink against libSystem.tbd for example, so -lSystem -lpthread loads libSystem.tbd twice. We could (and maybe should) cache loaded dylibs by realpath() to catch this.

2. Some tbd files are copies of each other. For example, CFNetwork.framework/CFNetwork.tbd and CFNetwork.framework/Versions/A/CFNetwork.tbd are two distinct copies of the same file. The former is found by -framework CFNetwork and the latter by the reexport in CoreServices.tbd. We could conceivably catch this by making -framework search look in Versions/Current instead of in the root, and/or by using a content hash to cache tbd files, but that's starting to sound complicated.

3. Magic $ld$ symbol processing can change the install name of a dylib based on the target platform_version. Here, two truly distinct dylibs can have the same install name.

So we need this code to deal with (3) anyways. Might as well use
it for 1 and 2, at least for now :)

With this (and D103430), clang-format links in the same dylibs
when linked with lld and ld64.

[int3]

lld/MachO/Writer.cpp

681

nit: no need for llvm::

716

lld/test/MachO/dylink-ordinal.s

3

also, TIL about this flag... looks like I can clean up some of our tests :D

6–23

are these really necessary? All these pass in the current implementation (only the LOAD check below fails)... plus they seem pretty similar to what's already in dylink.s

[thakis]

Thanks!

lld/test/MachO/dylink-ordinal.s

6–23

It's all different places that reference the ordinal – seems nice to have the coverage to me. But feel free to delete the bits you don't like :)

lld/MachO/Writer.cpp

template <class LP> void Writer::createLoadCommands() {
   in.header->addLoadCommand(uuidCommand);
 
   if (useLCBuildVersion(config->platformInfo))
     in.header->addLoadCommand(make<LCBuildVersion>(config->platformInfo));
   else
     in.header->addLoadCommand(make<LCMinVersion>(config->platformInfo));
 
   int64_t dylibOrdinal = 1;
+  llvm::DenseMap<llvm::StringRef, int64_t> ordinalForInstallName;
   for (InputFile *file : inputFiles) {
     if (auto *dylibFile = dyn_cast<DylibFile>(file)) {
       if (dylibFile->isBundleLoader) {
         dylibFile->ordinal = BIND_SPECIAL_DYLIB_MAIN_EXECUTABLE;
         // Shortcut since bundle-loader does not re-export the symbols.
 
         dylibFile->reexport = false;
         continue;
       }
 
       // Don't emit load commands for a dylib that is not referenced if:
       // - it was added implicitly (via a reexport, an LC_LOAD_DYLINKER --
       //   if it's on the linker command line, it's explicit)
       // - or it's marked MH_DEAD_STRIPPABLE_DYLIB
       // - or the flag -dead_strip_dylibs is used
       // FIXME: `isReferenced()` is currently computed before dead code
       // stripping, so references from dead code keep a dylib alive. This
       // matches ld64, but it's something we should do better.
       if (!dylibFile->isReferenced() &&
           (!dylibFile->explicitlyLinked || dylibFile->deadStrippable ||
            config->deadStripDylibs))
         continue;
 
-      dylibFile->ordinal = dylibOrdinal++;
+      // Several DylibFiles can have the same installName. Only emit a single
+      // load command for that installName and give all these DylibFiles the
+      // same ordinal.
+      // This can happen if:
+      // - a new framework could change its installName to an older
+      //   framework name via an $ld$ symbol depending on platform_version
+      // - symlink (eg libpthread.tbd is a symlink to libSystem.tbd)
+      // - a framework can be linked both explicitly on the linker
+      //   command line and implicitly as a reexport from a different
+      //   framework. The re-export will usually point to the tbd file
+      //   in Foo.framework/Versions/A/Foo.tbd, while the explicit link will
+      //   usually find Foo.framwork/Foo.tdb. These are usually two identical
+      //   but distinct files (concrete example: CFNetwork.framework, reexported
+      //   from CoreServices.framework).
+      // In the first case, *semantically distinct* DylibFiles will have the
+      // same installName.
+      int64_t &ordinal = ordinalForInstallName[dylibFile->dylibName];
+      if (ordinal) {
+        dylibFile->ordinal = ordinal;
+        continue;
+      }
+
+      ordinal = dylibFile->ordinal = dylibOrdinal++;
       LoadCommandType lcType =
           dylibFile->forceWeakImport || dylibFile->refState == RefState::Weak
               ? LC_LOAD_WEAK_DYLIB
               : LC_LOAD_DYLIB;
       in.header->addLoadCommand(make<LCDylib>(lcType, dylibFile->dylibName,
                                               dylibFile->compatibilityVersion,
                                               dylibFile->currentVersion));
 

lld/test/MachO/dylink-ordinal.s

+# REQUIRES: x86
+
+# --no-leading-lines needed for .tbd files.
+# RUN: rm -rf %t; split-file --no-leading-lines %s %t
+
+# RUN: llvm-mc -filetype=obj -triple=x86_64-apple-darwin %t/main.s -o %t/main.o
+# RUN: %lld -o %t/main -L%t -lFoo -lBar -lSystem %t/main.o
+# RUN: llvm-objdump --lazy-bind -d --no-show-raw-insn %t/main | FileCheck %s
+
+# CHECK: callq 0x[[#%x,FOO_OFF:]]
+# CHECK-NEXT: callq 0x[[#%x,BAR_OFF:]]
+
+# CHECK: [[#%x,BAR_OFF]]: jmpq {{.*}} # [[#%x,BAR_BIND:]]
+# CHECK: [[#%x,FOO_OFF]]: jmpq {{.*}} # [[#%x,FOO_BIND:]]
+
+# CHECK-LABEL: Lazy bind table:
+# CHECK-DAG: __DATA __la_symbol_ptr 0x[[#%x,FOO_BIND]] Foo _foo
+# CHECK-DAG: __DATA __la_symbol_ptr 0x[[#%x,BAR_BIND]] Foo _bar
+
+# RUN: llvm-nm -m %t/main | FileCheck --check-prefix=NM %s
+
+# NM-DAG: _bar (from Foo)
+# NM-DAG: _foo (from Foo)
+
+# RUN: llvm-otool -L %t/main | FileCheck %s --check-prefix=LOAD
+
+# LOAD: Foo.dylib
+# LOAD-NOT: Foo.dylib
+
+#--- libFoo.tbd
+--- !tapi-tbd
+tbd-version:      4
+targets:          [ x86_64-macos ]
+uuids:
+  - target:       x86_64-macos
+    value:        00000000-0000-0000-0000-000000000000
+install-name:     'Foo.dylib'
+current-version:  0001.001.1
+exports:
+  - targets:      [ x86_64-macos ]
+    symbols:      [ _foo ]
+
+#--- libBar.tbd
+--- !tapi-tbd
+tbd-version:      4
+targets:          [ x86_64-macos ]
+uuids:
+  - target:       x86_64-macos
+    value:        00000000-0000-0000-0000-000000000000
+## Also uses Foo.dylib as install-name!
+## Normally, this would happen conditionally via an $ld$ symbol.
+install-name:     'Foo.dylib'
+current-version:  0001.001.1
+exports:
+  - targets:      [ x86_64-macos ]
+    symbols:      [ _bar ]
+
+#--- main.s
+.section __TEXT,__text
+.globl _main, _foo, _bar
+
+_main:
+  callq _foo
+  callq _bar
+  ret