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

Vy Nguyen via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Feb 16 10:06:31 PST 2021


oontvoo added a comment.

In D95913#2563998 <https://reviews.llvm.org/D95913#2563998>, @int3 wrote:

> 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!

No worries. Thanks for taking the time to review this!

One more thing, some tests started to fail after the signed => unsigned types change ...  I must've missed something obvious ... thoughts?



================
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) {
----------------
int3 wrote:
> hmm doesn't BIND_OPCODE_SET_DYLIB_SPECIAL_IMM encode its argument as an immediate rather than in a trailing ULEB?
> hmm doesn't BIND_OPCODE_SET_DYLIB_SPECIAL_IMM encode its argument as an immediate rather than in a trailing ULEB?
Whoops, yes - it should be. (fixed now).


================
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
+
----------------
int3 wrote:
> 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.)
> 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.)

Yeah, it gets it from the symbol table[0] - Specifically, it looks at the `nlist_64::n_desc` field and prints `from executable` for ordinal-exec [1]

[0] https://github.com/llvm/llvm-project/blob/6c05005238a805a699d9dec39a61971affd1cab4/llvm/tools/llvm-nm/llvm-nm.cpp#L414
[1] https://github.com/llvm/llvm-project/blob/6c05005238a805a699d9dec39a61971affd1cab4/llvm/tools/llvm-nm/llvm-nm.cpp#L613



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