| Message ID | 51817A69.90805@redhat.com |
|---|---|
| State | New |
| Headers | show |
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. >
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
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
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.
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.