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

[Jez Ng via Phabricator via llvm-commits]

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