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