[PATCH] D95913: [lld-macho] Implement -bundle_loader

[James Y Knight via Phabricator via llvm-commits]

jyknight added inline comments.


================
Comment at: lld/MachO/Driver.cpp:328-329
+      newFile = *dylibFile;
+    else
+      error(Twine("Unable to parse bundle_loader target ") + path);
+
----------------
That an error message is emitted here but not in the above call to loadDylib confuses me. I don't know if that's because it's extraneous here or missing above, but it seems like one or the other must be wrong.


================
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);
----------------
oontvoo wrote:
> jyknight wrote:
> > Why is this check before opening needed?
> to fail early... should it not check that?
Usually failing on actual open is sufficient. Also, the call to addFile right below this doesn't do so -- so I'd expect that either that call is missing such a check, or this one is extraneous.


================
Comment at: lld/MachO/SyntheticSections.cpp:792
       n_desc |= dysym->isWeakRef() ? MachO::N_WEAK_REF : 0;
+      if (dysym->file->isBundleLoader)
+        MachO::SET_LIBRARY_ORDINAL(n_desc, MachO::EXECUTABLE_ORDINAL);
----------------
I'd expect to see the code that sets ->ordinal modified to set it to EXECUTABLE_ORDINAL, rather than the code that uses ->ordinal modified to override it.


================
Comment at: lld/test/MachO/bundle_loader-darwin.test:19-20
+
+# Produce the dylib
+# RUN: %lld  -dylib -install_name %t/my_lib.dylib -o %t/mylib.dylib %t/2.o
+
----------------
Is the dylib necessary in this test? It's weird to link the same "2.o" file into both a dylib and the main binary. 

And, assuming not, I think my_func definition can just be moved into main.s, eliminating 2.s.


================
Comment at: lld/test/MachO/bundle_loader-darwin.test:36
+my_func:           
+        pushq   %rbp
+        movq    %rsp, %rbp
----------------
There's no need for the test to be a correctly-executing function -- you can delete all the asm except the call (in order to have the needed symbol reference) or ret (just so there's at least an instruction in the function).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95913