[lld-macho] Support -dyld_env

[oontvoo]

Summary

This arg is undocumented but from looking at the code + experiment, it's used to add additional DYLD_ENVIRONMENT load commands to the output.

[thakis]

lld/MachO/Driver.cpp

1414

This looks involved enough that it could go into a parse...() helper in DriverUtils.

1418

Do we have to save() this? We don't change it, do we?

1419

ld64 doesn't seem to check for this (?)

lld/MachO/Writer.cpp

420

s/rpath_command/dyld_env_command/ probably?

849

no need for the if, the loop does 0 iterations if dyldEnvs is empty

lld/test/MachO/dyld-env.s

2

why does this need shell?

11

check that this errors when making dylibs

[thakis]

(also, test fails on CI)

[oontvoo]

required x86-64 (instead of just x86) because the cmdsize is platform dependent

[BertalanD]

lld/MachO/Driver.cpp

1413

This will ignore all but the last -dyld_env argument. Does ld64 let you specify it more than once?

[int3]

lld/test/MachO/dyld-env.s

2

not sure how platform-dependence factors in here. shouldn't it be enough that we are specifying -arch x86_64?

[thevinster]

lld/MachO/Driver.cpp

1417

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?

lld/test/MachO/dyld-env.s

2

I believe shell is needed to run the symlinks. Alternatively, we could make this test not be supported on windows.

[oontvoo]

addressed review comments:

• relax the value validations
• added supports for specifying -dyld_env multiple times
• more tests

[oontvoo]

lld/MachO/Driver.cpp

1413

Yes - added support for that. Thanks!

1417

relaxed the validations (per thakis's comment below), so I guess we don't even need to check for this

lld/test/MachO/dyld-env.s

2

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.

2

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.

[int3]

lgtm

lld/MachO/Driver.cpp

1414–1415

total nit but I think it's clearer this way. I don't actually know off the top of my head what strchr returns in error cases (though I can certainly infer based on how it's used here)

1416

grammar nit: malformed -dyld_env value or -dyld_env's argument is malformed

1418

if we construct envPair as a StringRef then I don't think we'll need the Twine() here

lld/MachO/Options.td

1215–1221

can we place this along with the rest of the grp_rare options? No need for the leading/trailing newlines too

[thakis]

lld/MachO/Writer.cpp

849

(not done?)

[oontvoo]

addressed review comments

lld/MachO/Writer.cpp

849

Done! (for real)

[int3]

lld/MachO/Options.td

1215–1220

looks like trailing spaces

[oontvoo]

removed trailing spaces

[oontvoo]

The windows CI failures look unrelated, so I'm gonna go ahead and commit this. Thanks, all, for the reviews!

lld/MachO/Config.h

struct Configuration {
 
   SymtabPresence localSymbolsPresence = SymtabPresence::All;
   SymbolPatterns localSymbolPatterns;
 
   bool zeroModTime = false;
 
   llvm::StringRef osoPrefix;
 
+  std::vector<llvm::StringRef> dyldEnvs;
+
   llvm::MachO::Architecture arch() const { return platformInfo.target.Arch; }
 
   llvm::MachO::PlatformType platform() const {
     return platformInfo.target.Platform;
   }
 };
 
 extern std::unique_ptr<Configuration> config;
 
 } // namespace macho
 } // namespace lld
 
 #endif

lld/MachO/Driver.cpp

bool macho::link(ArrayRef<const char *> argsArr, llvm::raw_ostream &stdoutOS,
   config->omitDebugInfo = args.hasArg(OPT_S);
   config->errorForArchMismatch = args.hasArg(OPT_arch_errors_fatal);
   if (const Arg *arg = args.getLastArg(OPT_bundle_loader)) {
     if (config->outputType != MH_BUNDLE)
       error("-bundle_loader can only be used with MachO bundle output");
     addFile(arg->getValue(), LoadType::CommandLine, /*isLazy=*/false,
             /*isExplicit=*/false, /*isBundleLoader=*/true);
   }
+  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");
+    // Validate that the argument is in the form
+    // DYLD_<something>_PATH=<some_value>.
+    StringRef dyldEnv = saver().save(arg->getValue());
+    if (!dyldEnv.startswith("DYLD_") || !dyldEnv.contains("_PATH="))
+      error("-dyld_env malformed value. Expected "
+            "-dyld_env=DYLD_<name>_PATH=<path>, got " +
+            Twine(dyldEnv));
+    else
+      config->dyldEnvs.push_back(dyldEnv);
+  }
+
   if (const Arg *arg = args.getLastArg(OPT_umbrella)) {
     if (config->outputType != MH_DYLIB)
       warn("-umbrella used, but not creating dylib");
     config->umbrella = arg->getValue();
   }
   config->ltoObjPath = args.getLastArgValue(OPT_object_path_lto);
   config->ltoo = args::getInteger(args, OPT_lto_O, 2);
   if (config->ltoo > 3)

lld/MachO/Options.td

def no_data_in_code_info : Flag<["-"], "no_data_in_code_info">,
     HelpText<"Do not emit data-in-code information">,
     Group<grp_undocumented>;
 def debug_snapshot : Flag<["-"], "debug_snapshot">,
     HelpText<"This option is undocumented in ld64">,
     Flags<[HelpHidden]>,
     Group<grp_undocumented>;
 def demangle : Flag<["-"], "demangle">,
     HelpText<"Demangle symbol names in diagnostics">;
-def dyld_env : Flag<["-"], "dyld_env">,
-    HelpText<"This option is undocumented in ld64">,
-    Flags<[HelpHidden]>,
-    Group<grp_undocumented>;
+    
+def dyld_env : Separate<["-"], "dyld_env">,
+    MetaVarName<"<dyld_env_var>">,
+    HelpText<"Specifies a LC_DYLD_ENVIRONMENT variable value pair.">,
+    Group<grp_rare>;
+    
 def encryptable : Flag<["-"], "encryptable">,
     HelpText<"Generate the LC_ENCRYPTION_INFO load command">,
     Group<grp_undocumented>;
 def no_encryption : Flag<["-"], "no_encryption">,
     HelpText<"Do not generate the LC_ENCRYPTION_INFO load command">,
     Group<grp_undocumented>;
 def executable_path : Flag<["-"], "executable_path">,
     HelpText<"This option is undocumented in ld64">,
     Flags<[HelpHidden]>,

lld/MachO/Writer.cpp

void writeTo(uint8_t *buf) const override {
     memcpy(buf, path.data(), path.size());
     buf[path.size()] = '\0';
   }
 
 private:
   StringRef path;
 };
 
+class LCDyldEnv final : public LoadCommand {
+public:
+  explicit LCDyldEnv(StringRef name) : name(name) {}
+
+  uint32_t getSize() const override {
+    return alignTo(sizeof(dyld_env_command) + name.size() + 1,
+                   target->wordSize);
+  }
+
+  void writeTo(uint8_t *buf) const override {
+    auto *c = reinterpret_cast<rpath_command *>(buf);
+    buf += sizeof(rpath_command);
+
+    c->cmd = LC_DYLD_ENVIRONMENT;
+    c->cmdsize = getSize();
+    c->path = sizeof(dyld_env_command);
+
+    memcpy(buf, name.data(), name.size());
+    buf[name.size()] = '\0';
+  }
+
+private:
+  StringRef name;
+};
+
 class LCMinVersion final : public LoadCommand {
 public:
   explicit LCMinVersion(const PlatformInfo &platformInfo)
       : platformInfo(platformInfo) {}
 
   uint32_t getSize() const override { return sizeof(version_min_command); }
 
   void writeTo(uint8_t *buf) const override {
in.header->addLoadCommand(make<LCDylib>(lcType, dylibFile->installName,
                                             dylibFile->compatibilityVersion,
                                             dylibFile->currentVersion));
 
     if (dylibFile->reexport)
       in.header->addLoadCommand(
           make<LCDylib>(LC_REEXPORT_DYLIB, dylibFile->installName));
   }
 
+  if (!config->dyldEnvs.empty()) {
+    for (const auto &dyldEnv : config->dyldEnvs)
+      in.header->addLoadCommand(make<LCDyldEnv>(dyldEnv));
+  }
+
   if (functionStartsSection)
     in.header->addLoadCommand(make<LCFunctionStarts>(functionStartsSection));
   if (dataInCodeSection)
     in.header->addLoadCommand(make<LCDataInCode>(dataInCodeSection));
   if (codeSignatureSection)
     in.header->addLoadCommand(make<LCCodeSignature>(codeSignatureSection));
 
   const uint32_t MACOS_MAXPATHLEN = 1024;

lld/test/MachO/dyld-env.s

+# REQUIRES: x86_64, shell
+## Requires 64 bit because the cmdsize is platform dependent.
+
+# RUN: rm -rf %t; split-file %s %t
+# RUN: mkdir %t/subdir
+
+# RUN: llvm-mc -filetype obj -triple x86_64-apple-darwin %t/main.s -o %t/main.o
+# RUN: llvm-mc -filetype obj -triple x86_64-apple-darwin %t/foo.s -o %t/foo.o
+
+# 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
+# RUN: ln -sf Versions/Current/Foo %t/Foo.framework/Foo
+
+
+# RUN: %lld -lSystem -dyld_env DYLD_FRAMEWORK_PATH=%t/Foo.framework %t/main.o -o %t/main.out
+# RUN: llvm-otool -l %t/main.out | FileCheck %s -DVALUE=%t/Foo.framework
+
+# RUN: not %lld -lSystem -dyld_env DYLD_FRAMEWORK_PATH,%t/Foo %t/main.o -o /dev/null 2>&1 | FileCheck %s -DVALUE=%t/Foo --check-prefix=ERR 
+
+CHECK:     Load command 11
+CHECK-NEXT:          cmd LC_DYLD_ENVIRONMENT
+CHECK-NEXT:      cmdsize 128
+CHECK-NEXT:         name DYLD_FRAMEWORK_PATH=[[VALUE]] (offset 12)
+
+ERR: error: -dyld_env malformed value. Expected -dyld_env=DYLD_<name>_PATH=<path>, got DYLD_FRAMEWORK_PATH,[[VALUE]]
+
+
+#--- foo.s
+.globl _foo
+_foo:
+  retq
+
+#--- main.s
+.section __TEXT,__text
+
+.global _main
+_main:
+  ret

llvm/include/llvm/BinaryFormat/MachO.h

struct build_version_command {
   uint32_t cmdsize;  // sizeof(struct build_version_command) +
                      // ntools * sizeof(struct build_tool_version)
   uint32_t platform; // platform
   uint32_t minos;    // X.Y.Z is encoded in nibbles xxxx.yy.zz
   uint32_t sdk;      // X.Y.Z is encoded in nibbles xxxx.yy.zz
   uint32_t ntools;   // number of tool entries following this
 };
 
+struct dyld_env_command {
+  uint32_t cmd;
+  uint32_t cmdsize;
+  uint32_t name;
+};
+
 struct dyld_info_command {
   uint32_t cmd;
   uint32_t cmdsize;
   uint32_t rebase_off;
   uint32_t rebase_size;
   uint32_t bind_off;
   uint32_t bind_size;
   uint32_t weak_bind_off;