[Bug c/114700] New: Front-end optimization generates wrong code with -ftrapv.

[Bug c/114700] New: Front-end optimization generates wrong code with -ftrapv.

[lin1.hu at intel dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114700

            Bug ID: 114700
           Summary: Front-end optimization generates wrong code with
                    -ftrapv.
           Product: gcc
           Version: unknown
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c
          Assignee: unassigned at gcc dot gnu.org
          Reporter: lin1.hu at intel dot com
  Target Milestone: ---

We test GCC vs Clang with -ftrapv, and test is 
z = c  - y  - c + a  + y - b;
https://godbolt.org/z/EW1xTsazG

We think the clang is right, the overflow judgment should be performed after
each operation. But the front-end generates a - b directly, looks like there's
a bug in the front-end's handling of -ftrapv.

-ftrapv
This option generates traps for signed overflow on addition, subtraction,
multiplication operations. The options -ftrapv and -fwrapv override each other,
so using -ftrapv -fwrapv on the command-line results in -fwrapv being
effective. Note that only active options override, so using -ftrapv -fwrapv
-fno-wrapv on the command-line results in -ftrapv being effective.


We have another question, we found the front-end won't optimize z = c - y + a -
c, while z = c - y - c + a will, is this for any particular reason or is it a
bug?

[Bug middle-end/114700] Front-end optimization generates wrong code with -ftrapv.

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114700

--- Comment #1 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Gcc's trapv is known not always to work correctly.

Try -fsanitize=undefined instead.

[Bug middle-end/114700] Front-end optimization generates wrong code with -ftrapv.

[lin1.hu at intel dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114700

--- Comment #2 from lin1.hu at intel dot com ---
(In reply to Andrew Pinski from comment #1)
> Gcc's trapv is known not always to work correctly.
> 
> Try -fsanitize=undefined instead.

Thanks, it solves the problem to some extent. But c is eliminated, I think c -
y may cause signed overflow, c

https://godbolt.org/z/Wbzx5Edsj

[Bug middle-end/114700] Front-end optimization generates wrong code with -ftrapv.

[lin1.hu at intel dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114700

--- Comment #3 from lin1.hu at intel dot com ---
(In reply to lin1.hu from comment #2)
> (In reply to Andrew Pinski from comment #1)
> > Gcc's trapv is known not always to work correctly.
> > 
> > Try -fsanitize=undefined instead.

Thanks, it solves the problem to some extent. But c is eliminated, I think c
- y may cause signed overflow.

https://godbolt.org/z/Wbzx5Edsj

[Bug middle-end/114700] Front-end optimization generates wrong code with -ftrapv.

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114700

--- Comment #4 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
int z;
void func(int a, int b, int c, int y){
    z = c  - y  - c + a  + y - b;
}

[Bug middle-end/114700] Front-end optimization generates wrong code with -ftrapv.

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114700

--- Comment #5 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
From match.pd:
  /* Match patterns that allow contracting a plus-minus pair
     irrespective of overflow issues.  */
  /* (A +- B) - A       ->  +- B */
  /* (A +- B) -+ B      ->  A */
  /* A - (A +- B)       -> -+ B */
  /* A +- (B -+ A)      ->  +- B */
  (simplify
   (minus (nop_convert1? (plus:c (nop_convert2? @0) @1)) @0)
   (view_convert @1))
  (simplify
   (minus (nop_convert1? (minus (nop_convert2? @0) @1)) @0)
   (if (!ANY_INTEGRAL_TYPE_P (type)
        || TYPE_OVERFLOW_WRAPS (type))
   (negate (view_convert @1))
   (view_convert (negate @1))))

Looks like missing a TYPE_OVERFLOW_SANITIZED check.

[Bug middle-end/114700] Front-end optimization generates wrong code with -ftrapv.

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114700

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           See Also|                            |https://gcc.gnu.org/bugzill
                   |                            |a/show_bug.cgi?id=106885,
                   |                            |https://gcc.gnu.org/bugzill
                   |                            |a/show_bug.cgi?id=66630

--- Comment #6 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Note `c  - y  - c` to become `-y` reduces the possible of an overflow and is
well defined for wrapping so this might be still on purpose as there will never
be an overflow that causes difference if assuming wrapping ...

[Bug middle-end/114700] Front-end optimization generates wrong code with -ftrapv.

[lin1.hu at intel dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114700

--- Comment #7 from lin1.hu at intel dot com ---
(In reply to Andrew Pinski from comment #5)
> From match.pd:
>   /* Match patterns that allow contracting a plus-minus pair
>      irrespective of overflow issues.  */
>   /* (A +- B) - A       ->  +- B */
>   /* (A +- B) -+ B      ->  A */
>   /* A - (A +- B)       -> -+ B */
>   /* A +- (B -+ A)      ->  +- B */
>   (simplify
>    (minus (nop_convert1? (plus:c (nop_convert2? @0) @1)) @0)
>    (view_convert @1))
>   (simplify
>    (minus (nop_convert1? (minus (nop_convert2? @0) @1)) @0)
>    (if (!ANY_INTEGRAL_TYPE_P (type)
>         || TYPE_OVERFLOW_WRAPS (type))
>    (negate (view_convert @1))
>    (view_convert (negate @1))))
> 
> Looks like missing a TYPE_OVERFLOW_SANITIZED check.

OK, this looks like the same reason why z = c - y + a - c is not optimized.

[Bug middle-end/114700] Front-end optimization generates wrong code with -ftrapv.

[lin1.hu at intel dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114700

--- Comment #8 from lin1.hu at intel dot com ---
(In reply to Andrew Pinski from comment #6)
> Note `c  - y  - c` to become `-y` reduces the possible of an overflow and is
> well defined for wrapping so this might be still on purpose as there will
> never be an overflow that causes difference if assuming wrapping ...

Indeed, so for -fsanitize=undefined, `c  - y  - c` become `-y` is right. It's
just missing the optimization to turn `z = c  - y  - c + a  + y - b` into `z =
a - b`.

[Bug middle-end/114700] Front-end optimization generates wrong code with -ftrapv.

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114700

--- Comment #9 from Richard Biener <rguenth at gcc dot gnu.org> ---
That that GCC doesn't promise that -ftrapv preserves all overflows and traps,
it merely guarantees that all overflows that actually happen trap.  So GCC is
fine to contract some expressions where the overall number of overflows can
only
decrease.

That's not a bug with -ftrapv.

It is considered a bug with -fsanitize=undefined though.

[Bug middle-end/114700] Front-end optimization generates wrong code with -fsanitize=undefined

[xry111 at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114700

Xi Ruoyao <xry111 at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|Front-end optimization      |Front-end optimization
                   |generates wrong code with   |generates wrong code with
                   |-ftrapv.                    |-fsanitize=undefined
                 CC|                            |xry111 at gcc dot gnu.org

--- Comment #10 from Xi Ruoyao <xry111 at gcc dot gnu.org> ---
(In reply to Richard Biener from comment #9)
> That that GCC doesn't promise that -ftrapv preserves all overflows and
> traps, it merely guarantees that all overflows that actually happen trap. 
> So GCC is fine to contract some expressions where the overall number of
> overflows can only
> decrease.
> 
> That's not a bug with -ftrapv.
> 
> It is considered a bug with -fsanitize=undefined though.

So modifying the subject.

[Bug middle-end/114700] Front-end optimization generates wrong code with -fsanitize=undefined

[lin1.hu at intel dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114700

--- Comment #11 from lin1.hu at intel dot com ---
(In reply to Richard Biener from comment #9)
> That that GCC doesn't promise that -ftrapv preserves all overflows and
> traps, it merely guarantees that all overflows that actually happen trap. 
> So GCC is fine to contract some expressions where the overall number of
> overflows can only
> decrease.
> 
> That's not a bug with -ftrapv.
> 
> It is considered a bug with -fsanitize=undefined though.

I think it doesn't mean that's not a bug with -ftrapv, it should preserve all
overflow traps. Because it doesn't work, we use -fsanitize=undefined instead of
it.

refer: Gcc's trapv is known not always to work correctly.

The current behavior is correct for -fsanitize=undefined, because the integer
signed overflow is well-defined, so GCC can eliminate some variables. I just
think GCC can optimize `z = c  - y  - c + a  + y - b` to `z = a - b`. But it
doesn't mean is a bug for -fsanitize=undefined.

[Bug middle-end/114700] Front-end optimization generates wrong code with -fsanitize=undefined

[lin1.hu at intel dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114700

--- Comment #12 from Hu Lin <lin1.hu at intel dot com> ---
(In reply to Hu Lin from comment #11)
> (In reply to Richard Biener from comment #9)
> > That that GCC doesn't promise that -ftrapv preserves all overflows and
> > traps, it merely guarantees that all overflows that actually happen trap. 
> > So GCC is fine to contract some expressions where the overall number of
> > overflows can only
> > decrease.
> > 
> > That's not a bug with -ftrapv.
> > 
> > It is considered a bug with -fsanitize=undefined though.
> 
> I think it doesn't mean that's not a bug with -ftrapv, it should preserve
> all overflow traps. Because it doesn't work, we use -fsanitize=undefined
> instead of it.
> 
> refer: Gcc's trapv is known not always to work correctly.
> 
> The current behavior is correct for -fsanitize=undefined, because the
> integer signed overflow is well-defined, so GCC can eliminate some
> variables. I just think GCC can optimize `z = c  - y  - c + a  + y - b` to
> `z = a - b`. But it doesn't mean is a bug for -fsanitize=undefined.

I was wrong about signed overflow, it's undefined behavior in c++20
(https://en.cppreference.com/w/cpp/language/ub). If I'm not mistaken about the
source， it's a bug.

[Bug middle-end/114700] Front-end optimization generates wrong code with -fsanitize=undefined

[xry111 at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114700

--- Comment #13 from Xi Ruoyao <xry111 at gcc dot gnu.org> ---
And IIRC there are various suggestion saying "if you want -fwrapv, you are
likely actually wanting -fsanitize=signed-integer-overflow" and some plan
deprecating -fwrapv.  So it's more important to fix the sanitizer issue.

[Bug middle-end/114700] Front-end optimization generates wrong code with -fsanitize=undefined

[lin1.hu at intel dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114700

--- Comment #14 from Hu Lin <lin1.hu at intel dot com> ---
Created attachment 57933
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=57933&action=edit
Untested fix.

[Bug middle-end/114700] middle-end optimization generates wrong code with -fsanitize=undefined

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114700

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jakub at gcc dot gnu.org

--- Comment #15 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
(In reply to Hu Lin from comment #14)
> Created attachment 57933 [details]
> Untested fix.

When a pattern already has one if, can't you just add that to the preexisting
if rather than adding yet another one.

[Bug middle-end/114700] middle-end optimization generates wrong code with -fsanitize=undefined

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114700

--- Comment #16 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
(In reply to Hu Lin from comment #11)
> I think it doesn't mean that's not a bug with -ftrapv, it should preserve
> all overflow traps. Because it doesn't work, we use -fsanitize=undefined
> instead of it.
> 
> refer: Gcc's trapv is known not always to work correctly.

No, -ftrapv isn't a debugging tool.  There is no overflow in the expression
that GCC actually evaluates (into which the expression has been optimized).
If you have overflow in an expression that is never used, GCC with -ftrapv will
also
eliminate it as unused and won't diagnose the trap.
-fsanitize=undefined behaves in that case actually the same with -O1 and higher
(intentionally, to decrease the cost of the sanitization).  So, one needs to
use -O0 -fsanitize=undefined to get as many cases of UB in the program
diagnosed as possible.

[Bug middle-end/114700] middle-end optimization generates causes -fsanitize=undefined not to happen in some cases

[lin1.hu at intel dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114700

--- Comment #17 from Hu Lin <lin1.hu at intel dot com> ---
(In reply to Jakub Jelinek from comment #16)
> (In reply to Hu Lin from comment #11)
> > I think it doesn't mean that's not a bug with -ftrapv, it should preserve
> > all overflow traps. Because it doesn't work, we use -fsanitize=undefined
> > instead of it.
> > 
> > refer: Gcc's trapv is known not always to work correctly.
> 
> No, -ftrapv isn't a debugging tool.  There is no overflow in the expression
> that GCC actually evaluates (into which the expression has been optimized).
> If you have overflow in an expression that is never used, GCC with -ftrapv
> will also
> eliminate it as unused and won't diagnose the trap.
> -fsanitize=undefined behaves in that case actually the same with -O1 and
> higher (intentionally, to decrease the cost of the sanitization).  So, one
> needs to use -O0 -fsanitize=undefined to get as many cases of UB in the
> program diagnosed as possible.

OK, that look like GCC's -ftrapv is not the same as clang's. Then my added
condition should be (optimize || !TYPE_OVERFLOW_SANITIZED (type)). 

> When a pattern already has one if, can't you just add that to the preexisting if rather than adding yet another one.

I made a mistake on this line, it should be
+   (if (!TYPE_OVERFLOW_SANITIZED (type))
     (if (!ANY_INTEGRAL_TYPE_P (type)
         || TYPE_OVERFLOW_WRAPS (type))
      (negate (view_convert @1))
      (view_convert (negate @1))))

I can't just modify the preexisting if, the optimization shouldn't be used with
-fsanitize=undefined.

[Bug middle-end/114700] middle-end optimization generates causes -fsanitize=undefined not to happen in some cases

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114700

--- Comment #18 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
(In reply to Hu Lin from comment #17)
> (In reply to Jakub Jelinek from comment #16)
> > (In reply to Hu Lin from comment #11)
> > > I think it doesn't mean that's not a bug with -ftrapv, it should preserve
> > > all overflow traps. Because it doesn't work, we use -fsanitize=undefined
> > > instead of it.
> > > 
> > > refer: Gcc's trapv is known not always to work correctly.
> > 
> > No, -ftrapv isn't a debugging tool.  There is no overflow in the expression
> > that GCC actually evaluates (into which the expression has been optimized).
> > If you have overflow in an expression that is never used, GCC with -ftrapv
> > will also
> > eliminate it as unused and won't diagnose the trap.
> > -fsanitize=undefined behaves in that case actually the same with -O1 and
> > higher (intentionally, to decrease the cost of the sanitization).  So, one
> > needs to use -O0 -fsanitize=undefined to get as many cases of UB in the
> > program diagnosed as possible.
> 
> OK, that look like GCC's -ftrapv is not the same as clang's. Then my added
> condition should be (optimize || !TYPE_OVERFLOW_SANITIZED (type)). 

Why?  Just !TYPE_OVERFLOW_SANITIZED (type).

> > When a pattern already has one if, can't you just add that to the preexisting if rather than adding yet another one.
> 
> I made a mistake on this line, it should be
> +   (if (!TYPE_OVERFLOW_SANITIZED (type))
>      (if (!ANY_INTEGRAL_TYPE_P (type)
>          || TYPE_OVERFLOW_WRAPS (type))
>       (negate (view_convert @1))
>       (view_convert (negate @1))))
> 
> I can't just modify the preexisting if, the optimization shouldn't be used
> with -fsanitize=undefined.

TYPE_OVERFLOW_SANITIZED is
#define TYPE_OVERFLOW_SANITIZED(TYPE)                   \
  (INTEGRAL_TYPE_P (TYPE)                               \
   && !TYPE_OVERFLOW_WRAPS (TYPE)                       \
   && (flag_sanitize & SANITIZE_SI_OVERFLOW))
so, it isn't true for non-integral types, nor for TYPE_OVERFLOW_WRAPS types.
So, if you want to avoid the (view_convert (negate @1)), just add (if
!TYPE_OVERFLOW_SANITIZED (type)) above the (view_convert (negate @1)).  But in
each case, you want to be careful which exact type you want to check, type is
the type of
the outermost expression, otherwise TREE_TYPE (@0) etc.

[Bug middle-end/114700] middle-end optimization generates causes -fsanitize=undefined not to happen in some cases

[lin1.hu at intel dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114700

--- Comment #19 from Hu Lin <lin1.hu at intel dot com> ---
(In reply to Jakub Jelinek from comment #18)
> (In reply to Hu Lin from comment #17)
> > (In reply to Jakub Jelinek from comment #16)
> > > 
> > > No, -ftrapv isn't a debugging tool.  There is no overflow in the expression
> > > that GCC actually evaluates (into which the expression has been optimized).
> > > If you have overflow in an expression that is never used, GCC with -ftrapv
> > > will also
> > > eliminate it as unused and won't diagnose the trap.
> > > -fsanitize=undefined behaves in that case actually the same with -O1 and
> > > higher (intentionally, to decrease the cost of the sanitization).  So, one
> > > needs to use -O0 -fsanitize=undefined to get as many cases of UB in the
> > > program diagnosed as possible.
> > 
> > OK, that look like GCC's -ftrapv is not the same as clang's. Then my added
> > condition should be (optimize || !TYPE_OVERFLOW_SANITIZED (type)). 
> 
> Why?  Just !TYPE_OVERFLOW_SANITIZED (type).
> 

OK, so the part is one of your suggestions on how to test UB in a program. 
I have another question, -fsanitize=undefined disable this optimization, but
you said -ftrapv won't diagnose the trap. Why is the logic here different for
these two options？

> 
> TYPE_OVERFLOW_SANITIZED is
> #define TYPE_OVERFLOW_SANITIZED(TYPE)                   \
>   (INTEGRAL_TYPE_P (TYPE)                               \
>    && !TYPE_OVERFLOW_WRAPS (TYPE)                       \
>    && (flag_sanitize & SANITIZE_SI_OVERFLOW))
> so, it isn't true for non-integral types, nor for TYPE_OVERFLOW_WRAPS types.
> So, if you want to avoid the (view_convert (negate @1)), just add (if
> !TYPE_OVERFLOW_SANITIZED (type)) above the (view_convert (negate @1)).  But
> in each case, you want to be careful which exact type you want to check,
> type is the type of
> the outermost expression, otherwise TREE_TYPE (@0) etc.

Thanks for your advice.

[Bug middle-end/114700] middle-end optimization generates causes -fsanitize=undefined not to happen in some cases

[lin1.hu at intel dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114700

--- Comment #20 from Hu Lin <lin1.hu at intel dot com> ---
Created attachment 57967
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=57967&action=edit
A new version

When I tested this patch, I met another question. g++.dg/ubsan/vla-1.C will
raise a ICE without (TREE_TYPE (@2) == ssizetype at match.pd:3497.

In the original step, GCC generates the intermediate language using variables
declared in other blocks. Like _5 = _1 + 1, this led to the ICE in 022t.ssa. I
don't know if it is a bug, and I didn't find a test to raise this ICE on trunk.
So I add a condition to avoid this optimization in this case, any other
comments on my newly added conditions?



I paste some information that I think is important.

vla-1.C.005t.original:

The original line 12 is 
__builtin___ubsan_handle_vla_bound_not_positive (&*.Lubsan_data0, (unsigned
long) ((ssizetype) SAVE_EXPR <i>));

The current is
__builtin___ubsan_handle_vla_bound_not_positive (&*.Lubsan_data0, (unsigned
long) (((ssizetype) SAVE_EXPR <i> - 1) + 1));


vla-1.C.006t.gimple
original:
 22       i.0 = i;
 23       if (i.0 <= 0) goto <D.3270>; else goto <D.3271>;
 24       <D.3270>:
 25       _1 = (unsigned long) i.0;
 26       __builtin___ubsan_handle_vla_bound_not_positive (&*.Lubsan_data0,
_1);
 27       goto <D.3272>;
 28       <D.3271>:
 29       <D.3272>:
 30       _2 = (ssizetype) i.0;
 31       _3 = _2 - 1;

current:
 22       i.0 = i;
 23       if (i.0 <= 0) goto <D.3270>; else goto <D.3271>;
 24       <D.3270>:
 25       _1 = (ssizetype) i.0;
 26       _2 = _1 - 1;
 27       _3 = _2 + 1;
 28       _4 = (unsigned long) _3;
 29       __builtin___ubsan_handle_vla_bound_not_positive (&*.Lubsan_data0,
_4);
 30       goto <D.3272>;
 31       <D.3271>:
 32       <D.3272>:
 33       _5 = _1 - 1;


vla-1.C.015t.cfg
original:
 37   int i.0;
 38
 39   <bb 2> :
 40   saved_stack.5 = __builtin_stack_save ();
 41   i.0 = i;
 42   if (i.0 <= 0)
 43     goto <bb 3>; [INV]
 44   else
 45     goto <bb 4>; [INV]
 46
 47   <bb 3> :
 48   _1 = (unsigned long) i.0;
 49   __builtin___ubsan_handle_vla_bound_not_positive (&*.Lubsan_data0, _1);
 50
 51   <bb 4> :
 52   _2 = (ssizetype) i.0;
 53   _3 = _2 - 1;

current:
 37   int i.0;
 38
 39   <bb 2> :
 40   saved_stack.5 = __builtin_stack_save ();
 41   i.0 = i;
 42   if (i.0 <= 0)
 43     goto <bb 3>; [INV]
 44   else
 45     goto <bb 4>; [INV]
 46
 47   <bb 3> :
 48   _1 = (ssizetype) i.0;
 49   _2 = _1 - 1;
 50   _3 = _2 + 1;
 51   _4 = (unsigned long) _3;
 52   __builtin___ubsan_handle_vla_bound_not_positive (&*.Lubsan_data0, _4);
 53
 54   <bb 4> :
 55   _5 = _1 - 1;
 56   _6 = (sizetype) _5;
 57   D.3273 = _6;