[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;
+
----------------
oontvoo wrote:
> 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).
P.S: updated test was:

```
llvm-objdump  --macho --lazy-bind %t/bundle.bundle | FileCheck %s --check-prefix BUNDLE-OBJ
segment   section                    address        dylib                          symbol
 __DATA    __la_symbol_ptr     0x{{[0-9a-f]*}}    main-executable       my_fun
```

Will sleep on this reland it on Monday, just in case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95913