[PATCH] D77087: [lld-macho] Require executables to link against libSystem.dylib

[Jez Ng via Phabricator via llvm-commits]

int3 created this revision.
int3 added reviewers: ruiu, pcc, MaskRay, smeenai, alexshap, gkm, Ktwu, christylee.
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.
int3 marked 2 inline comments as done.
int3 added inline comments.


================
Comment at: lld/MachO/Writer.cpp:351
 
-  // TODO: dyld requires libSystem to be loaded. libSystem is a universal
-  // binary and we don't have support for that yet, so mock it out here.
-  headerSection->addLoadCommand(
-      make<LCLoadDylib>("/usr/lib/libSystem.B.dylib"));
+  if (dylibOrdinal == 1 && config->outputType == MH_EXECUTE) {
+    // Used in tests, as we also run those tests on non-OSX systems.
----------------
Instead of checking for MH_EXECUTABLE, ld64 calls a function called `needsEntryPointLoadCommand()`. Not sure what else that encompasses


================
Comment at: lld/test/MachO/lit.local.cfg:1
+config.environment['LLD_FORCE_LOAD_LIBSYSTEM'] = '1'
 config.substitutions += [
----------------
I debated between using a flag and an environment variable, but since we already have the `LLD_IN_TEST` env var, another one seemed appropriate


Now that we can read universal binaries (D77006 <https://reviews.llvm.org/D77006>), we can properly link against libSystem, so we don't need to mock out that behavior... except in tests, which may run on non-OS X systems.

Depends on D77086 <https://reviews.llvm.org/D77086>.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D77087

Files:
  lld/MachO/Writer.cpp
  lld/test/MachO/lit.local.cfg
  lld/test/MachO/no-libsystem.s


Index: lld/test/MachO/no-libsystem.s
===================================================================
--- /dev/null
+++ lld/test/MachO/no-libsystem.s
@@ -0,0 +1,25 @@
+# REQUIRES: x86
+# RUN: mkdir -p %t
+# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-darwin %s -o %t/no-libsystem.o
+# RUN: LLD_FORCE_LOAD_LIBSYSTEM=0 not %lld -o /dev/null %t/no-libsystem.o 2>&1 \
+# RUN:   | FileCheck %s --check-prefix=ERROR
+# ERROR: executables must link with libSystem.dylib
+
+## Even though we are not loading libSystem here -- just a random dylib -- the
+## error doesn't show up. This mimics ld64's behavior, which only cares that
+## *some* dylib is loaded.
+# RUN: llvm-mc -filetype=obj -triple=x86_64-apple-darwin %p/Inputs/libhello.s \
+# RUN:   -o %t/libhello.o
+# RUN: %lld -dylib -install_name @executable_path/libhello.dylib \
+# RUN:   %t/libhello.o -o %t/libhello.dylib
+# RUN: LLD_FORCE_LOAD_LIBSYSTEM=0 %lld -Z -L%t -lhello -o %t/no-libsystem %t/no-libsystem.o
+# RUN: llvm-readobj --needed-libs %t/no-libsystem | FileCheck %s
+# CHECK:      NeededLibraries [
+# CHECK=NEXT:   @executable_path/libhello.dylib
+# CHECK=NEXT: ]
+
+.text
+.global _main
+_main:
+  mov $0, %rax
+  ret
Index: lld/test/MachO/lit.local.cfg
===================================================================
--- lld/test/MachO/lit.local.cfg
+++ lld/test/MachO/lit.local.cfg
@@ -1,3 +1,4 @@
+config.environment['LLD_FORCE_LOAD_LIBSYSTEM'] = '1'
 config.substitutions += [
     ('%lld', 'lld -flavor darwinnew'),
 ]
Index: lld/MachO/Writer.cpp
===================================================================
--- lld/MachO/Writer.cpp
+++ lld/MachO/Writer.cpp
@@ -348,10 +348,15 @@
     }
   }
 
-  // TODO: dyld requires libSystem to be loaded. libSystem is a universal
-  // binary and we don't have support for that yet, so mock it out here.
-  headerSection->addLoadCommand(
-      make<LCLoadDylib>("/usr/lib/libSystem.B.dylib"));
+  if (dylibOrdinal == 1 && config->outputType == MH_EXECUTE) {
+    // Used in tests, as we also run those tests on non-OSX systems.
+    if (StringRef(getenv("LLD_FORCE_LOAD_LIBSYSTEM")) == "1") {
+      headerSection->addLoadCommand(
+          make<LCLoadDylib>("/usr/lib/libSystem.B.dylib"));
+    } else {
+      error("executables must link with libSystem.dylib");
+    }
+  }
 }
 
 void Writer::scanRelocations() {


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D77087.253685.patch
Type: text/x-patch
Size: 2349 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20200330/f06ef836/attachment-0001.bin>