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

[Jez Ng via Phabricator via llvm-commits]

int3 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 =
----------------
oontvoo wrote:
> 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)
> 
> 
aha I see.


================
Comment at: lld/MachO/InputFiles.h:145
+
+  // If true, this file is used as bundle-loader.
+  // When building a bundle, users can use `--bundle_loader <exe>` to
----------------
this line is unnecessary (that much is clear from the variable name)


================
Comment at: lld/MachO/InputFiles.h:144
   bool forceWeakImport = false;
+  bool isBundleLoader;
 };
----------------
int3 wrote:
> this needs a detailed comment. I think it's pretty surprising that we're making DylibFiles out of bundles and executables, which are clearly not dylibs, but I get why -- they serve the same role here as binary interfaces whose contents do not appear in the final output. Still, surprising behavior should always be explained. (Would also be good to preface it with some explanation of what a bundle loader is, i.e. the stuff you have written in the commit message.)
Thanks for adding the comment. However, while it explains what a bundle loader is, it does not talk about why we are creating a DylibFile from things that are not dylibs, which was the main point of my comment :)


================
Comment at: lld/test/MachO/bundle_loader-darwin.test:1
+REQUIRES: x86
+
----------------
to follow the other tests

also can you name this test `bundle-loader.s`? I.e. don't use underscores, no need to mention `darwin` (all the tests here are for darwin), and since the test inputs are assembly, the extension should match.


================
Comment at: lld/test/MachO/bundle_loader-darwin.test:14
+# 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)
----------------
1) use hyphen to follow other tests
2) missing colon


================
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)
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95913