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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: lld/ELF/LTO.cpp:252
+    r.VisibleToDynamicLinker =
+        sym->isExportDynamic(sym->kind(), sym->visibility) ||
+        sym->inDynamicList;
----------------
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)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91583