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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
