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

[Teresa Johnson via Phabricator via cfe-commits]

tejohnson added a comment.

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

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

Ok what I missed is that you don't want to apply this to entire TUs, but rather just some paths that are header files, which may be included in many source files. So in your above example, you really only need to apply to the path of third-party/include/boost.h - is that correct? That would mark class A, and therefore anything derived from it won't get devirtualized. I guess in your example above, you are trying to prevent the devirtualization in a.cpp since there are hidden overrides (class C) in boost.a native objects.

The example included with the patch applies the option to the source file of the test case, and therefore its entire TU. It would be helpful to have a test case structured like your example above, where the file path is just that of the header.

Maybe a better option name is something like -funknown-vtable-visibility-filepaths= ? It seems a bit more descriptive.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152741