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

Sat Aug 12 09:03:15 PDT 2023

modimo added a comment.

Headed out on PTO and will be back on the 24th, will pick this back up then!

================
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
----------------
tejohnson wrote:
> 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.
I see, so the `BASE` scenario being tested here is already guarded by EnableSplitLTOUnit since ThinLTO would have EnableSplitLTOUnit=0 and RegularLTO would have EnableSplitLTOUnit=1. Is this the scenario described in the previous comment?

>RegularLTO summaries are added to the combined index used by ThinLTO, but it looks like the vtable summaries aren't currently created for them. I think you are right in that there is a potential hole here for ThinLTO WPD if linked with a regular LTO object containing an override. Can you test this case to confirm? If that is an issue, then I guess we do need another GlobalRes field. Maybe VisibleOutsideLTOUnit or something like that?

The mixed case then would be RegularLTO combined with ThinLTO + -split-lto-unit where neither generate `typeidCompatibleVTable` and all the analysis is done on the combined RegularLTO module.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155659