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
