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

Fri Feb 5 16:13:45 PST 2021

int3 added inline comments.

================
Comment at: lld/MachO/Driver.cpp:323
+  case file_magic::macho_bundle:
+    // We only allow executable/bundle type here if it is used
+    // as bundle_loader.
----------------
nit

================
Comment at: lld/MachO/Driver.cpp:324
+    // We only allow executable/bundle type here if it is used
+    // as bundle_loader.
+    if (!isBundleLoader)
----------------
ultra nit: here you refer to it as `bundle_loader`, in Writer.cpp you refer to it as `bundle-loader`. Personally I think it reads more naturally with a space in between, but anything is fine as long as it's consistent :P 

================
Comment at: lld/MachO/Driver.cpp:325-326
+    // as bundle_loader.
+    if (!isBundleLoader)
+      error(path + ": unhandled file type");
+    if (Optional<DylibFile *> dylibFile =
----------------
this should be an `assert()` -- `error()s` are things that can arise from user input

================
Comment at: lld/MachO/Driver.cpp:797
     switch (opt.getID()) {
+    case OPT_bundle_loader:
+      if (config->outputType != MH_BUNDLE)
----------------
`OPT_bundle_loader` should be handled using `getLastArg` (like the other options in lines 735-773 above) instead of in this loop. This loop should be reserved for options whose values can be specified multiple times.

================
Comment at: lld/MachO/Driver.cpp:798-799
+    case OPT_bundle_loader:
+      if (config->outputType != MH_BUNDLE)
+        error("-bundle_loader can only be used with MachO bundle output");
+      addFile(arg->getValue(), false, true);
----------------
should test this

================
Comment at: lld/MachO/InputFiles.cpp:628
+    // so it's OK.
+    ordinal = llvm::MachO::EXECUTABLE_ORDINAL;
   } else {
----------------
No need for `llvm::`. Also this should probably live in `Writer.cpp` (see comment there)

================
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)) {
----------------
I don't think this change is necessary, unless bundle loaders can somehow be sub-libraries

================
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) {
----------------
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.

================
Comment at: lld/MachO/InputFiles.h:128-129
   // (through an -lfoo flag), then `umbrella` should be a nullptr.
-  explicit DylibFile(MemoryBufferRef mb, DylibFile *umbrella = nullptr);
+  explicit DylibFile(MemoryBufferRef mb, DylibFile *umbrella = nullptr,
+                     bool isBundleLoader = false);

----------------
would be good to `assert(!isBundleLoader || !umbrella)`. Or just create a `makeBundleLoader` function that doesn't take an `umbrella`.

================
Comment at: lld/MachO/InputFiles.h:144
   bool forceWeakImport = false;
+  bool isBundleLoader;
 };
----------------
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.)

================
Comment at: lld/MachO/SyntheticSections.cpp:269-272
+    if (dysym->file->isBundleLoader) {
+      os << static_cast<uint8_t>(BIND_OPCODE_SET_DYLIB_ORDINAL_ULEB);
+      encodeULEB128(EXECUTABLE_ORDINAL, os);
+    } else if (dysym->file->ordinal <= BIND_IMMEDIATE_MASK) {
----------------
since EXECUTABLE_ORDINAL is larger than BIND_IMMEDIATE_MASK, this seems unnecessary

================
Comment at: lld/MachO/Writer.cpp:506-507
+      // The bundle-loader doesn't reexport the symbols.
+      if (dylibFile->isBundleLoader)
+        continue;
       LoadCommandType lcType =
----------------
I think we should assign `ordinal = EXECUTABLE_ORDINAL` here instead of in InputFiles.cpp. It's kind of weird to have `ordinal` set to its final value for bundle loaders at the parsing phase while all other `ordinal` values get set at the writing phase.

================
Comment at: lld/test/MachO/bundle_loader-darwin.test:8-17
+# Smoke check to ensure the object files are produced correctly.
+# RUN: llvm-nm -p %t/2.o | FileCheck %s --check-prefix TWO_CHECK
+# TWO_CHECK: 0000000000000000 T my_func
+
+# RUN: llvm-nm -p %t/3.o | FileCheck %s --check-prefix THREE_CHECK
+# THREE_CHECK: t my_user
+# THREE_CHECK: U my_func
----------------
ah I guess you were following `archive.s`, but most of our other tests don't do these smoke checks, I think they make things unnecessarily verbose. We're not testing `llvm-mc` here, we can just assume that the object files are correct

================
Comment at: lld/test/MachO/bundle_loader-darwin.test:19
+
+# Produce the dylib
+# RUN: %lld  -dylib -install_name %t/my_lib.dylib -o %t/mylib.dylib %t/2.o
----------------
the three "Produce" comments in this file aren't really necessary -- it's easy enough to see what the commands are doing

================
Comment at: lld/test/MachO/bundle_loader-darwin.test:26
+# Produce bundle.bunlde which uses `main` as bundle_loader
+# RUN: %lld -lSystem -demangle -dynamic -bundle -bundle_loader %t/main -o %t/bundle.bundle %t/3.o %t/mylib.dylib
+
----------------
`-dynamic` isn't necessary (it's currently a no-op in LLD, and even if it weren't, it's implied by `-bundle`)

I don't think `-demangle` is necessary either

================
Comment at: lld/test/MachO/bundle_loader-darwin.test:26
+# Produce bundle.bunlde which uses `main` as bundle_loader
+# RUN: %lld -lSystem -demangle -dynamic -bundle -bundle_loader %t/main -o %t/bundle.bundle %t/3.o %t/mylib.dylib
+
----------------
int3 wrote:
> `-dynamic` isn't necessary (it's currently a no-op in LLD, and even if it weren't, it's implied by `-bundle`)
> 
> I don't think `-demangle` is necessary either
Also, I guess if we created bundle.bundle from `3.o` and `2.o` (instead of mylib.dylib) then `my_func` would come from `2.o`, right? Could we have a test for that too?

================
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)
----------------
doesn't seem like the `grep` is necessary

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

================
Comment at: lld/test/MachO/bundle_loader-darwin.test:36
+my_func:           
+        retq
+
----------------
nit: most test files use two-space indents

================
Comment at: lld/test/MachO/bundle_loader-darwin.test:42
+.text
+my_user:                            # @my_user()
+        callq   my_func()
----------------

================
Comment at: lld/test/MachO/bundle_loader-darwin.test:52
+.text
+ _main:                                   # @main
+        retq
----------------

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95913