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

[Vy Nguyen via Phabricator via llvm-commits]

oontvoo added inline comments.


================
Comment at: lld/MachO/Driver.cpp:328-329
+      newFile = *dylibFile;
+    else
+      error(Twine("Unable to parse bundle_loader target ") + path);
+
----------------
jyknight wrote:
> 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.
I've removed it to be consistent with the others... 


================
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);
----------------
jyknight wrote:
> 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.
Lookin at the code, I think DylibFile::ordinal means a different thing than "ordinal" in EXECUTABLE_ORDINAL.
So we can't set DylibFile::ordinal, which is a uint64, to EXECUTABLE_ORDINAL (which is 0xff).


Also,  the line right above, which sets the WEAK_REF flag also didn't set it earlier. 


================
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
+
----------------
jyknight wrote:
> 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.
Yes, I think the point of the test to show that even though both (the dylib and the executable) have the symbol, the output bundle gets the symbol from  the executable


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95913