Re: [PATCH] gimple, internal-fn: Add IFN_TRAP and use it for __builtin_unreachable [PR106099]

[Richard Biener <rguenther@suse.de>]

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.