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



================
Comment at: lld/ELF/Driver.cpp:1038
+  for (InputFile *f : ctx.objectFiles) {
+    for (Symbol *sym : f->getSymbols()) {
+      processVtableAndTypeInfoSymbols(sym->getName());
----------------
Does this get both defs and refs? I think the latter as I am seeing a case where we are linking a bitcode object that contains both the vtable and typename defs but are disabling --lto-whole-program-visibility, and the reason seems to be a reference to the vtable from a native object.


================
Comment at: lld/ELF/Driver.cpp:1098
+  for (auto &s : vtableSymbolsWithNoRTTI) {
+    warn("--lto-validate-all-vtables-have-type-infos: RTTI missing for vtable "
+         "_ZTV" +
----------------
Prefer message() over warn() because the latter causes builds using -Werror to fail.


================
Comment at: llvm/lib/LTO/LTO.cpp:1707
   updateVCallVisibilityInIndex(ThinLTO.CombinedIndex,
-                               Conf.HasWholeProgramVisibility,
+                               WholeProgramVisibilityEnabledInLTO,
                                DynamicExportSymbols);
----------------
Can you do the same for updateVCallVisibilityInModule to get this fix to apply to regular LTO?


================
Comment at: llvm/lib/LTO/LTO.cpp:1710
 
+  auto typeInfoVisibleOutsideSummary = [&](StringRef name) {
+    return GlobalResolutions.count(name)
----------------
nit: lambda name should be upper camel case.

Also, can you add a comment here that this will return true for either the case where name is a local or where it is not defined, and so the expectation is that it will not be queried for local symbols.


================
Comment at: llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp:2413
+  // case bail on the optimization.
+  if (ValidateAllVtablesHaveTypeInfos && !AllVtablesHaveTypeInfos)
+    return;
----------------
modimo wrote:
> modimo wrote:
> > tejohnson wrote:
> > > modimo wrote:
> > > > 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?
> > > > That makes sense although it does tie this flag's functionality to requiring --lto-whole-program-visibility.
> > > 
> > > What would be the use case of the proposed handling without --lto-whole-program-visibility? Are you saying that there are cases where the normal LTO visibility is incorrect?
> > > 
> > > > 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?
> > > 
> > > Yep I think that would be cleaner.
> > > What would be the use case of the proposed handling without --lto-whole-program-visibility? Are you saying that there are cases where the normal LTO visibility is incorrect?
> > 
> > I don't have a known case so this is more theoretical. Currently there's an assertion that it's on the user to make sure LTO visibility is correct but in this case and in D91583 we can catch violations and prevent them from causing problems. How much this should also apply to normal LTO visibility is a question but thinking more about it is orthogonal to this change.
> > 
> > 
> Ah right, the issue with doing this in `updateVCallVisibilityInIndex`/`updateVCallVisibilityInModule` is that vcall visibility is keyed off the vtable symbol. However, TypeID and RTTI are both keyed off of the typename symbol. There's not always a translation from typename to vtable since abstract base classes wouldn't have vtables and by the time we get the association we're in the same place the logic is right now.
> 
> Given that, I think I'll keep the logic as-is.
> There's not always a translation from typename to vtable since abstract base classes wouldn't have vtables and by the time we get the association we're in the same place the logic is right now.

Can you clarify the case you are concerned about and how this is handled in the lld code that expects a translation from vtable to typename? I tried an example with an abstract base class and do get a vtable and typename.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155659