[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

1441

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

1445

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

1446

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

lld/MachO/Writer.cpp

421

s/rpath_command/dyld_env_command/ probably?

850

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

1440

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

3

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

1444

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

1440

Yes - added support for that. Thanks!

1444

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

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.

3

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.

[int3]

lgtm

lld/MachO/Driver.cpp

1441–1442

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)

1443

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

1445

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

lld/MachO/Options.td

1218–1219

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

850

(not done?)

[oontvoo]

addressed review comments

lld/MachO/Writer.cpp

850

Done! (for real)

[int3]

lld/MachO/Options.td

1218–1219

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);
   }
+  for (auto *arg : args.filtered(OPT_dyld_env)) {
+    StringRef envPair(arg->getValue());
+    if (!envPair.contains('='))
+      error("-dyld_env's argument is  malformed. Expected "
+            "-dyld_env <ENV_VAR>=<VALUE>, got `" +
+            envPair + "`");
+    config->dyldEnvs.push_back(envPair);
+  }
+  if (!config->dyldEnvs.empty() && config->outputType != MH_EXECUTE)
+    error("-dyld_env can only be used when creating executable output");
+
   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_dtrace_dof : Flag<["-"], "no_dtrace_dof">,
     HelpText<"Disable dtrace-dof processing (default).">,
     Group<grp_rare>;
 def objc_stubs_fast : Flag<["-"], "objc_stubs_fast">,
     HelpText<"Produce larger stubs for Objective-C method calls with fewer jumps (default).">,
     Group<grp_rare>;
 def objc_stubs_small : Flag<["-"], "objc_stubs_small">,
     HelpText<"Produce smaller stubs for Objective-C method calls with more jumps.">,
     Group<grp_rare>;
+def dyld_env : Separate<["-"], "dyld_env">,
+    MetaVarName<"<dyld_env_var>">,
+    HelpText<"Specifies a LC_DYLD_ENVIRONMENT variable value pair.">,
+    Group<grp_rare>;
 
 def grp_deprecated : OptionGroup<"deprecated">, HelpText<"DEPRECATED">;
 
 def lazy_framework : Separate<["-"], "lazy_framework">,
     MetaVarName<"<name>">,
     HelpText<"This option is deprecated and is now an alias for -framework.">,
     Flags<[HelpHidden]>,
     Group<grp_deprecated>;

 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 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<dyld_env_command *>(buf);
+    buf += sizeof(dyld_env_command);
+
+    c->cmd = LC_DYLD_ENVIRONMENT;
+    c->cmdsize = getSize();
+    c->name = 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));
   }
 
+  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
+
+# RUN: rm -rf %t && mkdir %t
+
+# RUN: llvm-mc -filetype obj -triple x86_64-apple-darwin %s -o %t/main.o
+
+# RUN: %lld -lSystem -dyld_env DYLD_FRAMEWORK_PATH=./Foo.framework %t/main.o -o %t/one_dyld_env.out
+# RUN: llvm-otool -l %t/one_dyld_env.out | FileCheck %s --check-prefix=ONE-ENV
+
+# RUN: %lld -lSystem -dyld_env DYLD_FRAMEWORK_PATH=./Foo.framework \
+# RUN:               -dyld_env DYLD_FRAMEWORK_PATH=./Bar.framework \
+# RUN:               %t/main.o -o %t/two_dyld_envs.out
+# RUN: llvm-otool -l %t/two_dyld_envs.out | FileCheck %s --check-prefix=TWO-ENV
+
+# RUN: not %lld -lSystem -dyld_env DYLD_FRAMEWORK_PATH,./Foo %t/main.o -o /dev/null 2>&1 | FileCheck %s --check-prefix=MALFORMED
+
+# RUN: not %lld  -dylib -lSystem -dyld_env DYLD_FRAMEWORK_PATH=./Foo %t/main.o -o /dev/null 2>&1 | FileCheck %s --check-prefix=DYLIB
+
+# ONE-ENV:     Load command 11
+# ONE-ENV-NEXT:          cmd LC_DYLD_ENVIRONMENT
+# ONE-ENV-NEXT:      cmdsize 48
+# ONE-ENV-NEXT:         name DYLD_FRAMEWORK_PATH=./Foo.framework (offset 12)
+
+# TWO-ENV:      Load command 11
+# TWO-ENV-NEXT:          cmd LC_DYLD_ENVIRONMENT
+# TWO-ENV-NEXT:      cmdsize 48
+# TWO-ENV-NEXT:         name DYLD_FRAMEWORK_PATH=./Foo.framework (offset 12)
+# TWO-ENV-NEXT: Load command 12
+# TWO-ENV-NEXT:          cmd LC_DYLD_ENVIRONMENT
+# TWO-ENV-NEXT:      cmdsize 48
+# TWO-ENV-NEXT:         name DYLD_FRAMEWORK_PATH=./Bar.framework (offset 12)
+
+# MALFORMED: error: -dyld_env's argument is malformed. Expected -dyld_env <ENV_VAR>=<VALUE>, got `DYLD_FRAMEWORK_PATH,./Foo`
+
+# DYLIB: error: -dyld_env can only be used when creating executable output
+
+.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;