[PATCH][RFC] Map -ftrapv to -fsanitize=signed-integer-overflow -fsanitize-undefined-trap-on-error

[Martin Sebor]

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