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

[Jez Ng via Phabricator via llvm-commits]

int3 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)) {
----------------
oontvoo wrote:
> 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? 
I think it's confusing either way...

I'm actually wondering if it we should have separate DylibFile and BundleLoader classes that inherit from some common base class. Not asking you to do it in this diff though. But for now I would prefer we just remove the `isBundleLoader` check here


================
Comment at: lld/MachO/Writer.cpp:504
+      if (dylibFile->isBundleLoader)
+        dylibFile->ordinal = llvm::MachO::BIND_SPECIAL_DYLIB_MAIN_EXECUTABLE;
+
----------------
hm why did this change from `EXECUTABLE_ORDINAL` to `BIND_SPECIAL_DYLIB_MAIN_EXECUTABLE`?

Also, shouldn't there be a `continue;` here since I assume we don't want to emit an `LC_LOAD_DYLIB` for this? (Can you add a test that would catch this issue?)

also, no need for `llvm::`


================
Comment at: lld/MachO/Writer.cpp:485
     break;
   case MH_BUNDLE:
     break;
----------------
oontvoo wrote:
> @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)
yup I'm aware it's a missing feature


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95913