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


================
Comment at: lld/ELF/Driver.cpp:1098
+  for (auto &s : vtableSymbolsWithNoRTTI) {
+    warn("--lto-validate-all-vtables-have-type-infos: RTTI missing for vtable "
+         "_ZTV" +
----------------
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).


================
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
----------------
The earlier version didn't have this second input file -  why is it needed now for this test? 


================
Comment at: llvm/include/llvm/LTO/Config.h:85
+  bool ValidateAllVtablesHaveTypeInfos = false;
+  /// If all native vtables have corresponding type infos allowing
+  /// usage of RTTI to block devirtualization on types used in native files.
----------------
This would read better like "If all native vtables have corresponding type infos, allow usage..."


================
Comment at: llvm/include/llvm/LTO/LTO.h:367
+    /// WPD optimizations and should be treated conservatively if requested.
+    bool VisibleToRegularObj = false;
+
----------------
What's the downside in practice with using VisibleOutsideSummary?


================
Comment at: llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp:801
+      if (IsVisibleToRegularObj(TypeID->getString())) {
+        skip = true;
+        break;
----------------
Just early return true here, and return false below the loop.


================
Comment at: llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp:885
+    const DenseSet<GlobalValue::GUID> &DynamicExportSymbols,
+    const DenseSet<GlobalValue::GUID> &VisibleToRegularObjSymbols) {
   if (!hasWholeProgramVisibility(WholeProgramVisibilityEnabledInLTO))
----------------
Suggest naming this VisibleToRegularObjVTables.


================
Comment at: llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp:902
+      if (VisibleToRegularObjSymbols.count(P.first) &&
+          !GlobalValue::isLocalLinkage(GVar->linkage()))
+        continue;
----------------
Hmm, now that I think about it, shouldn't local symbols have gotten VCallVisibilityTranslationUnit from clang? Same question in the regularLTO handling.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155659