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

[Di Mo via Phabricator via llvm-commits]

modimo added a comment.

In D155659#4555162 <https://reviews.llvm.org/D155659#4555162>, @tejohnson wrote:

> In D155659#4548653 <https://reviews.llvm.org/D155659#4548653>, @modimo wrote:
>
>>> In D155659#4533387 <https://reviews.llvm.org/D155659#4533387>, @tejohnson wrote:
>>>
>>>> In D155659#4529471 <https://reviews.llvm.org/D155659#4529471>, @tejohnson wrote:
>>>>
>>>>> I'm curious to give this a try internally on a few codes and see how frequently it ends up disabling WPD.
>>>>
>>>> I cranked through a bunch of builds with this change and thankfully while they all do have at least one vtable from an -fno-rtti native object, there are only a handful of unique symbols (which all appear safe), so we could consider using --lto-known-safe-vtables to allowlist them. I did find a couple that seem spurious (see comment inline below about this).
>>
>> For clarification, were the builds only to validate the linker check? If so, are there plans to try out the E2E solution?
>
> I was just looking for the linker check messages, I didn't enable WPD. With this solution in place we can hopefully try it out internally, but it might not be immediate. However, I'm keen to have this solution available so we can move forward on WPD!

Thanks for the clarification! Definitely very interested in your results internally when this finalizes.



================
Comment at: llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp:885
+    const DenseSet<GlobalValue::GUID> &DynamicExportSymbols,
+    const DenseSet<GlobalValue::GUID> &VisibleToRegularObjSymbols) {
   if (!hasWholeProgramVisibility(WholeProgramVisibilityEnabledInLTO))
----------------
tejohnson wrote:
> modimo wrote:
> > tejohnson wrote:
> > > Suggest naming this VisibleToRegularObjVTables.
> > Another area that's unknown for thinLTO are the types used in full LTO where `VisibleOutsideSummary` is set to false and vice-versa with full LTO where types used in thinLTO get `GlobalResolution::External` partition. I'm thinking then to categorize vtables we want to not upgrade as something like `RefOutsideWPD` instead of `VisibleToRegularObj`. WDYT?
> 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?
Added `devirt_validate_vtable_typeinfos_mixed_lto.ll` to test mixing LTO modes:
1. RegularLTO without summary indeed does not export vtable summaries. With validation, because the type is present in a ThinLTO module the partition is set to `GlobalResolution::External` this type does not get its visibility upgraded in RegularLTO and with `VisibleOutsideSummary` also set to true this type does not get its visibility upgraded in ThinLTO either.
2. RegularLTO with summary we do get all the vtable summaries in the combined index. With validation, `GlobalResolution::External` blocks RegularLTO visibility upgrade but since `VisibleOutsideSummary` is not set everything is optimized in the combined index.

For the purposes of validation I think this is the behavior we want. It does seem like we may want to fix this hole even without validation enabled although that seems more of a separate change since it alters baseline behavior.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155659