On Wed, 27 Jul 2022, Jakub Jelinek wrote: > On Wed, Jul 27, 2022 at 10:09:34AM +0000, Richard Biener wrote: > > > We chose to sanitize not just explicit user __builtin_unreachable calls, > > > but also the internally generated __builtin_unreachable calls (with the > > > one exception of fall through to end of C++ function returning non-void, > > > which had before a separate sanitizer) and we've been doing it since 2013 > > > when ubsan was added. > > > Even for the internally generated unreachable calls like those from > > > devirtualization or other reasons like ivcanon/unrolling, having the > > > possibility to get some runtime diagnostics or trap can be useful over > > > just falling through to random following code. > > > > So at least for the unrolling use the intent is to have the > > unreachable () fully elided by later passes. Honza can correct me > > if I'm wrong. Using __builtin_trap from the start until sanopt > > may prevent some of that from happening, keeping dead conditions > > live, no? > > That is true. > I guess changing the sanopt gate back and building __builtin_unreachable > only in the ivcanon/unrolling case is possible too. > > Without or with this patch then, the advantage of the patch would be that > we wouldn't need to recompute vops if we replace unreachables with traps > during the sanopt pass.
Yes, as I said on that ground the patch is OK, but I think it doesn't really address the few PRs that popped up after the earlier change. Richard. > > > > > Previously we'd always emit __builtin_unreachable, then perhaps in some > > > cases could e.g. optimize it away (say if there is a guarding condition > > > around the implicitly added unreachable turning the condition into VRP > > > info and optimizing the conditional away), otherwise the sanopt pass > > > would turn those __builtin_unreachable calls into __builtin_trap. > > > With the recent changes, we don't run the sanopt pass when only > > > doing -fsanitize=unreachable (or -funrechable-traps) though, so we need > > > to emit the trap/__ubsan_handle_unreachable/__builtin_unreachable right > > > away. > > > > Why did the recent changes not just replace __builtin_unreachable > > at RTL expansion time? Was the intent really to force the paths > > to be kept live? I can see that for user or frontend generated > > unreachables but not so much for some of the middle-end ones. > > It is easier on GIMPLE and perhaps allows e.g. sharing the data for > __ubsan_handle_unreachable calls. sanopt pass is quite late anyway.
