[Bug sanitizer/108845] New: Unnecessary signed integer overflow checks

[Bug sanitizer/108845] New: Unnecessary signed integer overflow checks

[qrzhang at gatech dot edu]

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

            Bug ID: 108845
           Summary: Unnecessary signed integer overflow checks
           Product: gcc
           Version: unknown
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: sanitizer
          Assignee: unassigned at gcc dot gnu.org
          Reporter: qrzhang at gatech dot edu
                CC: dodji at gcc dot gnu.org, dvyukov at gcc dot gnu.org,
                    jakub at gcc dot gnu.org, kcc at gcc dot gnu.org, marxin at gcc dot gnu.org
  Target Milestone: ---

When compiled with "gcc -fsanitize=signed-integer-overflow -O3" (reproducible
on the latest trunk version), Gcc will produce ".UBSAN_CHECK_SUB" checks for
the following code.

=======
void printf();
void main() {
  int a;
  for (a = 0; a != -3; a--)
    printf("%d\n", a);
}
========


I noticed that clang could indeed optimize this program to three printf() calls
with "-fsanitize=signed-integer-overflow -O3". Therefore, no overflow checks
are needed.

Is this behavior in GCC expected? Thanks!

[Bug sanitizer/108845] Unnecessary signed integer overflow checks

[pinskia at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |missed-optimization

--- Comment #1 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
We don't try to optimize out UBSAN_CHECK_SUB just yet.

[Bug sanitizer/108845] Unnecessary signed integer overflow checks

[jakub at gcc dot gnu.org]

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

--- Comment #2 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
I'm not convinced it is a good idea.
Sure, in the above case it is obvious it will never trigger, but if we say use
ranger to decide if the operation can or can't overflow, then VRP is in many
cases based on assumptions which only hold for valid code, but sanitizers
actually want to diagnose invalid code.

[Bug sanitizer/108845] Unnecessary signed integer overflow checks

[qrzhang at gatech dot edu]

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

--- Comment #3 from Qirun Zhang <qrzhang at gatech dot edu> ---
(In reply to Jakub Jelinek from comment #2)
> I'm not convinced it is a good idea.
> Sure, in the above case it is obvious it will never trigger, but if we say
> use ranger to decide if the operation can or can't overflow, then VRP is in
> many cases based on assumptions which only hold for valid code, but
> sanitizers actually want to diagnose invalid code.


Thanks!

Here is another (similar) example.  Earlier versions of GCC will not inject
UBSAN_CHECK_ADD. However, the latest version of GCC will.

the code example:
======
void main() {
  int a = 0;
  for (; a != 2; a++)
    ;
}
======

Compile with "gcc-11 -fsanitize=signed-integer-overflow -O3 
-fdump-tree-optimized", we got no UBSAN checks:

======
void main ()
{
  int a;

  <bb 2> [local count: 118111600]:

  <bb 3> [local count: 955630225]:
  # a_6 = PHI <1(3), 0(2)>
  a_3 = a_6 + 1;
  if (a_3 != 2)
    goto <bb 3>; [87.64%]
  else
    goto <bb 4>; [12.36%]

  <bb 4> [local count: 118111600]:
  return;

}
======

Compile with "gcc-trunk -fsanitize=signed-integer-overflow -O3 
-fdump-tree-optimized", we got one:

======
void main ()
{
  int a;

  <bb 2> [local count: 118111600]:

  <bb 3> [local count: 955630225]:
  # a_5 = PHI <a_3(3), 0(2)>
  a_3 = .UBSAN_CHECK_ADD (a_5, 1);
  if (a_3 != 2)
    goto <bb 3>; [89.00%]
  else
    goto <bb 4>; [11.00%]

  <bb 4> [local count: 118111600]:
  return;

}
======

$ gcc-trunk -v
gcc version 13.0.1 20230218 (experimental) [master r13-6132-g32b5875c911] (GCC)

[Bug sanitizer/108845] Unnecessary signed integer overflow checks

[qrzhang at gatech dot edu]

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

--- Comment #4 from Qirun Zhang <qrzhang at gatech dot edu> ---
(In reply to Qirun Zhang from comment #3)
> (In reply to Jakub Jelinek from comment #2)
> > I'm not convinced it is a good idea.
> > Sure, in the above case it is obvious it will never trigger, but if we say
> > use ranger to decide if the operation can or can't overflow, then VRP is in
> > many cases based on assumptions which only hold for valid code, but
> > sanitizers actually want to diagnose invalid code.
> 
> 
> Thanks!
> 
> Here is another (similar) example.  Earlier versions of GCC will not inject
> UBSAN_CHECK_ADD. However, the latest version of GCC will.
> 
> the code example:
> ======
> void main() {
>   int a = 0;
>   for (; a != 2; a++)
>     ;
> }
> ======
> 
> Compile with "gcc-11 -fsanitize=signed-integer-overflow -O3 
> -fdump-tree-optimized", we got no UBSAN checks:
> 
> ======
> void main ()
> {
>   int a;
> 
>   <bb 2> [local count: 118111600]:
> 
>   <bb 3> [local count: 955630225]:
>   # a_6 = PHI <1(3), 0(2)>
>   a_3 = a_6 + 1;
>   if (a_3 != 2)
>     goto <bb 3>; [87.64%]
>   else
>     goto <bb 4>; [12.36%]
> 
>   <bb 4> [local count: 118111600]:
>   return;
> 
> }
> ======
> 
> Compile with "gcc-trunk -fsanitize=signed-integer-overflow -O3 
> -fdump-tree-optimized", we got one:
> 
> ======
> void main ()
> {
>   int a;
> 
>   <bb 2> [local count: 118111600]:
> 
>   <bb 3> [local count: 955630225]:
>   # a_5 = PHI <a_3(3), 0(2)>
>   a_3 = .UBSAN_CHECK_ADD (a_5, 1);
>   if (a_3 != 2)
>     goto <bb 3>; [89.00%]
>   else
>     goto <bb 4>; [11.00%]
> 
>   <bb 4> [local count: 118111600]:
>   return;
> 
> }
> ======
> 
> $ gcc-trunk -v
> gcc version 13.0.1 20230218 (experimental) [master r13-6132-g32b5875c911]
> (GCC)

I did a bisect. For the testcase provided in comment #3, the behavior was
introduced in the following commit:


commit 502ffb1f389011b28ee51815242c7397790802d5
Author: Andrew MacLeod <amacleod@redhat.com>
Date:   Tue Nov 2 21:26:44 2021 -0400

    Switch vrp2 to ranger.

    This patch flips the default for the VRP2 pass to execute ranger vrp rather
    than the assert_expr version of VRP.

            * params.opt (param_vrp2_mode): Make ranger the default for VRP2.