[PATCH] D91583: [LTO] Prevent devirtualization for symbols exported to dynamic linker

Wed Jan 27 11:42:04 PST 2021

MaskRay added inline comments.

================
Comment at: lld/ELF/LTO.cpp:252
+    r.VisibleToDynamicLinker =
+        sym->isExportDynamic(sym->kind(), sym->visibility) ||
+        sym->inDynamicList;
----------------
tejohnson wrote:
> MaskRay wrote:
> > tejohnson wrote:
> > > MaskRay wrote:
> > > > tejohnson wrote:
> > > > > MaskRay wrote:
> > > > > > MaskRay wrote:
> > > > > > > tejohnson wrote:
> > > > > > > > tejohnson wrote:
> > > > > > > > > MaskRay wrote:
> > > > > > > > > > tejohnson wrote:
> > > > > > > > > > > MaskRay wrote:
> > > > > > > > > > > > `sym->exportDynamic || sym->inDynamicList`
> > > > > > > > > > > > 
> > > > > > > > > > > > Then `isExportDynamic` does not need to be public.
> > > > > > > > > > > sym->exportDynamic is false for linkonce_odr vtables, that was what I was referencing in this comment (otherwise I could use includeInDynsym which checks that):
> > > > > > > > > > > 
> > > > > > > > > > > > Note that I couldn't use includeInDynsym in lld because that is not set for linkonce_odr symbols that were thus have canBeOmittedFromSymbolTable set (since any referencing module must have it's own copy) - we still want to block the LTO visibility upgrade for those symbols to avoid WPD. So I am using a slightly different interface that more directly checks whether export-dynamic is in effect.
> > > > > > > > > > Sorry, I don't understand the difference. If I replace this with `sym->exportDynamic`, I don't get a test failure...
> > > > > > > > > Ah, this is a test deficiency, looks like I need to make one or more of the vtables linkonce_odr to expose it. Will address that. The reason it is an issue for linkonce_odr can be seen in createBitcodeSymbol in lld/ELF/InputFiles.cpp, where it does:
> > > > > > > > > 
> > > > > > > > >     if (canOmitFromDynSym)
> > > > > > > > >       newSym.exportDynamic = false;
> > > > > > > > > 
> > > > > > > > > The canOmitFromDynSym gets propagated via the input file but is originally set in GlobalValue::canBeOmittedFromSymbolTable() for linkonce_odr with hasAtLeastLocalUnnamedAddr().
> > > > > > > > I've improved the tests. Confirmed that the improved lld test fails if you make the change you proposed.
> > > > > > > I see that `sym->isExportDynamic` is used to prevent `canOmitFromDynSym` (unnamed_addr linkonce_odr or local_unnamed_addr linkonce_odr constant) logic.
> > > > > > > 
> > > > > > > There is one case where `sym->isExportDynamic(sym->kind(), sym->visibility)` may be false while `sym->exportDynamic` is true: a shared object with a STV_DEFAULT reference to the symbol can set `exportDynamic` (`InputFiles.cpp:1557`).
> > > > > > > 
> > > > > > > `sym->isExportDynamic(...) || sym->exportDynamic` should be safe.
> > > > > > The comprehensive rule for when `exportDynamic` is set:
> > > > > > 
> > > > > > ```
> > > > > > * non-local `STV_DEFAULT/STV_PROTECTED` (this means it can be hid by `--exclude-libs`)
> > > > > > * logical OR of the following:
> > > > > >   + undefined
> > > > > >   + (`--export-dynamic` || `-shared`) && ! (unnamed_addr linkonce_odr GlobalVariable || local_unnamed_addr linkonce_odr constant GlobalVariable)
> > > > > >   + matched by `--dynamic-list/--export-dynamic-symbol-list/--export-dynamic-symbol`
> > > > > >   + defined or referenced by a shared object as `STV_DEFAULT`
> > > > > >   + `STV_PROTECTED` definition in a shared object preempted by copy relocation/canonical PLT when `--ignore-{data,function}-address-equality}` is specified
> > > > > >   + `-z ifunc-noplt` && has at least one relocation
> > > > > > ```
> > > > > > 
> > > > > > The last two are edge cases (but works if you use `sym->exportDynamic`).
> > > > > > 
> > > > > > About the common case "defined or referenced by a shared object as `STV_DEFAULT`":
> > > > > > for `ld.lld %t.o %t1.so -o %t`, if `%t1.so` defines (linkonce_odr) the vtable, it can be preempted by the executable definition. `%t` thus needs to export the vtable.
> > > > > > 
> > > > > > This may deserve a test (`sym->isExportDynamic(...) || sym->exportDynamic` and ``sym->isExportDynamic(...)` have different behaviors)
> > > > > I see, so essentially sym->isExportDynamic(...) and sym->exportDynamic are non-overlapping and neither is a superset of the either. I will change the code to check them both. In terms of creating a test, can any of the cases where the latter is true but the former is not be triggered for a vtable? Since this is only being used for WPD right now, I'd need to be able to test this with a vtable def visible to a virtual call that would otherwise be devirtualized.
> > > > ```
> > > > llc -filetype=obj -relocation-model=pic %t.ll -o %t.o
> > > > lld -shared %t.o -o %t.so
> > > > ```
> > > > 
> > > > The output has a dynamic symbol `_ZTV1B`.
> > > sym->isExportDynamic(...) already returns true if config->shared, so in the library link it should already be handled by the current patch, or am I misunderstanding?
> > ```
> > llc -filetype=obj -relocation-model=pic %t.ll -o %t.o
> > ld.lld -shared %t.o -o %t.so
> > ld.lld %t.o %t.so -o %t
> > ```
> > 
> > The ELF semantic is that %t.o preempts every default visibility definition in `%t.so`. So, even in the absence of `--export-dynamic`/`--dynamic-list`/`--export-dynamic-symbol`, the definitions in %t need to be exported (to .dynsym) to allow preemption at runtime.
> > 
> > This is a case where `sym->isExportDynamic(...)` is false while `sym->exportDynamic` is true.
> > 
> > (`sym->isExportDynamic(...)` is true while `sym->exportDynamic` is false should be impossible, so no need for a test.)
> > ```
> > llc -filetype=obj -relocation-model=pic %t.ll -o %t.o
> > ld.lld -shared %t.o -o %t.so
> > ld.lld %t.o %t.so -o %t
> > ```
> > 
> > The ELF semantic is that %t.o preempts every default visibility definition in `%t.so`. So, even in the absence of `--export-dynamic`/`--dynamic-list`/`--export-dynamic-symbol`, the definitions in %t need to be exported (to .dynsym) to allow preemption at runtime.
> > 
> > This is a case where `sym->isExportDynamic(...)` is false while `sym->exportDynamic` is true.
> 
> Ok let me create a test for this.
> 
> > 
> > (`sym->isExportDynamic(...)` is true while `sym->exportDynamic` is false should be impossible, so no need for a test.)
> 
> It can happen which is why I added the check for sym->isExportDynamic(...) and not sym->exportDynamic in the first place. The current test case (with the linkonce_odr vtables) is exactly this case, because of canOmitFromDynSym (this was what I was describing upthread).
> 
Ah yes, I forgot again that `sym->exportDynamic` can be set to false due to canOmitFromDynSym (ThinLTO auto hiding) logic.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91583