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

[Vy Nguyen via Phabricator via llvm-commits]

oontvoo marked an inline comment as done.
oontvoo added inline comments.


================
Comment at: lld/MachO/Driver.cpp:1413
   }
+  if (const Arg *arg = args.getLastArg(OPT_dyld_env)) {
+    if (config->outputType != MH_EXECUTE)
----------------
BertalanD wrote:
> This will ignore all but the last `-dyld_env` argument. Does ld64 let you specify it more than once?
Yes - added support for that. Thanks!


================
Comment at: lld/MachO/Driver.cpp:1417
+    // Validate that the argument is in the form
+    // DYLD_<something>_PATH=<some_value>.
+    StringRef dyldEnv = saver().save(arg->getValue());
----------------
thevinster wrote:
> Should `something` be validated here? Is it limited to a list of known possible values that could be enumerated or is it valid to have `DYLD_FOO_PATH=foo` embedded in the binary? 
relaxed the validations (per thakis's comment below), so I guess we don't even need to check for this


================
Comment at: lld/test/MachO/dyld-env.s:2
+# REQUIRES: x86_64, shell
+## Requires 64 bit because the cmdsize is platform dependent.
+
----------------
int3 wrote:
> not sure how platform-dependence factors in here. shouldn't it be enough that we are specifying `-arch x86_64`?
no, it was platform-dependence because of the test path (which is part of the DYLD_FRAMEWORK_PATH value. I've changed the tests to use relative paths instead.




================
Comment at: lld/test/MachO/dyld-env.s:1
+# REQUIRES: x86, shell
+
----------------
thevinster wrote:
> thakis wrote:
> > why does this need shell?
> I believe shell is needed to run the symlinks. Alternatively, we could make this test not be supported on windows. 
Yes - it was needed because of `ln`, but I guess we dont really need it in this tests. (ie., the paths don't need to exist...)
Removed all the shell stuff.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134058