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

[Vy Nguyen via Phabricator via llvm-commits]

oontvoo added inline comments.


================
Comment at: lld/MachO/Writer.cpp:504
+      if (dylibFile->isBundleLoader)
+        dylibFile->ordinal = llvm::MachO::BIND_SPECIAL_DYLIB_MAIN_EXECUTABLE;
+
----------------
int3 wrote:
> oontvoo wrote:
> > 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
> > 
> > 
> > These two are kind of the same thing (The former being 0xff and the latter -1).
> 
> Ah, I see... the encoded ordinal value isn't actually a signed integer, but an unsigned byte with a few special values.
> 
> > No, I think it doesn't "reexport" the symbols but it still needs the load_dylib for the symbols that are referenced.
> 
> Got it, thanks!
Actually, I think you were right about this. The bundle-loader also didn't need this extra load command. (I've jsut retried the test case with ld64, and it skipped this.)
This was also why we hit the ubsan one (the path name is going to be empty for executable).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95913