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

[Vy Nguyen via Phabricator via llvm-commits]

oontvoo added inline comments.


================
Comment at: lld/MachO/Driver.cpp:325-326
+    // as bundle_loader.
+    if (!isBundleLoader)
+      error(path + ": unhandled file type");
+    if (Optional<DylibFile *> dylibFile =
----------------
int3 wrote:
> this should be an `assert()` -- `error()s` are things that can arise from user input
This was because some tests was expecting this error upon seeing executable file being passed here. (It was a correct error, except for the case where the executable being the bundle-loader)




================
Comment at: lld/MachO/InputFiles.cpp:669-672
+DylibFile::DylibFile(const InterfaceFile &interface, DylibFile *umbrella,
+                     bool isBundleLoader)
+    : InputFile(DylibKind, interface), refState(RefState::Unreferenced),
+      isBundleLoader(isBundleLoader) {
----------------
int3 wrote:
> it would be ideal if this TBD code path were tested as well. We have some example TBD files e.g. at `test/MachO/Inputs/MacOSX.sdk/usr/lib/libSystem.tbd`. But I'm fine with having this added later so long as you leave a TODO.
Added a FIXME for this case. 


================
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:
> 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
```




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95913