[PATCH] D95913: [lld-macho] Implement -bundle_loader
Jez Ng via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Feb 15 11:44:36 PST 2021
int3 added a comment.
Okay, this seems like it's almost there. Thanks for putting up with all the nitty comments and explaining the details, I know it's taken a while :) Once the remaining comments are addressed I'll stamp this. Thank you!
================
Comment at: lld/MachO/SyntheticSections.cpp:271
+ os << static_cast<uint8_t>(BIND_OPCODE_SET_DYLIB_SPECIAL_IMM);
+ encodeULEB128(dysym->getFile()->ordinal, os);
+ } else if (dysym->getFile()->ordinal <= BIND_IMMEDIATE_MASK) {
----------------
hmm doesn't BIND_OPCODE_SET_DYLIB_SPECIAL_IMM encode its argument as an immediate rather than in a trailing ULEB?
================
Comment at: lld/MachO/Writer.cpp:504
+ if (dylibFile->isBundleLoader)
+ dylibFile->ordinal = llvm::MachO::BIND_SPECIAL_DYLIB_MAIN_EXECUTABLE;
+
----------------
oontvoo wrote:
> int3 wrote:
> > hm why did this change from `EXECUTABLE_ORDINAL` to `BIND_SPECIAL_DYLIB_MAIN_EXECUTABLE`?
> >
> > Also, shouldn't there be a `continue;` here since I assume we don't want to emit an `LC_LOAD_DYLIB` for this? (Can you add a test that would catch this issue?)
> >
> > also, no need for `llvm::`
> > hm why did this change from `EXECUTABLE_ORDINAL` to `BIND_SPECIAL_DYLIB_MAIN_EXECUTABLE`?
>
> These two are kind of the same thing (The former being 0xff and the latter -1). `BIND_SPECIAL_DYLIB_MAIN_EXECUTABLE` is the right thing to use when encoding because it's consistent with others `BIND_*` things.
> Also (from looking at what ld64 does), it's more convenient to set `BIND_SPECIAL_DYLIB_MAIN_EXECUTABLE` here so that you can have the `encodeDylibOrdinal` function do the right thing for the special cases where ordinal <=0. ( 0 is for self, and -1 is for dylib-main-exec)
>
>
> > Also, shouldn't there be a `continue;` here since I assume we don't want to emit an `LC_LOAD_DYLIB` for this? (Can you add a test that would catch this issue?)
>
> No, I think it doesn't "reexport" the symbols but it still needs the load_dylib for the symbols that are referenced.
> (otherwise, we're missing those. And the test is that when you do `llvm-objdump`, you'd get an error for "truncated or malformed object")
>
> > also, no need for `llvm::`
>
> Done
>
>
> These two are kind of the same thing (The former being 0xff and the latter -1).
Ah, I see... the encoded ordinal value isn't actually a signed integer, but an unsigned byte with a few special values.
> No, I think it doesn't "reexport" the symbols but it still needs the load_dylib for the symbols that are referenced.
Got it, thanks!
================
Comment at: lld/test/MachO/bundle_loader-darwin.test:17
+# RUN: llvm-objdump --macho --lazy-bind %t/bundle.bundle | FileCheck %s --check-prefix BUNDLE_OBJ_CHECK
+# BUNDLE_OBJ_CHECK: __DATA __la_symbol_ptr 0x00001010 my_fun
+
----------------
oontvoo wrote:
> int3 wrote:
> > Since the address is not relevant to the test, let's not match on it (to make future changes to this file easier)
> >
> > I was also confused for a bit about how this was testing for EXECUTABLE_ORDINAL. Then I realized that the lack of dylib name in the output indicates that it's not from a dylib. I think it would be clearer if you included the segment/section/address etc header before this line.
> > I was also confused for a bit about how this was testing for EXECUTABLE_ORDINAL.
> It's a bit clearer to use `llvm-nm` to test for this. (because it'd say "from executable")
>
> > Then I realized that the lack of dylib name in the output indicates that it's not from a dylib.
> Right.
>
> > I think it would be clearer if you included the segment/section/address etc header before this line.
> done
>
>
>
> It's a bit clearer to use llvm-nm to test for this. (because it'd say "from executable")
Fair. Though I'm actually not sure if `llvm-nm` is getting that information from the bind table or from the symbol table. (I think it's actually the symbol table.)
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