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

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added inline comments.


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


================
Comment at: llvm/include/llvm/LTO/LTO.h:466
+  /// by a shared library not visible to the linker.
+  unsigned VisibleToDynamicLinker : 1;
+
----------------
MaskRay wrote:
> tejohnson wrote:
> > MaskRay wrote:
> > > MaskRay wrote:
> > > > tejohnson wrote:
> > > > > MaskRay wrote:
> > > > > > How about VisibleToOtherModules?
> > > > > > 
> > > > > > The name VisibleToDynamicLinker is too tied to the ELF binary format.
> > > > > VisibleToOtherModules sounds to me like it means LLVM Modules that are being linked together statically. I wanted to note that these are symbols that may have dynamic references not seen by the static link. Is there anything like --export-dynamic for other binary formats? If not, then it is ELF specific anyway.
> > > > @rnk for thoughts on COFF.
> > > Perhaps another name is `Exported`.
> > > 
> > > For ELF, the does not seem to be restricted to shared objects seen as in the input file.
> > "Exported" is ambiguous - we use that throughout ThinLTO to mean exported from the current module (to other modules being LTO linked).
> > 
> > > For ELF, the does not seem to be restricted to shared objects seen as in the input file.
> > 
> > Sorry I don't follow?
> Seems that the idea is just whether the symbol is exported and can be used by other linked images. A dynamic linker is the ELF concept but the idea can be used by other binary formats. In COFF it is called "export table". In Mach-O there are linker options `-exported_*`, and non-exported symbols are converted to private externs.
> 
> ThinLTO has already used `exported` to mean symbols exchanged among LLVM modules so `export` should not be used. Perhaps just `exportDynamic`? The name stills stems from ELF but users from other binary formats can still find similarity.
Sounds good, will change to simply ExportDynamic.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91583