[PATCH] D95913: Implement -bundle_loader

[James Y Knight via Phabricator via llvm-commits]

jyknight added a comment.

Please prefix the title of the commit with "[lld-macho]" so that it's easier to understand what bundle_loader is being implemented in.

And, I'm not an expert on this codebase, so while I have some comments, I'm also adding a code-owner of lld macho to the review.



================
Comment at: lld/MachO/Driver.cpp:801
+        error("-bundle_loader can only be used with MachO bundle output");
+      if (fs::exists(arg->getValue()))
+        addFile(arg->getValue(), false, true);
----------------
Why is this check before opening needed?


================
Comment at: lld/MachO/InputFiles.cpp:626
+  } else if (!isBundleLoader) {
+    // macho_executable and macho_bundle don't have LC_ID_DYLIB
     error("dylib " + toString(this) + " missing LC_ID_DYLIB load command");
----------------
As discussed via chat, you'll need to make sure that the proper "EXECUTABLE_ORDINAL" ordinal gets used, rather than emitting a LOAD_DYLIB with an empty dylib name.


================
Comment at: lld/test/MachO/bundle_loader-darwin.test:1
+REQUIRES: x86
+REQUIRES: darwin
----------------
The tests for LLD shouldn't invoke clang; this will need to use assembly, and invoke llvm-mc and lld, instead.  Also, for this sort of multi-input test, the "split-file" util will be better than a bunch of "echo" commands.

See e.g. lld/test/MachO/archive.s for examples of how you might structure it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95913