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

Thanks for taking a look!

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

> Thanks for the patch. I have some initial comments below. From my reading, I guess any native object with a vtable and no RTTI will disable WPD globally, which is unfortunate.

It makes sense from a correctness point of view but yeah it is a stringent requirement. The linker warning does keep this from being silent although how much churn this causes remains to be seen.



================
Comment at: llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp:2413
+  // case bail on the optimization.
+  if (ValidateAllVtablesHaveTypeInfos && !AllVtablesHaveTypeInfos)
+    return;
----------------
tejohnson wrote:
> Rather than doing this down here in LTO/WPD could the linker simply unset the HasWholeProgramVisibility config flag? That would also allow WPD to proceed on types with hidden LTO visibility. This early return would prevent any and all WPD which seems overly conservative in the case of hidden LTO visibility classes. 
That makes sense although it does tie this flag's functionality to requiring `--lto-whole-program-visibility`. 

Doing that though means we can instead pass the blocklist to `updateVCallVisibilityInIndex`/`updateVCallVisibilityInModule` similarly to how D91583 does it for dynamically exported symbols which would be cleaner. Thoughts on that approach?


================
Comment at: llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp:2485
+        // Local linkage is always safe
+        // Note that if RTTI is not enabled in LTO unit, the typename
+        // symbol doesn't exist and we don't have a resolution meaning
----------------
tejohnson wrote:
> Can we ever get here if RTTI is not enabled? My understanding of the change to line 2413 is that we return early in that case. Given that early return, aren't we guaranteed that the typename symbol has a GlobalResolution if it is non-local?
> 
> Oh - I guess we are only early returning if RTTI is off in native objects, so you could get here if RTTI is only disabled in bitcode objects? And we need to be conservative for any typenames for vtables defined in bitcode objects with RTTI off? I didn't see a test for this case, can you add one (or did I miss it)?
>Oh - I guess we are only early returning if RTTI is off in native objects, so you could get here if RTTI is only disabled in bitcode objects?

Yep!

>And we need to be conservative for any typenames for vtables defined in bitcode objects with RTTI off?

This is primarily an implementation detail with how resolutions are only provided for IR symbols. If we instead pass more information from the linker (like resolutions for these summary symbols or the whole list of typenames) we can support bitcode files with RTTI off. There's not a correctness issue at play here since the summary information is a superset of RTTI.

> I didn't see a test for this case, can you add one (or did I miss it)?

Good catch, will add a test case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155659