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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

Generally looks good.



================
Comment at: lld/ELF/LTO.cpp:252
+    r.VisibleToDynamicLinker =
+        sym->isExportDynamic(sym->kind(), sym->visibility) ||
+        sym->inDynamicList;
----------------
`sym->exportDynamic || sym->inDynamicList`

Then `isExportDynamic` does not need to be public.


================
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.
 
----------------
Nit: in LLD tests we use `;; ` to differentiate regular comments from `CHECK` `RUN` markers.


================
Comment at: lld/test/ELF/lto/devirt_vcall_vis_export_dynamic.ll:43
+; RUN:   -mllvm -pass-remarks=. \
+; RUN: 	 --export-dynamic 2>&1 | FileCheck %s --implicit-check-not single-impl --allow-empty
 ; RUN: llvm-dis %t.o.4.opt.bc -o - | FileCheck %s --check-prefix=CHECK-NODEVIRT-IR
----------------
`%s` -> `/dev/null`


================
Comment at: lld/test/ELF/lto/devirt_vcall_vis_public.ll:8
 ; RUN: ld.lld %t2.o -o %t3 -save-temps --lto-whole-program-visibility \
-; RUN: 	 -mllvm -pass-remarks=. --export-dynamic 2>&1 | FileCheck %s --check-prefix=REMARK
+; RUN: 	 -mllvm -pass-remarks=. \
+; RUN:	 2>&1 | FileCheck %s --check-prefix=REMARK
----------------
`s/\t/ /`


================
Comment at: lld/test/ELF/lto/devirt_vcall_vis_public.ll:9
+; RUN: 	 -mllvm -pass-remarks=. \
+; RUN:	 2>&1 | FileCheck %s --check-prefix=REMARK
 ; RUN: llvm-dis %t2.o.4.opt.bc -o - | FileCheck %s --check-prefix=CHECK-IR
----------------
`s/\t/ /`

The two lines can be joined.


================
Comment at: llvm/include/llvm/LTO/LTO.h:466
+  /// by a shared library not visible to the linker.
+  unsigned VisibleToDynamicLinker : 1;
+
----------------
How about VisibleToOtherModules?

The name VisibleToDynamicLinker is too tied to the ELF binary format.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91583