[PATCH] D79228: [lld-macho] Add support for creating and reading reexported dylibs

[Shoaib Meenai via Phabricator via llvm-commits]

smeenai added inline comments.


================
Comment at: lld/MachO/Driver.cpp:120
+// with a path of .*/libfoo.dylib.
+static bool markSubLibrary(StringRef searchName) {
+  for (InputFile *file : inputFiles) {
----------------
This seems pretty inefficient in a bunch of ways in that we're iterating over the entire input file list (only a small number of which will be dylibs) multiple times (once for each `-sub_library` option). I don't know how bad it'll end up being in practice though, and there's a bunch of straightforward ways to make it better if need be, so I guess it's okay for now.


================
Comment at: lld/MachO/Driver.cpp:178
+  // re-exported.
+  for (opt::Arg *arg : args) {
+    if (arg->getOption().getID() == OPT_sub_library) {
----------------
Nit: It's more conventional to write loops like this using args.filtered, e.g. https://github.com/llvm/llvm-project/blob/eb7d32e46fe184fdfcb52e0a25973e713047e305/llvm/tools/llvm-objcopy/CopyConfig.cpp#L647-L648


================
Comment at: lld/MachO/InputFiles.cpp:131
+template <class T, class F>
+static const load_command *forEachCommand(const mach_header_64 *hdr,
+                                          uint32_t type, F callback) {
----------------
Do you envision this being used in other places? If not, I'd just inline the loop in the one place it's being used right now; I know @ruiu hasn't been a fan of higher-order functions in the past.


================
Comment at: lld/MachO/InputFiles.cpp:285
+
+  if (hdr->flags & MH_NO_REEXPORTED_DYLIBS)
+    return;
----------------
Is it an error if a library has this flag set but has no re-exports? (I don't think it's terribly important to implement; just curious.)


================
Comment at: lld/MachO/InputFiles.cpp:312
+StringRef DylibFile::expandPath(StringRef path) {
+  if (path.consume_front("@executable_path/")) {
+    SmallString<64> executablePath{getName()};
----------------
Hmm. This isn't correct in that `@executable_path` is supposed to refer to the path of the exectuable that'll be loading this library (which we have no way of knowing right now). `@loader_path` would be the path of the library that's triggering the load.

Where are you seeing `@executable_path` be used for re-exports? Hmm, is it from the install name of sub-libraries getting copied over to the re-export load commands? Not sure what the best way to handle that is ... I'd be curious what ld64 does. One idea that springs to mind is to see if the current dylib's install name also begins with `@executable_path/`, and if so, compute the path of the other dylib relative to the current dylib. (As in, if your current dylib's install name is `@executable_path/libfoo.dylib` and the one you're re-exporting is `@executable_path/../lib/libbar.dylib`, it follows that the re-exported dylib should live in `../lib` relative to the current dylib. On the other hand, if you flipped that around, the current dylib would have no way of knowing which subdirectory of its parent the re-exported dylib is supposed to live in...)


================
Comment at: lld/MachO/Options.td:26
 
+def sub_library: Separate<["-"], "sub_library">, MetaVarName<"<libname>">,
+  HelpText<"Re-export the specified dylib">;
----------------
What about `-reexport-lx` and `-reexport_library foo`? Those seem to be the more modern options.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79228