[PATCH] D95913: [lld-macho] Implement -bundle_loader
James Y Knight via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri Feb 5 11:47:25 PST 2021
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
More information about the llvm-commits
mailing list