[lld-macho] Implement `-no_implicit_dylibs`

[int3]

Summary

Dylibs that are "public" -- i.e. top-level system libraries -- are considered
implicitly linked when another library re-exports them. That is, we should load
them & bind directly to their symbols instead of via their re-exporting
umbrella library. This diff implements that behavior by default, as well as an
opt-out flag.

In theory, this is just a performance optimization, but in practice it seems
that it's needed for correctness.

Fixes llvm.org/PR48395.

[thakis]

Thanks! I'll patch this in and verify if it fixes PR48395

lld/MachO/Config.h

39

nit: Because double negation is confusing, naming booleans always for the positive variant ("enableFoo" instead of "disableFoo" etc) tends to make for more readable code. Can we name this implictDylibs? It doesn't make a huge difference for readability in this case, but not having negated booleans is a good general guideline imho.

lld/MachO/InputFiles.cpp

512

nit: isImplicitlyLinked or shouldImplicitlyLink

[thakis]

In D93000#2445641, @thakis wrote:

Thanks! I'll patch this in and verify if it fixes PR48395

It does :) base_unittests now starts up fine on macOS 10.13, and llvm-objdump --macho --dylibs-used out/gnmac/base_unittests shows /usr/lib/libobjc.A.dylib (a whole bunch of times, but D93001 will fix that too).

[int3]

Thanks for verifying the fix!

[int3]

rename

lld/MachO/Config.h

 };
 
 struct Configuration {
   Symbol *entry;
   bool hasReexports = false;
   bool allLoad = false;
   bool forceLoadObjC = false;
   bool staticLink = false;
+  bool implicitDylibs = false;
   bool isPic = false;
   bool headerPadMaxInstallNames = false;
   bool printEachFile = false;
   bool printWhyLoad = false;
   bool searchDylibsFirst = false;
   bool saveTemps = false;
   uint32_t headerPad;
   llvm::StringRef installName;

lld/MachO/Driver.cpp

bool macho::link(llvm::ArrayRef<const char *> argsArr, bool canExitEarly,
   config->printEachFile = args.hasArg(OPT_t);
   config->printWhyLoad = args.hasArg(OPT_why_load);
   config->outputType = getOutputType(args);
   config->ltoObjPath = args.getLastArgValue(OPT_object_path_lto);
   config->runtimePaths = args::getStrings(args, OPT_rpath);
   config->allLoad = args.hasArg(OPT_all_load);
   config->forceLoadObjC = args.hasArg(OPT_ObjC);
   config->demangle = args.hasArg(OPT_demangle);
+  config->implicitDylibs = !args.hasArg(OPT_no_implicit_dylibs);
 
   if (const opt::Arg *arg = args.getLastArg(OPT_static, OPT_dynamic))
     config->staticLink = (arg->getOption().getID() == OPT_static);
 
   config->systemLibraryRoots = getSystemLibraryRoots(args);
   config->librarySearchPaths =
       getLibrarySearchPaths(args, config->systemLibraryRoots);
   config->frameworkSearchPaths =

lld/MachO/InputFiles.cpp

 //
 // ld64 allows a TAPI re-export to reference documents nested within other TBD
 // files, but that seems like a strange design, so this is an intentional
 // deviation.
 const InterfaceFile *currentTopLevelTapi = nullptr;
 
 // Re-exports can either refer to on-disk files, or to documents within .tbd
 // files.
-static Optional<DylibFile *> loadReexport(StringRef path, DylibFile *umbrella) {
+static Optional<DylibFile *> loadReexportHelper(StringRef path,
+                                                DylibFile *umbrella) {
   if (path::is_absolute(path, path::Style::posix))
     for (StringRef root : config->systemLibraryRoots)
       if (Optional<std::string> dylibPath =
               resolveDylibPath((root + path).str()))
         return loadDylib(*dylibPath, umbrella);
 
   // TODO: Expand @loader_path, @executable_path etc
 
   if (currentTopLevelTapi) {
     for (InterfaceFile &child :
          make_pointee_range(currentTopLevelTapi->documents())) {
       if (path == child.getInstallName())
         return make<DylibFile>(child, umbrella);
       assert(child.documents().empty());
     }
   }
 
   if (Optional<std::string> dylibPath = resolveDylibPath(path))
     return loadDylib(*dylibPath, umbrella);
 
   error("unable to locate re-export with install name " + path);
   return {};
 }
 
+// If a re-exported dylib is public (lives in /usr/lib or
+// /System/Library/Frameworks), then it is considered implicitly linked: we
+// should bind to its symbols directly instead of via the re-exporting umbrella
+// library.
+static bool isImplicitlyLinked(StringRef path) {
+  if (!config->implicitDylibs)
+    return false;
+
+  return path::parent_path(path) == "/usr/lib";
+  // TODO: check for public frameworks too. We'll need to implement
+  // -sub_umbrella first to write a test case.
+}
+
+static Optional<DylibFile *> loadReexport(StringRef path, DylibFile *umbrella) {
+  Optional<DylibFile *> reexport = loadReexportHelper(path, umbrella);
+  if (reexport && isImplicitlyLinked(path))
+    inputFiles.push_back(*reexport);
+  return reexport;
+}
+
 DylibFile::DylibFile(MemoryBufferRef mb, DylibFile *umbrella)
     : InputFile(DylibKind, mb) {
   if (umbrella == nullptr)
     umbrella = this;
 
   auto *buf = reinterpret_cast<const uint8_t *>(mb.getBufferStart());
   auto *hdr = reinterpret_cast<const mach_header_64 *>(mb.getBufferStart());
 
   // Initialize dylibName.
   if (const load_command *cmd = findCommand(hdr, LC_ID_DYLIB)) {
     auto *c = reinterpret_cast<const dylib_command *>(cmd);
     dylibName = reinterpret_cast<const char *>(cmd) + read32le(&c->dylib.name);
   } else {
     error("dylib " + toString(this) + " missing LC_ID_DYLIB load command");
     return;
   }
 
   // Initialize symbols.
-  // TODO: if a re-exported dylib is public (lives in /usr/lib or
-  // /System/Library/Frameworks), we should bind to its symbols directly
-  // instead of the re-exporting umbrella library.
+  DylibFile *exportingFile = isImplicitlyLinked(dylibName) ? this : umbrella;
   if (const load_command *cmd = findCommand(hdr, LC_DYLD_INFO_ONLY)) {
     auto *c = reinterpret_cast<const dyld_info_command *>(cmd);
     parseTrie(buf + c->export_off, c->export_size,
               [&](const Twine &name, uint64_t flags) {
                 bool isWeakDef = flags & EXPORT_SYMBOL_FLAGS_WEAK_DEFINITION;
                 bool isTlv = flags & EXPORT_SYMBOL_FLAGS_KIND_THREAD_LOCAL;
-                symbols.push_back(symtab->addDylib(saver.save(name), umbrella,
-                                                   isWeakDef, isTlv));
+                symbols.push_back(symtab->addDylib(
+                    saver.save(name), exportingFile, isWeakDef, isTlv));
               });
   } else {
     error("LC_DYLD_INFO_ONLY not found in " + toString(this));
     return;
   }
 
   if (hdr->flags & MH_NO_REEXPORTED_DYLIBS)
     return;

 }
 
 DylibFile::DylibFile(const InterfaceFile &interface, DylibFile *umbrella)
     : InputFile(DylibKind, interface) {
   if (umbrella == nullptr)
     umbrella = this;
 
   dylibName = saver.save(interface.getInstallName());
+  DylibFile *exportingFile = isImplicitlyLinked(dylibName) ? this : umbrella;
   auto addSymbol = [&](const Twine &name) -> void {
-    symbols.push_back(symtab->addDylib(saver.save(name), umbrella,
+    symbols.push_back(symtab->addDylib(saver.save(name), exportingFile,
                                        /*isWeakDef=*/false,
                                        /*isTlv=*/false));
   };
   // TODO(compnerd) filter out symbols based on the target platform
   // TODO: handle weak defs, thread locals
   for (const auto symbol : interface.symbols()) {
     if (!symbol->getArchitectures().has(config->arch))
       continue;

lld/MachO/Options.td

 def seg1addr : Separate<["-"], "seg1addr">,
      MetaVarName<"<address>">,
      Alias<image_base>,
      HelpText<"Alias for -image_base">,
      Flags<[HelpHidden]>,
      Group<grp_opts>;
 def no_implicit_dylibs : Flag<["-"], "no_implicit_dylibs">,
      HelpText<"Do not optimize public dylib transitive symbol references">,
-     Flags<[HelpHidden]>,
      Group<grp_opts>;
 def exported_symbols_order : Separate<["-"], "exported_symbols_order">,
      MetaVarName<"<file>">,
      HelpText<"Specify frequently-used symbols in <file> to optimize symbol exports">,
      Flags<[HelpHidden]>,
      Group<grp_opts>;
 def no_zero_fill_sections : Flag<["-"], "no_zero_fill_sections">,
      HelpText<"Explicitly store zeroed data in the final image">,

lld/test/MachO/implicit-dylibs.s

+# REQUIRES: x86
+# RUN: rm -rf %t; split-file %s %t
+# RUN: mkdir -p %t/usr/lib/system
+
+# RUN: llvm-mc -filetype=obj -triple=x86_64-apple-darwin %t/libfoo.s -o %t/libfoo.o
+# RUN: llvm-mc -filetype=obj -triple=x86_64-apple-darwin %t/libtoplevel.s -o %t/libtoplevel.o
+# RUN: llvm-mc -filetype=obj -triple=x86_64-apple-darwin %t/libsublevel.s -o %t/libsublevel.o
+## libunused will be used to verify that we load implicit dylibs even if we
+## don't use any symbols they contain.
+# RUN: echo "" | llvm-mc -filetype=obj -triple=x86_64-apple-darwin -o %t/libunused.o
+# RUN: echo "" | llvm-mc -filetype=obj -triple=x86_64-apple-darwin -o %t/reexporter.o
+
+# RUN: %lld -dylib -lSystem %t/libfoo.o -o %t/libfoo.dylib
+# RUN: %lld -dylib -lSystem %t/libtoplevel.o -o %t/usr/lib/libtoplevel.dylib -install_name /usr/lib/libtoplevel.dylib
+# RUN: %lld -dylib -lSystem %t/libsublevel.o -o %t/usr/lib/system/libsublevel.dylib -install_name /usr/lib/system/libsublevel.dylib
+# RUN: %lld -dylib -lSystem %t/libunused.o -o %t/usr/lib/libunused.dylib -install_name /usr/lib/libunused.dylib
+# RUN: %lld -dylib -syslibroot %t \
+# RUN:   -lc++ -ltoplevel -lunused %t/usr/lib/system/libsublevel.dylib %t/libfoo.dylib \
+# RUN:   -sub_library libc++ -sub_library libfoo -sub_library libtoplevel \
+# RUN:   -sub_library libsublevel -sub_library libunused \
+# RUN:   %t/reexporter.o -o %t/libreexporter.dylib
+
+# RUN: llvm-mc -filetype obj -triple x86_64-apple-darwin %t/test.s -o %t/test.o
+# RUN: %lld -syslibroot %t -o %t/test -lSystem -L%t -lreexporter %t/test.o
+# RUN: llvm-objdump --bind --no-show-raw-insn -d %t/test | FileCheck %s
+# CHECK:     Bind table:
+# CHECK-DAG: __DATA __data {{.*}} pointer 0 libreexporter _foo
+# CHECK-DAG: __DATA __data {{.*}} pointer 0 libtoplevel   _toplevel
+# CHECK-DAG: __DATA __data {{.*}} pointer 0 libreexporter _sublevel
+# CHECK-DAG: __DATA __data {{.*}} pointer 0 libc++abi     ___gxx_personality_v0
+
+# RUN: llvm-objdump --macho --all-headers %t/test | FileCheck %s \
+# RUN:   --check-prefix=LOAD -DDIR=%t --implicit-check-not LC_LOAD_DYLIB
+# LOAD:          cmd LC_LOAD_DYLIB
+# LOAD-NEXT: cmdsize
+# LOAD-NEXT:    name /usr/lib/libSystem.B.dylib
+# LOAD:          cmd LC_LOAD_DYLIB
+# LOAD-NEXT: cmdsize
+# LOAD-NEXT:    name /usr/lib/libc++abi.dylib
+# LOAD:          cmd LC_LOAD_DYLIB
+# LOAD-NEXT: cmdsize
+# LOAD-NEXT:    name /usr/lib/libc++.dylib
+# LOAD:          cmd LC_LOAD_DYLIB
+# LOAD-NEXT: cmdsize
+# LOAD-NEXT:    name /usr/lib/libtoplevel.dylib
+# LOAD:          cmd LC_LOAD_DYLIB
+# LOAD-NEXT: cmdsize
+# LOAD-NEXT:    name /usr/lib/libunused.dylib
+# LOAD:          cmd LC_LOAD_DYLIB
+# LOAD-NEXT: cmdsize
+# LOAD-NEXT:    name [[DIR]]/libreexporter.dylib
+
+# RUN: %lld -no_implicit_dylibs -syslibroot %t -o %t/no-implicit -lSystem -L%t -lreexporter %t/test.o
+# RUN: llvm-objdump --bind --no-show-raw-insn -d %t/no-implicit | FileCheck %s --check-prefix=NO-IMPLICIT
+# NO-IMPLICIT:     Bind table:
+# NO-IMPLICIT-DAG: __DATA __data {{.*}} pointer 0 libreexporter _foo
+# NO-IMPLICIT-DAG: __DATA __data {{.*}} pointer 0 libreexporter _toplevel
+# NO-IMPLICIT-DAG: __DATA __data {{.*}} pointer 0 libreexporter _sublevel
+# NO-IMPLICIT-DAG: __DATA __data {{.*}} pointer 0 libreexporter ___gxx_personality_v0
+
+#--- libfoo.s
+.data
+.globl _foo
+_foo:
+
+#--- libtoplevel.s
+.data
+.globl _toplevel
+_toplevel:
+
+#--- libsublevel.s
+.data
+.globl _sublevel
+_sublevel:
+
+#--- test.s
+.text
+.globl _main
+
+_main:
+  ret
+
+.data
+  .quad _foo
+  .quad _toplevel
+  .quad _sublevel
+  .quad ___gxx_personality_v0

lld/test/MachO/reexport-stub.s

 # DYLIB-HEADERS:     cmd     LC_REEXPORT_DYLIB
 # DYLIB-HEADERS-NOT: Load command
 # DYLIB-HEADERS:     name    /usr/lib/libc++.dylib
 
 # RUN: llvm-mc -filetype obj -triple x86_64-apple-darwin %s -o %t/test.o
 # RUN: %lld -o %t/test -lSystem -L%t -lreexporter %t/test.o
 # RUN: llvm-objdump --bind --no-show-raw-insn -d %t/test | FileCheck %s
 
-# CHECK: Bind table:
-# CHECK-DAG: __DATA __data {{.*}} pointer 0 libreexporter ___gxx_personality_v0
+# CHECK:     Bind table:
+# CHECK-DAG: __DATA __data {{.*}} pointer 0 libc++abi ___gxx_personality_v0
 
 .text
 .globl _main
 
 _main:
   ret
 
 .data
   .quad ___gxx_personality_v0

lld/test/MachO/stub-link.s

 # CHECK: movq {{.*}} # [[ADDR:[0-9a-f]+]]
 
 # CHECK: Bind table:
 # CHECK-DAG: __DATA_CONST __got 0x[[ADDR]] pointer 0 libSystem ___nan
 # CHECK-DAG: __DATA __data {{.*}} pointer 0 CoreFoundation _OBJC_CLASS_$_NSObject
 # CHECK-DAG: __DATA __data {{.*}} pointer 0 CoreFoundation _OBJC_METACLASS_$_NSObject
 # CHECK-DAG: __DATA __data {{.*}} pointer 0 CoreFoundation _OBJC_IVAR_$_NSConstantArray._count
 # CHECK-DAG: __DATA __data {{.*}} pointer 0 CoreFoundation _OBJC_EHTYPE_$_NSException
-# CHECK-DAG: __DATA __data {{.*}} pointer 0 libc++         ___gxx_personality_v0
+# CHECK-DAG: __DATA __data {{.*}} pointer 0 libc++abi      ___gxx_personality_v0
 
 .section __TEXT,__text
 .global _main
 
 _main:
 ## This symbol is defined in an inner TAPI document within libSystem.tbd.
   movq ___nan@GOTPCREL(%rip), %rax
   ret