[llvm-branch-commits] [lld] 6a348f6 - [lld-macho] Implement `-no_implicit_dylibs`

Jez Ng via llvm-branch-commits llvm-branch-commits at lists.llvm.org
Thu Dec 10 16:03:02 PST 2020


Author: Jez Ng
Date: 2020-12-10T15:57:52-08:00
New Revision: 6a348f6158ecdb7a4bcac3f4cd1d3c5b6e80a550

URL: https://github.com/llvm/llvm-project/commit/6a348f6158ecdb7a4bcac3f4cd1d3c5b6e80a550
DIFF: https://github.com/llvm/llvm-project/commit/6a348f6158ecdb7a4bcac3f4cd1d3c5b6e80a550.diff

LOG: [lld-macho] Implement `-no_implicit_dylibs`

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.

Reviewed By: thakis

Differential Revision: https://reviews.llvm.org/D93000

Added: 
    lld/test/MachO/implicit-dylibs.s

Modified: 
    lld/MachO/Config.h
    lld/MachO/Driver.cpp
    lld/MachO/InputFiles.cpp
    lld/MachO/Options.td
    lld/test/MachO/reexport-stub.s
    lld/test/MachO/stub-link.s

Removed: 
    


################################################################################
diff  --git a/lld/MachO/Config.h b/lld/MachO/Config.h
index 46ce02fb25e0..2835b5c1546e 100644
--- a/lld/MachO/Config.h
+++ b/lld/MachO/Config.h
@@ -36,6 +36,7 @@ struct Configuration {
   bool allLoad = false;
   bool forceLoadObjC = false;
   bool staticLink = false;
+  bool implicitDylibs = false;
   bool isPic = false;
   bool headerPadMaxInstallNames = false;
   bool printEachFile = false;

diff  --git a/lld/MachO/Driver.cpp b/lld/MachO/Driver.cpp
index b8a926279891..469ab801bfe8 100644
--- a/lld/MachO/Driver.cpp
+++ b/lld/MachO/Driver.cpp
@@ -719,6 +719,7 @@ bool macho::link(llvm::ArrayRef<const char *> argsArr, bool canExitEarly,
   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);

diff  --git a/lld/MachO/InputFiles.cpp b/lld/MachO/InputFiles.cpp
index 3e0ad220bff1..e9e71e0c6a40 100644
--- a/lld/MachO/InputFiles.cpp
+++ b/lld/MachO/InputFiles.cpp
@@ -479,7 +479,8 @@ 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 =
@@ -504,6 +505,26 @@ static Optional<DylibFile *> loadReexport(StringRef path, DylibFile *umbrella) {
   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)
@@ -522,17 +543,15 @@ DylibFile::DylibFile(MemoryBufferRef mb, DylibFile *umbrella)
   }
 
   // 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));
@@ -564,8 +583,9 @@ DylibFile::DylibFile(const InterfaceFile &interface, DylibFile *umbrella)
     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));
   };

diff  --git a/lld/MachO/Options.td b/lld/MachO/Options.td
index f00e23bfd791..d1d06879cafa 100644
--- a/lld/MachO/Options.td
+++ b/lld/MachO/Options.td
@@ -246,7 +246,6 @@ def seg1addr : Separate<["-"], "seg1addr">,
      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>">,

diff  --git a/lld/test/MachO/implicit-dylibs.s b/lld/test/MachO/implicit-dylibs.s
new file mode 100644
index 000000000000..292fedb31dde
--- /dev/null
+++ b/lld/test/MachO/implicit-dylibs.s
@@ -0,0 +1,87 @@
+# 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

diff  --git a/lld/test/MachO/reexport-stub.s b/lld/test/MachO/reexport-stub.s
index c63e49812e4d..a1706d19a14c 100644
--- a/lld/test/MachO/reexport-stub.s
+++ b/lld/test/MachO/reexport-stub.s
@@ -15,8 +15,8 @@
 # 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

diff  --git a/lld/test/MachO/stub-link.s b/lld/test/MachO/stub-link.s
index df8d94707d63..a0ee36d700ec 100644
--- a/lld/test/MachO/stub-link.s
+++ b/lld/test/MachO/stub-link.s
@@ -16,7 +16,7 @@
 # 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


        


More information about the llvm-branch-commits mailing list