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

[Jez Ng via Phabricator via llvm-commits]

int3 accepted this revision.
int3 added inline comments.
This revision is now accepted and ready to land.


================
Comment at: lld/test/MachO/bundle-loader.s:1
+REQUIRES: x86
+
----------------
This currently works because `split-file` is being used, which means the uncommented line doesn't get passed to `llvm-mc`, but we should still comment it out for uniformity


================
Comment at: lld/test/MachO/bundle-loader.s:13
+# RUN: llvm-nm -m %t/bundle.bundle |  FileCheck %s --check-prefix BUNDLE_CHECK
+# BUNDLE_CHECK (undefined) external _main (from executable)
+# BUNDLE_CHECK: (undefined) external my_func (from executable)
----------------
missing colon + consider not adding the `_CHECK` suffix which is kinda redundant. Also I would suggest using hyphens instead of underscores as separators, since that's what llvm-lit uses for its native prefixes (e.g. `CHECK-NEXT`).


================
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:
> > 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
> 
Great, we're testing both outputs then :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95913