pcc added a comment. In https://reviews.llvm.org/D53524#1279288, @tejohnson wrote:
> In https://reviews.llvm.org/D53524#1276038, @pcc wrote: > > > In https://reviews.llvm.org/D53524#1274505, @tejohnson wrote: > > > > > In https://reviews.llvm.org/D53524#1271387, @tejohnson wrote: > > > > > > > In https://reviews.llvm.org/D53524#1271357, @pcc wrote: > > > > > > > > > The reason why LTO unit is always enabled is so that you can link > > > > > translation units compiled with `-fsanitize=cfi` and/or > > > > > `-fwhole-program-vtables` against translation units compiled without > > > > > CFI/WPD. With this change we will see miscompiles in the translation > > > > > units compiled with CFI/WPD if they use vtables in the translation > > > > > units compiled without CFI/WPD. If we really need this option I think > > > > > it should be an opt out. > > > > > > > > > > > > Is there an important use case for support thing mixing and matching? > > > > The issue is that it comes at a cost to all ThinLTO compiles for codes > > > > with vtables by requiring them all to process IR during the thin link. > > > > > > > > > Ping on the question of why this mode needs to be default. If it was a > > > matter of a few percent overhead that would be one thing, but we're > > > talking a *huge* overhead (as noted off-patch for my app I'm seeing >20x > > > thin link time currently, and with improvements to the hashing to always > > > get successful splitting we could potentially get down to closer to 2x - > > > still a big overhead). This kind of overhead should be opt-in. The > > > average ThinLTO user is not going to realize they are paying a big > > > overhead because CFI is always pre-enabled. > > > > > > Well, the intent was always that the overhead would be minimal, which is > > why things are set up the way that they are. But it doesn't sound like > > anyone is going to have the time to fully address the performance problems > > that you've seen any time soon, so maybe it would be fine to introduce the > > -flto-unit flag. I guess we can always change the flag so that it has no > > effect if/when the performance problem is addressed. > > > Just to clarify, since there is already a -flto-unit flag: it is currently a > cc1 flag, did you want it made into a driver option as well? Yes, that's what I had in mind. >>>> Can we detect that TUs compiled with -flto-unit are being mixed with those >>>> not built without -flto-unit at the thin link time and issue an error? >>> >>> This would be doable pretty easily. E.g. add a flag at the index level that >>> the module would have been split but wasn't. Users who get the error and >>> want to support always-enabled CFI could opt in via -flto-unit. >> >> Yes. I don't think we should make a change like this unless there is >> something like that in place, though. The documentation (LTOVisibility.rst) >> needs to be updated too. > > Ok, let me work on that now and we can get that in before this one. Repository: rC Clang https://reviews.llvm.org/D53524 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
