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

[Vy Nguyen via Phabricator via llvm-commits]

oontvoo added inline comments.


================
Comment at: lld/MachO/InputFiles.cpp:635-636
   // Initialize symbols.
-  DylibFile *exportingFile = isImplicitlyLinked(dylibName) ? this : umbrella;
+  DylibFile *exportingFile =
+      (isBundleLoader || isImplicitlyLinked(dylibName)) ? this : umbrella;
   if (const load_command *cmd = findCommand(hdr, LC_DYLD_INFO_ONLY)) {
----------------
int3 wrote:
> I don't think this change is necessary, unless bundle loaders can somehow be sub-libraries
No, it's not technically needed. I just thought it should be made a bit clearer (Otherwise, it was calling isImplicitlyLinked() on an empty dylibName).  WDYT? 


================
Comment at: lld/MachO/Writer.cpp:485
     break;
   case MH_BUNDLE:
     break;
----------------
@int3  I think this needs to emit the `__mh_bundle_header` here too?

(not because of this patch, but it seems it's currently lacking that)


================
Comment at: lld/test/MachO/bundle_loader-darwin.test:29
+# Check bundle.bundle to ensure the `my_func` symbol is from executable
+# RUN: llvm-nm -m %t/bundle.bundle | grep 'my_func' | FileCheck %s --check-prefix BUNDLE_CHECK
+# BUNDLE_CHECK: (undefined) external my_func (from executable)
----------------
int3 wrote:
> oontvoo wrote:
> > int3 wrote:
> > > int3 wrote:
> > > > doesn't seem like the `grep` is necessary
> > > I would prefer we use `llvm-objdump --macho --bind` instead of `llvm-nm` here -- it should provide a more "raw" view of the file data since it's Mach-O-specific and isn't trying to shoehorn multiple file formats into one output format
> > llvm-objdump didn't seem to produce anything useful to assert on for me:
> > 
> > ```
> > llvm-objdump-9 --macho --bind /mnt/ssd/repo/llvm-project/build/tools/lld/test/MachO/Output/bundle_loader-darwin.test.tmp/bundle.bundle
> > /mnt/ssd/repo/llvm-project/build/tools/lld/test/MachO/Output/bundle_loader-darwin.test.tmp/bundle.bundle:
> > Bind table:
> > segment  section            address    type       addend dylib            symbol
> > __DATA_CONST __got              0x00002000 pointer         0 libSystem        dyld_stub_binder
> > ```
> > 
> > 
> Sorry, it should have been `--lazy-bind`, since `_main` is a function and all functions are lazily bound.
> 
> Anyway I tried it locally and ran into this error:
> 
> ```
> (base) test/MachO: llvm-objdump --macho --lazy-bind /Users/jezng/src/llvm-project/build/release/tools/lld/test/MachO/Output/bundle_loader-darwin.test.tmp/bundle2.bundle
> /Users/jezng/src/llvm-project/build/release/tools/lld/test/MachO/Output/bundle_loader-darwin.test.tmp/bundle2.bundle:
> Lazy bind table:
> segment  section            address     dylib            symbol
> llvm-objdump: error: '/Users/jezng/src/llvm-project/build/release/tools/lld/test/MachO/Output/bundle_loader-darwin.test.tmp/bundle2.bundle': truncated or malformed object (for BIND_OPCODE_SET_DYLIB_ORDINAL_ULEB bad library ordinal: 255 (max 1) for opcode at: 0x2)
> ```
> 
> I wasn't sure if this was a problem in our output or in `llvm-objdump`, so I tried replacing `%lld` in the test with `ld` to see what ld64 was emitting. But it appears that ld64 isn't even emitting any references to `_main` in `bundle2.bundle`:
> 
> ```
> (base) test/MachO: llvm-nm -m /Users/jezng/src/llvm-project/build/release/tools/lld/test/MachO/Output/bundle_loader-darwin.test.tmp/bundle2.bundle
>                  (undefined) external dyld_stub_binder (from libSystem)
> 0000000000003fb7 (__TEXT,__text) external my_func
> 0000000000003fb1 (__TEXT,__text) non-external my_user
> ```
> 
> I was testing this on Catalina 10.15.7 with ld64-609.7. Does the behavior differ on your system?
Ah, I think the error was because the EXECUTABLE_ORDINAL should not have been emitted. (really, the point of setting that EXECUTABLE_ORDINAL was for llvm-nm to know to emit "from executable" when it prints the symbol).

Anyways, I've fixed that and updated the tests to also use llvm-objdump.

About` _main`, yeah, maybe there needs to be a special case where `_main` doesn't get emitted?




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95913