[PATCH] D152741: [WPD] implement -funknown-vtable-visibility-filepaths

[Di Mo via Phabricator via cfe-commits]

modimo marked 4 inline comments as done.
modimo added a comment.

In D152741#4445112 <https://reviews.llvm.org/D152741#4445112>, @wenlei wrote:

>> The big advantage of doing this in the FE is that we know which types are actually coming from the native headers. Blocking all types in the TU is overly conservative and also less stable as header changes can effectively turn on/off unrelated large chunks of WPD.
>
> This is clearly going to be selective in punting unsafe devirt, however do you have data comparing the effectiveness of the two (module granularity vs header granularity)?

Some data would help quantify the difference, I'll hack up a module granularity implementation and compare to the current one.

> I also think introducing unknown visibility is a good idea for this to work, but this is going to be exposed to users (not hidden implementing only), so we would probably need to have spec/rule to handle conflicting visibility from different source and make those explicit here: https://clang.llvm.org/docs/LTOVisibility.html.

The ordering for conflicts is embedded in the logic for `CodeGenModule::GetVCallVisibilityLevel` which has priority order of:

1. Unknown
2. Public
3. LinkageUnit
4. TranslationUnit

I'll update the documentation to reflect this.

> There's a spectrum of solutions we could use to make WPD safer, but we need to be careful not to make this whole thing too convoluted with multiple solutions implemented, but little differentiation in their incremental value (extra perf, extra safety). So having concrete data backing the incremental value of this solution would be helpful.

For concrete data are you talking about between the different solutions e.g. --lto-whole-program-visibility, -funknown-vtable-visibility-filepaths, RTTI based, FatLTO based etc or something else?



================
Comment at: clang/include/clang/Basic/CodeGenOptions.h:191
+  struct RegexWithPattern {
+    std::string Pattern;
+    std::shared_ptr<llvm::Regex> Regex;
----------------
wenlei wrote:
> Pattern string doesn't seem to be used anywhere, can we simplify this using `llvm::Regex` instead of `RegexWithPattern`?
It's used here in CompilerInvocation.cpp:
```
  if (Opts.SkipVtableFilepaths.hasValidPattern())
    GenerateArg(Args, OPT_funknown_vtable_visibility_filepaths_EQ,
                Opts.SkipVtableFilepaths.Pattern, SA);
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152741