[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:
> `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.


================
Comment at: lld/test/ELF/lto/devirt_vcall_vis_export_dynamic.ll:2
 ; REQUIRES: x86
-; Test that --lto-whole-program-visibility enables devirtualization.
+; Test that --export-dynamic[-symbol] and --dynamic-list prevents devirtualization.
 
----------------
MaskRay wrote:
> Nit: in LLD tests we use `;; ` to differentiate regular comments from `CHECK` `RUN` markers.
Can I do this in a follow on NFC commit? Otherwise it will make the diffs really noisy in this test.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91583