[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:
> 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
Done. That makes sense.


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

These two are kind of the same thing (The former being 0xff and the latter -1). `BIND_SPECIAL_DYLIB_MAIN_EXECUTABLE` is the right thing to use when encoding because it's consistent with others `BIND_*` things.
Also (from looking at what ld64 does), it's more convenient to set `BIND_SPECIAL_DYLIB_MAIN_EXECUTABLE` here so that you can have the `encodeDylibOrdinal` function do the right thing for the special cases where ordinal <=0.  ( 0 is for self, and -1 is for dylib-main-exec)


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

No,  I think it doesn't "reexport" the symbols but it still needs the load_dylib for the symbols that are referenced.
(otherwise, we're missing those. And the test is that when you do `llvm-objdump`, you'd get an error for "truncated or malformed object")

> also, no need for `llvm::`

Done




================
Comment at: lld/test/MachO/bundle_loader-darwin.test:17
+# RUN: llvm-objdump  --macho --lazy-bind %t/bundle.bundle | FileCheck %s --check-prefix BUNDLE_OBJ_CHECK
+# BUNDLE_OBJ_CHECK: __DATA   __la_symbol_ptr    0x00001010     my_fun
+
----------------
int3 wrote:
> Since the address is not relevant to the test, let's not match on it (to make future changes to this file easier)
> 
> I was also confused for a bit about how this was testing for EXECUTABLE_ORDINAL. Then I realized that the lack of dylib name in the output indicates that it's not from a dylib. I think it would be clearer if you included the segment/section/address etc header before this line.
> I was also confused for a bit about how this was testing for EXECUTABLE_ORDINAL. 
It's a bit clearer to use `llvm-nm` to test for this. (because it'd say "from executable")

> Then I realized that the lack of dylib name in the output indicates that it's not from a dylib. 
Right.

> I think it would be clearer if you included the segment/section/address etc header before this line.
done





Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95913