[PATCH] D155659: [WPD][LLD] Add option to validate RTTI is enabled on all native types and prevent devirtualization on types with native RTTI

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson accepted this revision.
tejohnson added a comment.
This revision is now accepted and ready to land.

lgtm with a couple comments/suggestions below. Thanks!

In D155659#4618301 <https://reviews.llvm.org/D155659#4618301>, @modimo wrote:

> Looking at https://github.com/llvm/llvm-project/blob/9b6b6bb/clang/lib/CodeGen/BackendUtil.cpp#L170-L173, RegularLTO will have a summary attached through Clang by default except for ld64 targets. The mixed case then reduces to RegularLTO+Summary and ThinLTO+Split since `EnableSplitLTOUnit` must be consistent so everything is done by `DevirtModule::run` and there's no mixture with `DevirtIndex::run`. The mechanism for `IsVisibleToRegularObj` for RegularLTO also ends up being identical to that of ThinLTO since all symbols will be present in the combined summary.

Ok, yes - I think the scenario I was worried about is caught by the existing verification that EnableSplitLTOUnit is set consistently.



================
Comment at: lld/test/ELF/lto/devirt_validate_vtable_typeinfos.ll:6
+; RUN: opt --thinlto-bc --thinlto-split-lto-unit -o %t1_hybrid.o %s
+; RUN: cp %s %t1_regular.ll
+; RUN: echo '!llvm.module.flags = !{!12}' >> %t1_regular.ll
----------------
modimo wrote:
> Appending module flags so RegularLTO correctly generates it's summary without `typeidCompatibleVTable` means the test can be re-used. However I think duplicating the tests is reasonable as well and could be cleaner, WDYT?
Do we need these module flags for correct operation of this test (ditto for the similar no_rtti one later)? If not, then probably don't bother adding in these tests (I think these may only be needed in practice for the hybrid testing). If they are now needed for correct operation of the regular LTO testing, then I am ok with the approach here as I think it is probably better to reduce duplication of nearly identical IR tests (and I see this approach used in other tests too).


================
Comment at: llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp:876
+    if (IsVisibleToRegularObj(typeID.first)) {
+      for (const TypeIdOffsetVtableInfo &P : typeID.second) {
+        VisibleToRegularObjSymbols.insert(P.VTableVI.getGUID());
----------------
nit: the braces can be removed from the for loop


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155659