[PATCH] D95913: [lld-macho] Implement -bundle_loader
Vy Nguyen via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Sat Feb 20 19:45:35 PST 2021
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
More information about the llvm-commits
mailing list