Patchwork [RFA] Fix tree-optimization/57124

login
register
mail settings
Submitter Jeff Law
Date May 1, 2013, 8:26 p.m.
Message ID <51817A69.90805@redhat.com>
Download mbox | patch
Permalink /patch/240811/
State New
Headers show

Comments

Jeff Law - May 1, 2013, 8:26 p.m.
range_fits_type_p erroneously returns true in cases where the range has 
overflowed.  So for example, we might have a range [0, +INF(OVF)] and 
conclude the range fits in an unsigned type.

This in turn can cause VRP to rewrite a conditional in an unsafe way as 
seen by the testcase.

Bootstrapped and regression tested on x86_64-unknown-linux-gnu.

OK for the trunk?
commit 45d8f974a4fae7bf07b7213b4ccda81fe410d49b
Author: Jeff Law <law@redhat.com>
Date:   Wed May 1 12:33:20 2013 -0600

    	PR tree-optimization/57124
            * tree-vrp.c (range_fits_type_p): If min/max of the range has
            overflowed, then the range does not fit the type.
    
            PR tree-optimization/57124
            * gcc.c-torture/execute/pr57124.c: New test.
Richard Guenther - May 2, 2013, 7:55 a.m.
On Wed, May 1, 2013 at 10:26 PM, Jeff Law <law@redhat.com> wrote:
>
> range_fits_type_p erroneously returns true in cases where the range has
> overflowed.  So for example, we might have a range [0, +INF(OVF)] and
> conclude the range fits in an unsigned type.
>
> This in turn can cause VRP to rewrite a conditional in an unsafe way as seen
> by the testcase.
>
> Bootstrapped and regression tested on x86_64-unknown-linux-gnu.
>
> OK for the trunk?

Hmm, actually when [0, +INF(OVF)] appears then it says that the
range is [0, +INF], but we relied on that no undefined overflow happened
when computing it.

So in fact the value must fit, otherwise the program invoked undefined
behavior.

Which means you should at most _warn_, not disable the transform.

In fact the testcase is invalid:

  x1 = *p1;
  x2 = (int) x1;
  x3 = x2 * 65536;

performs 65531 * 65536 which overflows in type int.

You probably should disable the transform with -fno-strict-overflow,
that is, make sure the testcase works at -O1 -ftree-vrp for example.

Richard.

> commit 45d8f974a4fae7bf07b7213b4ccda81fe410d49b
> Author: Jeff Law <law@redhat.com>
> Date:   Wed May 1 12:33:20 2013 -0600
>
>         PR tree-optimization/57124
>             * tree-vrp.c (range_fits_type_p): If min/max of the range has
>             overflowed, then the range does not fit the type.
>
>             PR tree-optimization/57124
>             * gcc.c-torture/execute/pr57124.c: New test.
>
> diff --git a/gcc/ChangeLog b/gcc/ChangeLog
> index 50a3b1d..658ddda 100644
> --- a/gcc/ChangeLog
> +++ b/gcc/ChangeLog
> @@ -1,3 +1,9 @@
> +2013-04-26  Jeff Law  <law@redhat.com>
> +
> +       PR tree-optimization/57124
> +       * tree-vrp.c (range_fits_type_p): If min/max of the range has
> +       overflowed, then the range does not fit the type.
> +
>  2013-04-30  Greta Yorsh  <Greta.Yorsh@arm.com>
>
>         * config/arm/thumb2.md (thumb2_incscc, thumb2_decscc): Delete.
> diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog
> index 1016036..3ed531e 100644
> --- a/gcc/testsuite/ChangeLog
> +++ b/gcc/testsuite/ChangeLog
> @@ -1,3 +1,8 @@
> +2013-05-01  Jeff Law  <law@redhat.com>
> +
> +       PR tree-optimization/57124
> +       * gcc.c-torture/execute/pr57124.c: New test.
> +
>  2013-04-30  Thomas Koenig  <tkoenig@gcc.gnu.org>
>
>         PR fortran/57071
> diff --git a/gcc/testsuite/gcc.c-torture/execute/pr57124.c
> b/gcc/testsuite/gcc.c-torture/execute/pr57124.c
> new file mode 100644
> index 0000000..835d249
> --- /dev/null
> +++ b/gcc/testsuite/gcc.c-torture/execute/pr57124.c
> @@ -0,0 +1,27 @@
> +__attribute__ ((noinline))
> +foo(short unsigned int *p1, short unsigned int *p2)
> +{
> +  short unsigned int x1, x4;
> +  int x2, x3, x5, x6;
> +  unsigned int x7;
> +
> +  x1 = *p1;
> +  x2 = (int) x1;
> +  x3 = x2 * 65536;
> +  x4 = *p2;
> +  x5 = (int) x4;
> +  x6 = x3 + x4;
> +  x7 = (unsigned int) x6;
> +  if (x7 <= 268435455U)
> +    abort ();
> +  exit (0);
> +}
> +
> +main()
> +{
> +  short unsigned int x, y;
> +  x = -5;
> +  y = -10;
> +  foo (&x, &y);
> +}
> +
> diff --git a/gcc/tree-vrp.c b/gcc/tree-vrp.c
> index 6ed353f..02f2f19 100644
> --- a/gcc/tree-vrp.c
> +++ b/gcc/tree-vrp.c
> @@ -8537,7 +8537,9 @@ range_fits_type_p (value_range_t *vr, unsigned
> precision, bool unsigned_p)
>    /* Now we can only handle ranges with constant bounds.  */
>    if (vr->type != VR_RANGE
>        || TREE_CODE (vr->min) != INTEGER_CST
> -      || TREE_CODE (vr->max) != INTEGER_CST)
> +      || TREE_CODE (vr->max) != INTEGER_CST
> +      || is_negative_overflow_infinity (vr->min)
> +      || is_positive_overflow_infinity (vr->max))
>      return false;
>
>    /* For sign changes, the MSB of the double_int has to be clear.
>
Jeff Law - May 3, 2013, 8:46 p.m.
On 05/02/2013 01:55 AM, Richard Biener wrote:
> On Wed, May 1, 2013 at 10:26 PM, Jeff Law <law@redhat.com> wrote:
>>
>> range_fits_type_p erroneously returns true in cases where the range has
>> overflowed.  So for example, we might have a range [0, +INF(OVF)] and
>> conclude the range fits in an unsigned type.
>>
>> This in turn can cause VRP to rewrite a conditional in an unsafe way as seen
>> by the testcase.
>>
>> Bootstrapped and regression tested on x86_64-unknown-linux-gnu.
>>
>> OK for the trunk?
>
> Hmm, actually when [0, +INF(OVF)] appears then it says that the
> range is [0, +INF], but we relied on that no undefined overflow happened
> when computing it.
>
> So in fact the value must fit, otherwise the program invoked undefined
> behavior.
>
> Which means you should at most _warn_, not disable the transform.
>
> In fact the testcase is invalid:
>
>    x1 = *p1;
>    x2 = (int) x1;
>    x3 = x2 * 65536;
>
> performs 65531 * 65536 which overflows in type int.
>
> You probably should disable the transform with -fno-strict-overflow,
> that is, make sure the testcase works at -O1 -ftree-vrp for example.
I'll check the inputs going into the code in 254.gap; the sequence in 
the testcase is effectively the computation that's going awry in 254.gap.


Jeff
Richard Guenther - May 6, 2013, 11:10 a.m.
On Fri, May 3, 2013 at 10:46 PM, Jeff Law <law@redhat.com> wrote:
> On 05/02/2013 01:55 AM, Richard Biener wrote:
>>
>> On Wed, May 1, 2013 at 10:26 PM, Jeff Law <law@redhat.com> wrote:
>>>
>>>
>>> range_fits_type_p erroneously returns true in cases where the range has
>>> overflowed.  So for example, we might have a range [0, +INF(OVF)] and
>>> conclude the range fits in an unsigned type.
>>>
>>> This in turn can cause VRP to rewrite a conditional in an unsafe way as
>>> seen
>>> by the testcase.
>>>
>>> Bootstrapped and regression tested on x86_64-unknown-linux-gnu.
>>>
>>> OK for the trunk?
>>
>>
>> Hmm, actually when [0, +INF(OVF)] appears then it says that the
>> range is [0, +INF], but we relied on that no undefined overflow happened
>> when computing it.
>>
>> So in fact the value must fit, otherwise the program invoked undefined
>> behavior.
>>
>> Which means you should at most _warn_, not disable the transform.
>>
>> In fact the testcase is invalid:
>>
>>    x1 = *p1;
>>    x2 = (int) x1;
>>    x3 = x2 * 65536;
>>
>> performs 65531 * 65536 which overflows in type int.
>>
>> You probably should disable the transform with -fno-strict-overflow,
>> that is, make sure the testcase works at -O1 -ftree-vrp for example.
>
> I'll check the inputs going into the code in 254.gap; the sequence in the
> testcase is effectively the computation that's going awry in 254.gap.

Thanks - it's not the first time that GCC hits undefined C code in SPEC CPU.
Generally we make sure that a workaround exists, like -fno-strict-overflow.

Richard.

> Jeff

Patch

diff --git a/gcc/ChangeLog b/gcc/ChangeLog
index 50a3b1d..658ddda 100644
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,3 +1,9 @@ 
+2013-04-26  Jeff Law  <law@redhat.com>
+
+	PR tree-optimization/57124
+	* tree-vrp.c (range_fits_type_p): If min/max of the range has
+	overflowed, then the range does not fit the type.
+
 2013-04-30  Greta Yorsh  <Greta.Yorsh@arm.com>
 
 	* config/arm/thumb2.md (thumb2_incscc, thumb2_decscc): Delete.
diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog
index 1016036..3ed531e 100644
--- a/gcc/testsuite/ChangeLog
+++ b/gcc/testsuite/ChangeLog
@@ -1,3 +1,8 @@ 
+2013-05-01  Jeff Law  <law@redhat.com>
+
+	PR tree-optimization/57124
+	* gcc.c-torture/execute/pr57124.c: New test.
+
 2013-04-30  Thomas Koenig  <tkoenig@gcc.gnu.org>
 
 	PR fortran/57071
diff --git a/gcc/testsuite/gcc.c-torture/execute/pr57124.c b/gcc/testsuite/gcc.c-torture/execute/pr57124.c
new file mode 100644
index 0000000..835d249
--- /dev/null
+++ b/gcc/testsuite/gcc.c-torture/execute/pr57124.c
@@ -0,0 +1,27 @@ 
+__attribute__ ((noinline))
+foo(short unsigned int *p1, short unsigned int *p2)
+{
+  short unsigned int x1, x4;
+  int x2, x3, x5, x6;
+  unsigned int x7;
+  
+  x1 = *p1;
+  x2 = (int) x1;
+  x3 = x2 * 65536;
+  x4 = *p2;
+  x5 = (int) x4;
+  x6 = x3 + x4;
+  x7 = (unsigned int) x6;
+  if (x7 <= 268435455U)
+    abort ();
+  exit (0);
+}
+
+main()
+{
+  short unsigned int x, y;
+  x = -5;
+  y = -10;
+  foo (&x, &y);
+}
+
diff --git a/gcc/tree-vrp.c b/gcc/tree-vrp.c
index 6ed353f..02f2f19 100644
--- a/gcc/tree-vrp.c
+++ b/gcc/tree-vrp.c
@@ -8537,7 +8537,9 @@  range_fits_type_p (value_range_t *vr, unsigned precision, bool unsigned_p)
   /* Now we can only handle ranges with constant bounds.  */
   if (vr->type != VR_RANGE
       || TREE_CODE (vr->min) != INTEGER_CST
-      || TREE_CODE (vr->max) != INTEGER_CST)
+      || TREE_CODE (vr->max) != INTEGER_CST
+      || is_negative_overflow_infinity (vr->min)
+      || is_positive_overflow_infinity (vr->max))
     return false;
 
   /* For sign changes, the MSB of the double_int has to be clear.