[PATCH] D134058: [lld-macho] Support -dyld_env

[Nico Weber via Phabricator via llvm-commits]

thakis added inline comments.


================
Comment at: lld/MachO/Driver.cpp:1414
+  if (const Arg *arg = args.getLastArg(OPT_dyld_env)) {
+    if (config->outputType != MH_EXECUTE)
+      error("-dyld_env can only be used when creating executable output");
----------------
This looks involved enough that it could go into a `parse...()` helper in DriverUtils.


================
Comment at: lld/MachO/Driver.cpp:1418
+    // DYLD_<something>_PATH=<some_value>.
+    StringRef dyldEnv = saver().save(arg->getValue());
+    if (!dyldEnv.startswith("DYLD_") || !dyldEnv.contains("_PATH="))
----------------
Do we have to save() this? We don't change it, do we?


================
Comment at: lld/MachO/Driver.cpp:1419
+    StringRef dyldEnv = saver().save(arg->getValue());
+    if (!dyldEnv.startswith("DYLD_") || !dyldEnv.contains("_PATH="))
+      error("-dyld_env malformed value. Expected "
----------------
ld64 doesn't seem to check for this (?)


================
Comment at: lld/MachO/Writer.cpp:420
+  void writeTo(uint8_t *buf) const override {
+    auto *c = reinterpret_cast<rpath_command *>(buf);
+    buf += sizeof(rpath_command);
----------------
s/rpath_command/dyld_env_command/ probably?


================
Comment at: lld/MachO/Writer.cpp:849
 
+  if (!config->dyldEnvs.empty()) {
+    for (const auto &dyldEnv : config->dyldEnvs)
----------------
no need for the if, the loop does 0 iterations if dyldEnvs is empty


================
Comment at: lld/test/MachO/dyld-env.s:1
+# REQUIRES: x86, shell
+
----------------
why does this need shell?


================
Comment at: lld/test/MachO/dyld-env.s:10
+# RUN: mkdir -p %t/Foo.framework/Versions/A
+# RUN: %lld -dylib -install_name %t/Foo.framework/Versions/A/Foo %t/foo.o -o %t/Foo.framework/Versions/A/Foo
+# RUN: ln -sf A %t/Foo.framework/Versions/Current
----------------
check that this errors when making dylibs


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134058/new/

https://reviews.llvm.org/D134058