[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 added inline comments.


================
Comment at: lld/test/ELF/lto/devirt_validate_vtable_typeinfos_mixed_lto.ll:117
+  ;; Check that the call was devirtualized.
+  ; CHECK-IR: %call2 = tail call i32 @_ZN1A1fEi
+  ;; Summary analysis does not devirtualize this, but RegularLTO's
----------------
Both this and the CHECK-SUMMARY-IR case below are incorrect devirtualizations, right? Is this another case that we are not doing correctly without the validation options in this patch?


================
Comment at: lld/test/ELF/lto/devirt_validate_vtable_typeinfos_mixed_lto.ll:123
+  ; CHECK-VALIDATE-IR: %call2 = tail call i32 %fptr22
+  ;; With everything present in summary, identified as not singleimpl
+  ; CHECK-VALIDATE-SUMMARY-IR: %call2 = tail call i32 %fptr22
----------------
I think we only get the vtable summary from the regular LTO object because it doesn't have the EnableSplitLTOUnit module flag set in the IR here. Normally, this is added by clang when building -flto. And this currently prevents vtable summaries being added to the LTO summary (https://github.com/llvm/llvm-project/blob/8a15bdb5e637f81041591d97bea0267b5f053f16/llvm/lib/Analysis/ModuleSummaryAnalysis.cpp#L734-L736).

When I added that guard, it was because I didn't think we needed these summaries when splitting was enabled, as I was thinking of either the everything-is-regular LTO case or the -fsplit-lto-unit case that you get by default with -flto=thin -fwhole-program-vtables, where all the vtables are placed in the regular LTO split modules. It's possible that we could remove that guard, but with it I think this case would do the wrong thing if the regular IR was built from clang with -flto.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155659