Re: [PATCH RFA] ubsan: default to trap on unreachable at -O0 and -Og [PR104642]

[Jason Merrill <jason@redhat.com>]

On 6/14/22 07:44, Jakub Jelinek wrote:
> On Mon, Jun 13, 2022 at 03:53:13PM -0400, Jason Merrill via Gcc-patches wrote:
>> When not optimizing, we can't do anything useful with unreachability in
>> terms of code performance, so we might as well improve debugging by turning
>> __builtin_unreachable into a trap.  In the PR richi suggested introducing an
>> -funreachable-traps flag for this, but this functionality is already
>> implemented as -fsanitize=unreachable -fsanitize-undefined-trap-on-error, we
>> just need to set those flags by default.
>>
>> I think it also makes sense to do this when we're explicitly optimizing for
>> the debugging experience.
>>
>> I then needed to make options-save handle -fsanitize and
>> -fsanitize-undefined-trap-on-error; since -fsanitize is has custom parsing,
>> that meant handling it explicitly in the awk scripts.  I also noticed we
>> weren't setting it in opts_set.
>>
>> Do we still want -funreachable-traps as an alias (or separate flag) for this
>> behavior that doesn't mention the sanitizer?
> 
> I do not like doing it this way, -fsanitize-undefined-trap-on-error is
> (unfortunately for compatibility with llvm misdesign, users should have
> ways to control which of the enabled sanitizers should be handled which way,
> where the 3 ways are runtime diagnostics without abort, runtime diagnostics
> with abort and __builtin_trap ()) an all or nothing option which affects all
> the sanitizers.

Makes sense.  The first is -fsanitize-recover=, I think, the second is 
the default, and perhaps the third could be -fsanitize-trap=?

> Your patch will e.g. silently enable the sanitization
> whenever just -fsanitize-undefined-trap-on-error is passed, but that can be
> passed even when users don't really expect any sanitization, just making
> sure that if it is enabled (say for selected TUs or functions), it doesn't
> need a runtime library to report it.

I'm happy to drop that part.

> 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 pass over the function to rewrite __builtin_unreachable, but that 
doesn't seem like much overhead.

> 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.

It seems to me that we want -funreachable-traps (or whatever spelling) 
to have exactly the same effect as the current 
-fsanitize=unreachable,return -fsanitize-undefined-trap-on-error, so 
using an entirely novel implementation strategy seems wrong.

OTOH, maybe the sanitizer should also hook into fold_builtin_0 to 
rewrite many of the calls before any of the optimizers run, so we don't 
need to explicitly check SANITIZE_UNREACHABLE in e.g. 
optimize_unreachable.  And I notice that we currently optimize away the 
call to f() in

bool b;
int f() {
   if (b) return 42;
   __builtin_unreachable ();
} 

int main() { f(); }

with -fsanitize=unreachable -O, so the test exits normally.

> 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.

Jason