On 6/16/22 09:14, Jakub Jelinek wrote:
> On Wed, Jun 15, 2022 at 04:38:49PM -0400, Jason Merrill wrote:
>>> Furthermore, handling it the UBSan way means we slow down the compiler
>>> (enable a bunch of extra passes, like sanopt, ubsan), which is undesirable
>>> e.g. for -O0 compilation speed.
>>
>> The ubsan pass is not enabled for unreachable|return.  sanopt does a single
> 
> You're right.
> 
>> pass over the function to rewrite __builtin_unreachable, but that doesn't
>> seem like much overhead.
> 
> But I think we are trying to avoid hard any kind of unnecessary whole IL
> extra walks, especially for -O0.

OK.

>>> So, I think -funreachable-traps should be a separate flag and not an alias,
>>> enabled by default for -O0 and -Og, which would be handled elsewhere
>>> (I'd say e.g. in fold_builtin_0 and perhaps gimple_fold_builtin too to
>>> avoid allocating trees unnecessary)
>>
>> I tried this approach, but it misses some __builtin_unreachable calls added
>> by e.g. execute_fixup_cfg; it seems they never get folded by any subsequent
>> pass.
> 
> We could also expand BUILT_IN_UNREACHABLE as BUILT_IN_TRAP during expansion
> to catch whatever isn't caught by folding.

That was an early thing I tried, but that's too late to prevent it from 
being used for optimization.  More recently I've put an assert in 
expand_builtin_unreachable to catch ones that slip past.

>>> and would be done if
>>> flag_unreachable_traps && !sanitize_flag_p (SANITIZE_UNREACHABLE),
>>> just replacing that __builtin_unreachable call with __builtin_trap.
>>> For the function ends in fact under those conditions we could emit
>>> __builtin_trap right away instead of emitting __builtin_unreachable
>>> and waiting on folding it later to __builtin_trap.
>>
>> Sure, but I generally prefer to change fewer places.
> 
> I'd say this would be very small change and the fastest + most reliable.
> Simply replace all builtin_decl_implicit (BUILT_IN_UNREACHABLE) calls
> with builtin_decl_unreachable () (12 of them) and define
> tree
> builtin_decl_unreachable ()
> {
>    enum built_in_function fncode = BUILT_IN_UNREACHABLE;
> 
>    if (sanitize_flag_p (SANITIZE_UNREACHABLE))
>      {
>        if (flag_sanitize_undefined_trap_on_error)
> 	fncode = BUILT_IN_TRAP;
>        /* Otherwise we want __builtin_unreachable () later folded into
> 	 __ubsan_handle_builtin_unreachable with extra args.  */
>      }
>    else if (flag_unreachable_traps)
>      fncode = BUILT_IN_TRAP;
>    return builtin_decl_implicit (fncode);
> }
> and that's it (well, also in build_common_builtin_nodes
> declare __builtin_trap for FEs that don't do that - like it is done
> for __builtin_unreachable).

OK, here's another version of the patch using that approach.