[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;
----------------
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.


================
Comment at: llvm/include/llvm/LTO/LTO.h:466
+  /// by a shared library not visible to the linker.
+  unsigned VisibleToDynamicLinker : 1;
+
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91583