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

Tue Jun 13 16:08:56 PDT 2023

modimo added a comment.

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

>>> Also, isn't this hiding possibly necessary info from WPD that might be needed for correct class hierarchy analysis affecting other IR modules? I.e. in the type-metadata-skip-vtable-filepaths.cpp test, what if A was derived from a struct B, which was also defined/used in another module without this skipping option. We would lose information about the override of f in A, and possibly do an incorrect devirtualization elsewhere. It seems like a dangerous option to provide.
>>>
>>> It might be better to provide an option that can somehow mark vtables in a given module as unsafe for devirt, and propagate that info to WPD.
>>
>> That would nicely side-step mismatched flags. `Public` `vcall_visibility` describes this case but with `--lto-whole-program-visibility` there's no a distinction between `Public` because of deferred vs. `Public` because the type is known unsafe. Thoughts on an `unsafe` `vcall_visibility` to capture the latter notion?
>
> That would be better I think. Maybe "unknown".

Sounds good, I'll rework the patch.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152741