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

[Di Mo via Phabricator via cfe-commits]

modimo added a comment.

In D152741#4421067 <https://reviews.llvm.org/D152741#4421067>, @tejohnson wrote:

> 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.

The blocklists need to be enforced on internal files that interact with native libraries and those live in many different areas:

  ; /third-party/include/boost.h
  
  class A {}
  
  ; /internal-source/a.cpp
  #include "boost.h"
  
  class B : public A
  
  ; /third-party/lib/boost.a
  #include "boost.h"
  
  class C : public A

That being said, this is something the build system can detect and mark.

> 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.

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152741