[PATCH] D155659: [WPD][LLD] Add option to validate RTTI is enabled on all native types and prevent devirtualization on types with native RTTI

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

Will study...



================
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:
> > > > 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.
This long list here makes me nervous. Will try to learn it.


================
Comment at: lld/ELF/Options.td:601
   "turn off warnings about profile cfg mismatch">;
+defm lto_known_safe_vtables : Eq<"lto-known-safe-vtables", "When --lto-validate-all-vtables-have-type-infos is enabled, skip validation on these vtables">;
 def lto_obj_path_eq: JJ<"lto-obj-path=">;
----------------
New options use `EEq` to disallow single-dash long options, to not conflict with `-l`.


================
Comment at: lld/test/ELF/lto/Inputs/devirt_validate_vtable_typeinfos.ll:4
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-grtev4-linux-gnu"
+
----------------
`grtev4` => `unknown`


================
Comment at: llvm/tools/opt/opt.cpp:575
+      *M,
+      /* WholeProgramVisibilityEnabledInLTO */ false,
+      /* DynamicExportSymbols */ {},
----------------
The prevailing and recommended style liked by clang-format and clang-tidy is

`/*WholeProgramVisibilityEnabledInLTO=*/false`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155659