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

[Jez Ng via Phabricator via llvm-commits]

int3 marked 6 inline comments as done.
int3 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) {
----------------
smeenai wrote:
> 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.
yup, this is definitely quadratic, but I figured input file lists + sub_library options would probably be relatively small


================
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) {
----------------
smeenai wrote:
> 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.
No plans to use it in other places yet; can inline for now


================
Comment at: lld/MachO/InputFiles.cpp:285
+
+  if (hdr->flags & MH_NO_REEXPORTED_DYLIBS)
+    return;
----------------
smeenai wrote:
> 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.)
yup! That's why I was always setting it prior to this diff. IIRC dyld would complain


================
Comment at: lld/MachO/InputFiles.cpp:312
+StringRef DylibFile::expandPath(StringRef path) {
+  if (path.consume_front("@executable_path/")) {
+    SmallString<64> executablePath{getName()};
----------------
smeenai wrote:
> 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...)
Ah, yeah I misunderstood what @executable_path was supposed to represent... 

> is it from the install name of sub-libraries getting copied over to the re-export load commands?

yup

> I'd be curious what ld64 does

I think it loads sub-libraries after parsing the entire command line. I guess our equivalent would be putting the executable path in the Config object, though I'm a bit wary of bloating Config...

Might be easier to just have the test use absolute paths for now and punt on implementing this till later


================
Comment at: lld/MachO/Options.td:26
 
+def sub_library: Separate<["-"], "sub_library">, MetaVarName<"<libname>">,
+  HelpText<"Re-export the specified dylib">;
----------------
smeenai wrote:
> What about `-reexport-lx` and `-reexport_library foo`? Those seem to be the more modern options.
I just wanted to implement enough sub-library creation functionality to test our ability to read them. I think we can punt full support for sub-library creation till later


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79228