| Message ID | 20on4s19-o945-4686-o662-6n4q48246q92@fhfr.qr |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC] Map -ftrapv to -fsanitize=signed-integer-overflow -fsanitize-undefined-trap-on-error | expand |
On 10/20/21 7:22 AM, Richard Biener via Gcc-patches wrote: > This maps -ftrapv to -fsanitize=signed-integer-overflow > -fsanitize-undefined-trap-on-error, effectively removing > flag_trapv (or rather making it always false). It sounds like C/C++ programmers might benefit from this change but users of the option in other languages would not. I'm sure they'd appreciate a heads up on the upcoming removal of a feature so they could adjust to it. Issuing a warning would be one way to give them such a heads up, while keeping the existing behavior for a release, and then removing it. > > This has implications on language support - while -ftrapv > was formerly universally available the mapping restricts it > to the C family of frontends. > > It also raises questions on mixing -ftrapv with -fsanitize > flags, specifically with other recovery options for the > undefined sanitizer since -fsanitize-undefined-trap-on-error > cannot be restricted to the signed-integer-overflow part at > the moment. To more closely map behavior we could add > -fsanitize=trapv where with a single option we could also > simply alias -ftrapv to that. > > Code quality wise a simple signed add compiles to > > movl %edi, %eax > addl %esi, %eax > jo .L5 > ... > .L5: > ud2 > > compared to > > call __addvsi3 > > and it has less of the bugs -ftrapv has. The IL will > not contain a PLUS_EXPR but a .UBSAN_CHECK_ADD internal > function call which has rudimentary support throughout > optimizers but is not recognized as possibly terminating > the program so > > int foo (int i, int j, int *p, int k) > { > int tem = i + j; > *p = 0; > if (k) > return tem; > return 0; > } > > will be optimized to perform the add only conditional > and the possibly NULL *p dereference first (note the > same happens with the "legacy" -ftrapv). The behavior > with -fnon-call-exceptions is also different as the > internal functions are marked as not throwing and > as seen above the actual kind of trap can change (SIGILL > vs. SIGABRT). > > One question is whether -ftrapv makes signed integer overflow > well-defined (to trap) Trapping isn't well-defined in the C/C++ sense of the word. It's still undefined behavior, even if it's documented that way. (Same way dereferencing a null pointer is undefined, even if it results in SIGBUS.) Martin like -fwrapv makes it wrap. If so > the the above behavior is ill-formed. Not sure how > sanitizers position themselves with respect to this and > whether the current behavior is OK there. The patch below > instruments signed integer ops but leaves them undefined > so the compiler still has to be careful as to not introduce > new signed overflow (but at least that won't trap). > Currently -fwrapv -fsanitize=signed-integer-overflow will > not instrument any signed operations for example. > > I do consider the option to simply make -ftrapv do nothing > but warn that people should use UBSAN - that wouldn't > imply semantics are 1:1 the same (which they are not). > > Bootstrapped and tested on x86_64-unknown-linux-gnu, regresses > > FAIL: gcc.dg/vect/trapv-vect-reduc-4.c scan-tree-dump-times vect "Detected > reduc > tion\\\\." 3 > FAIL: gcc.dg/vect/trapv-vect-reduc-4.c scan-tree-dump-times vect "using an > in-or > der \\\\(fold-left\\\\) reduction" 1 > FAIL: gcc.dg/vect/trapv-vect-reduc-4.c scan-tree-dump-times vect > "vectorized 3 l > oops" 1 > > where the vectorizer doesn't know the UBSAN IFNs. > > 2021-10-20 Richard Biener <rguenther@suse.de> > > * opts.c (common_handle_option): Handle -ftrapv like > -fsanitize=signed-integer-overflow > -fsanitize-undefined-trap-on-error and do not set > flag_trapv. > --- > gcc/opts.c | 16 +++++++++++++++- > 1 file changed, 15 insertions(+), 1 deletion(-) > > diff --git a/gcc/opts.c b/gcc/opts.c > index 65fe192a198..909d2a031ff 100644 > --- a/gcc/opts.c > +++ b/gcc/opts.c > @@ -3022,7 +3022,21 @@ common_handle_option (struct gcc_options *opts, > > case OPT_ftrapv: > if (value) > - opts->x_flag_wrapv = 0; > + { > + opts->x_flag_wrapv = 0; > + opts->x_flag_sanitize > + = parse_sanitizer_options ("signed-integer-overflow", > + loc, code, opts->x_flag_sanitize, > + value, false); > + if (!opts_set->x_flag_sanitize_undefined_trap_on_error) > + opts->x_flag_sanitize_undefined_trap_on_error = 1; > + /* This keeps overflow undefined and not trap. Specifically > + it does no longer allow to catch exceptions together with > + -fnon-call-exceptions. It also makes -ftrapv cease to > + work with non-C-family languages since ubsan only works for > + those. */ > + opts->x_flag_trapv = 0; > + } > break; > > case OPT_fstrict_overflow: >
On Wed, Oct 20, 2021 at 03:22:10PM +0200, Richard Biener via Gcc-patches wrote: > This maps -ftrapv to -fsanitize=signed-integer-overflow > -fsanitize-undefined-trap-on-error, effectively removing > flag_trapv (or rather making it always false). > > This has implications on language support - while -ftrapv > was formerly universally available the mapping restricts it > to the C family of frontends. > > It also raises questions on mixing -ftrapv with -fsanitize > flags, specifically with other recovery options for the > undefined sanitizer since -fsanitize-undefined-trap-on-error > cannot be restricted to the signed-integer-overflow part at > the moment. To more closely map behavior we could add > -fsanitize=trapv where with a single option we could also > simply alias -ftrapv to that. I think we shouldn't do it this way. There is no reason not to support it in all FEs, not just C family, the instrumentation is done during in this case in the ubsan pass anyway. And it also should cope well with different sanitizers, while -ftrapv vs. -fsanitize=signed-integer-overflow probably needs to be either/or, so one of those should take precedence over the other, e.g. -fsanitize=shift -fsanitize-recover=shift -ftrapv should result in recovering from shift UBs, not trap on them. My preference would be new set of ifns for -ftrapv, similar to .UBSAN_CHECK_{ADD,SUB,MUL}, say .TRAPV_CHECK_{ADD,SUB,MUL,DIV}, that uses moreless the same internal-fn.c expansion as .UBSAN_CHECK_*, but doesn't call ubsan_build_overflow_builtin and rely on flag_sanitize_undefined_trap_on_error, instead either emits the trap call directly, or also uses libcalls if optab isn't available and libcall is (or for -Os cases if libcall is smaller). Because some of the -fsanitize=signed-integer-overflow emitted code for multiplication at least on some architectures is very large... Jakub
On Wed, 20 Oct 2021, Richard Biener via Gcc-patches wrote: > This maps -ftrapv to -fsanitize=signed-integer-overflow > -fsanitize-undefined-trap-on-error, Isn't that UBSAN target-dependent, i.e. not supported on all targets, whereas -ftrapv is just about universally supported? I.e. isn't this patch breaking -ftrapv for some targets? brgds, H-P
On Thu, 28 Oct 2021, Hans-Peter Nilsson wrote: > On Wed, 20 Oct 2021, Richard Biener via Gcc-patches wrote: > > > This maps -ftrapv to -fsanitize=signed-integer-overflow > > -fsanitize-undefined-trap-on-error, > > Isn't that UBSAN target-dependent, i.e. not supported on all > targets, whereas -ftrapv is just about universally supported? I think only libubsan from libsanitizer has target dependences, -fsanitize-undefined-trap-on-error specifically avoids requiring -lubsan and thus should be fine in that regard. > I.e. isn't this patch breaking -ftrapv for some targets? I don't think so. As proposed the patch would make -ftrapv non-functional for non-C/C++ languages. But Jakub already stated he didn't like this simple approach. Note that the UBSAN instrumentation in the C/C++ frontends has the advantage over anything done in the middle-end that it reliably happens before early folding which might influence what and how things are instrumented. The next obvious places to instrument things would be the gimplifier or the ubsan pass. Richard.
diff --git a/gcc/opts.c b/gcc/opts.c index 65fe192a198..909d2a031ff 100644 --- a/gcc/opts.c +++ b/gcc/opts.c @@ -3022,7 +3022,21 @@ common_handle_option (struct gcc_options *opts, case OPT_ftrapv: if (value) - opts->x_flag_wrapv = 0; + { + opts->x_flag_wrapv = 0; + opts->x_flag_sanitize + = parse_sanitizer_options ("signed-integer-overflow", + loc, code, opts->x_flag_sanitize, + value, false); + if (!opts_set->x_flag_sanitize_undefined_trap_on_error) + opts->x_flag_sanitize_undefined_trap_on_error = 1; + /* This keeps overflow undefined and not trap. Specifically + it does no longer allow to catch exceptions together with + -fnon-call-exceptions. It also makes -ftrapv cease to + work with non-C-family languages since ubsan only works for + those. */ + opts->x_flag_trapv = 0; + } break; case OPT_fstrict_overflow:
This maps -ftrapv to -fsanitize=signed-integer-overflow -fsanitize-undefined-trap-on-error, effectively removing flag_trapv (or rather making it always false). This has implications on language support - while -ftrapv was formerly universally available the mapping restricts it to the C family of frontends. It also raises questions on mixing -ftrapv with -fsanitize flags, specifically with other recovery options for the undefined sanitizer since -fsanitize-undefined-trap-on-error cannot be restricted to the signed-integer-overflow part at the moment. To more closely map behavior we could add -fsanitize=trapv where with a single option we could also simply alias -ftrapv to that. Code quality wise a simple signed add compiles to movl %edi, %eax addl %esi, %eax jo .L5 ... .L5: ud2 compared to call __addvsi3 and it has less of the bugs -ftrapv has. The IL will not contain a PLUS_EXPR but a .UBSAN_CHECK_ADD internal function call which has rudimentary support throughout optimizers but is not recognized as possibly terminating the program so int foo (int i, int j, int *p, int k) { int tem = i + j; *p = 0; if (k) return tem; return 0; } will be optimized to perform the add only conditional and the possibly NULL *p dereference first (note the same happens with the "legacy" -ftrapv). The behavior with -fnon-call-exceptions is also different as the internal functions are marked as not throwing and as seen above the actual kind of trap can change (SIGILL vs. SIGABRT). One question is whether -ftrapv makes signed integer overflow well-defined (to trap) like -fwrapv makes it wrap. If so the the above behavior is ill-formed. Not sure how sanitizers position themselves with respect to this and whether the current behavior is OK there. The patch below instruments signed integer ops but leaves them undefined so the compiler still has to be careful as to not introduce new signed overflow (but at least that won't trap). Currently -fwrapv -fsanitize=signed-integer-overflow will not instrument any signed operations for example. I do consider the option to simply make -ftrapv do nothing but warn that people should use UBSAN - that wouldn't imply semantics are 1:1 the same (which they are not). Bootstrapped and tested on x86_64-unknown-linux-gnu, regresses FAIL: gcc.dg/vect/trapv-vect-reduc-4.c scan-tree-dump-times vect "Detected reduc tion\\\\." 3 FAIL: gcc.dg/vect/trapv-vect-reduc-4.c scan-tree-dump-times vect "using an in-or der \\\\(fold-left\\\\) reduction" 1 FAIL: gcc.dg/vect/trapv-vect-reduc-4.c scan-tree-dump-times vect "vectorized 3 l oops" 1 where the vectorizer doesn't know the UBSAN IFNs. 2021-10-20 Richard Biener <rguenther@suse.de> * opts.c (common_handle_option): Handle -ftrapv like -fsanitize=signed-integer-overflow -fsanitize-undefined-trap-on-error and do not set flag_trapv. --- gcc/opts.c | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-)