[PATCH] D152741: [WPD] implement -fskip-vtable-filepaths

[Teresa Johnson via Phabricator via cfe-commits]

tejohnson added a comment.

In D152741#4419366 <https://reviews.llvm.org/D152741#4419366>, @modimo wrote:

> In D152741#4419324 <https://reviews.llvm.org/D152741#4419324>, @tejohnson wrote:
>
>> In D152741#4419265 <https://reviews.llvm.org/D152741#4419265>, @modimo wrote:
>>
>>> In D152741#4418831 <https://reviews.llvm.org/D152741#4418831>, @tejohnson wrote:
>>>
>>>> I think I understand the motivation, but not sure I agree this is the right approach - can you simply not pass -flto-unit and -fwhole-program-vtables for these files?
>>>
>>> For our third-party libraries, they're pre-built into native files by GCC so that's unfortunately not an option.
>>
>> I'm confused - how would you pass this new option then? I was assuming you were passing this option to some LLVM built files at the interface of those libraries. In which case not passing -flto-unit and -fwhole-program-visibility should have a similar effect (suppress the type metadata).
>
> Oh I see, I misunderstood. Yes this is being passed to LLVM built files. We want to avoid manual allowlists/blocklists because code changes make it less flexible and scalable than an automatic option.

It seems like you need allowlists or blocklists in either case - either it is passed as a regex via the option proposed here, or the build system modifies the options for that set of files.

> This can also be pretty tricky to do correctly since we can get type metadata from multiple TUs and all of them would need to be opted out for WPD to not kick in.

But clang is presumably compiling a single TU at a time, so your regex needs to cover them all anyway? I'm not sure I understand the distinction between doing something like -fskip-vtable-filepaths=third-party/.* vs something like applying -funknown-vtable-visibility to each third-party/*.cc compile.

I really think the logic for which files to apply this option to belongs in the build system, not in the clang driver - just like any other clang option. It isn't clear to me why this particular option should be applied based on a file regex.

I like the approach of communicating this via vcall visibility, but just think that it should be applied to the current TU whenever a TBD new option is provided.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152741