[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 marked 2 inline comments as done.
modimo added a comment.

> 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 validate the E2E solution?



================
Comment at: lld/ELF/Driver.cpp:1098
+  for (auto &s : vtableSymbolsWithNoRTTI) {
+    warn("--lto-validate-all-vtables-have-type-infos: RTTI missing for vtable "
+         "_ZTV" +
----------------
tejohnson wrote:
> modimo wrote:
> > tejohnson wrote:
> > > modimo wrote:
> > > > tejohnson wrote:
> > > > > Prefer message() over warn() because the latter causes builds using -Werror to fail.
> > > > Is that the case for lld? Looking through the equivalent functionality is under `--fatal-warnings` and on a small example `-Werror` doesn't affect this flag/cause the build to fail.
> > > Sorry, you are correct. We use --fatal-warnings for this on linker actions (and -Werror on compile actions). In general, I think this should be an informational message, not a warning, since it is being handled automatically.
> > The case I want to catch is when an existing project changes its native dependencies and disables the optimization which would better fit a warning. In the general case agreed that this isn't a warning. I don't have much experience in linker protocol here, @MaskRay thoughts? 
> The problem is if it is a warning, then we have to go in and manually change options or builds will fail. Can you intercept message output, which should always be emitted (i.e. don't need verbose options).
Sure, changed to message.


================
Comment at: lld/test/ELF/lto/devirt_validate_vtable_typeinfos_no_rtti.ll:8
+
+; RUN: llvm-as %S/Inputs/devirt_validate_vtable_typeinfos.ll -o %t2.bc
+; RUN: llc -filetype=obj %t2.bc -o %t2.o
----------------
tejohnson wrote:
> The earlier version didn't have this second input file -  why is it needed now for this test? 
Good catch, I re-used the index/hybrid/full commands from `devirt_validate_vtable_typeinfos.ll` and that came along for the ride, removed.


================
Comment at: llvm/include/llvm/LTO/LTO.h:367
+    /// WPD optimizations and should be treated conservatively if requested.
+    bool VisibleToRegularObj = false;
+
----------------
tejohnson wrote:
> What's the downside in practice with using VisibleOutsideSummary?
It doesn't extend to Full LTO. However, the same functionality for full LTO is captured with `GlobalResolutions[name].Partition == GlobalResolution::External` so this doesn't need to be broken out separately.


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


================
Comment at: llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp:902
+      if (VisibleToRegularObjSymbols.count(P.first) &&
+          !GlobalValue::isLocalLinkage(GVar->linkage()))
+        continue;
----------------
tejohnson wrote:
> Hmm, now that I think about it, shouldn't local symbols have gotten VCallVisibilityTranslationUnit from clang? Same question in the regularLTO handling.
Good point. Originally excluding based on type name had to explicitly take into account local vcall_visibility. Now, it's a test setup where `VCallVisibilityTranslationUnit` was only used for one of the local types. I'll remove this check and the type that doesn't have the proper vcall_visibility in the tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155659