[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 a comment.

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!



================
Comment at: llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp:885
+    const DenseSet<GlobalValue::GUID> &DynamicExportSymbols,
+    const DenseSet<GlobalValue::GUID> &VisibleToRegularObjSymbols) {
   if (!hasWholeProgramVisibility(WholeProgramVisibilityEnabledInLTO))
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155659