| Message ID | 5742E6B2.7050904@foss.arm.com |
|---|---|
| State | New |
| Headers | show |
On Mon, May 23, 2016 at 1:17 PM, Kyrill Tkachov <kyrylo.tkachov@foss.arm.com> wrote: > Hi all, > > In this PR we end up hitting a signed overflow in noce_get_alt_condition > when we try to > increment or decrement a HOST_WIDE_INT that might be HOST_WIDE_INT_MAX or > HOST_WIDE_INT_MIN. > > I've confirmed the overflow by adding an assert before the operation: > gcc_assert (desired_val != HOST_WIDE_INT_MAX); > > This patch fixes those cases by catching the cases when desired_val has the > extreme > value and avoids the transformation that function is trying to make. > > Bootstrapped and tested on arm, aarch64, x86_64. > > I've added the testcase that I used to trigger the assert mentioned above as > a compile test, > though I'm not sure how much value it has... > > Ok for trunk? If this isn't also a wrong-code issue (runtime testcase?) then why not perform the operation in unsigned HOST_WIDE_INT instead? Richard. > Thanks, > Kyrill > > 2016-05-23 Kyrylo Tkachov <kyrylo.tkachov@arm.com> > > PR rtl-optimization/66940 > * ifcvt.c (noce_get_alt_condition): Check that incrementing or > decrementing desired_val will not overflow before performing these > operations. > > 2016-05-23 Kyrylo Tkachov <kyrylo.tkachov@arm.com> > > PR rtl-optimization/66940 > * gcc.c-torture/compile/pr66940.c: New test.
On 23/05/16 12:27, Richard Biener wrote: > On Mon, May 23, 2016 at 1:17 PM, Kyrill Tkachov > <kyrylo.tkachov@foss.arm.com> wrote: >> Hi all, >> >> In this PR we end up hitting a signed overflow in noce_get_alt_condition >> when we try to >> increment or decrement a HOST_WIDE_INT that might be HOST_WIDE_INT_MAX or >> HOST_WIDE_INT_MIN. >> >> I've confirmed the overflow by adding an assert before the operation: >> gcc_assert (desired_val != HOST_WIDE_INT_MAX); >> >> This patch fixes those cases by catching the cases when desired_val has the >> extreme >> value and avoids the transformation that function is trying to make. >> >> Bootstrapped and tested on arm, aarch64, x86_64. >> >> I've added the testcase that I used to trigger the assert mentioned above as >> a compile test, >> though I'm not sure how much value it has... >> >> Ok for trunk? > If this isn't also a wrong-code issue (runtime testcase?) then why not perform > the operation in unsigned HOST_WIDE_INT instead? This part of the code transforms a comparison "x < CST" to "x <= CST - 1" and similar transformations. Fro what I understand the LT,LE,GT,GE RTL comparison operators operate on signed integers, so I'm not sure how valid it would be to do all this on unsigned HOST_WIDE_INT. Thanks, Kyrill > Richard. > >> Thanks, >> Kyrill >> >> 2016-05-23 Kyrylo Tkachov <kyrylo.tkachov@arm.com> >> >> PR rtl-optimization/66940 >> * ifcvt.c (noce_get_alt_condition): Check that incrementing or >> decrementing desired_val will not overflow before performing these >> operations. >> >> 2016-05-23 Kyrylo Tkachov <kyrylo.tkachov@arm.com> >> >> PR rtl-optimization/66940 >> * gcc.c-torture/compile/pr66940.c: New test.
n Mon, May 23, 2016 at 2:28 PM, Kyrill Tkachov <kyrylo.tkachov@foss.arm.com> wrote: > > On 23/05/16 12:27, Richard Biener wrote: >> >> On Mon, May 23, 2016 at 1:17 PM, Kyrill Tkachov >> <kyrylo.tkachov@foss.arm.com> wrote: >>> >>> Hi all, >>> >>> In this PR we end up hitting a signed overflow in noce_get_alt_condition >>> when we try to >>> increment or decrement a HOST_WIDE_INT that might be HOST_WIDE_INT_MAX or >>> HOST_WIDE_INT_MIN. >>> >>> I've confirmed the overflow by adding an assert before the operation: >>> gcc_assert (desired_val != HOST_WIDE_INT_MAX); >>> >>> This patch fixes those cases by catching the cases when desired_val has >>> the >>> extreme >>> value and avoids the transformation that function is trying to make. >>> >>> Bootstrapped and tested on arm, aarch64, x86_64. >>> >>> I've added the testcase that I used to trigger the assert mentioned above >>> as >>> a compile test, >>> though I'm not sure how much value it has... >>> >>> Ok for trunk? >> >> If this isn't also a wrong-code issue (runtime testcase?) then why not >> perform >> the operation in unsigned HOST_WIDE_INT instead? > > > This part of the code transforms a comparison "x < CST" to "x <= CST - 1" > and similar transformations. Fro what I understand the LT,LE,GT,GE RTL > comparison > operators operate on signed integers, so I'm not sure how valid it would be > to do all this on unsigned HOST_WIDE_INT. But then this is a wrong-code issue and you should see miscompiles and thus can add a dg-do run testcase instead? Richard. > Thanks, > Kyrill > > > >> Richard. >> >>> Thanks, >>> Kyrill >>> >>> 2016-05-23 Kyrylo Tkachov <kyrylo.tkachov@arm.com> >>> >>> PR rtl-optimization/66940 >>> * ifcvt.c (noce_get_alt_condition): Check that incrementing or >>> decrementing desired_val will not overflow before performing these >>> operations. >>> >>> 2016-05-23 Kyrylo Tkachov <kyrylo.tkachov@arm.com> >>> >>> PR rtl-optimization/66940 >>> * gcc.c-torture/compile/pr66940.c: New test. > >
On 23/05/16 13:46, Richard Biener wrote: > n Mon, May 23, 2016 at 2:28 PM, Kyrill Tkachov > <kyrylo.tkachov@foss.arm.com> wrote: >> On 23/05/16 12:27, Richard Biener wrote: >>> On Mon, May 23, 2016 at 1:17 PM, Kyrill Tkachov >>> <kyrylo.tkachov@foss.arm.com> wrote: >>>> Hi all, >>>> >>>> In this PR we end up hitting a signed overflow in noce_get_alt_condition >>>> when we try to >>>> increment or decrement a HOST_WIDE_INT that might be HOST_WIDE_INT_MAX or >>>> HOST_WIDE_INT_MIN. >>>> >>>> I've confirmed the overflow by adding an assert before the operation: >>>> gcc_assert (desired_val != HOST_WIDE_INT_MAX); >>>> >>>> This patch fixes those cases by catching the cases when desired_val has >>>> the >>>> extreme >>>> value and avoids the transformation that function is trying to make. >>>> >>>> Bootstrapped and tested on arm, aarch64, x86_64. >>>> >>>> I've added the testcase that I used to trigger the assert mentioned above >>>> as >>>> a compile test, >>>> though I'm not sure how much value it has... >>>> >>>> Ok for trunk? >>> If this isn't also a wrong-code issue (runtime testcase?) then why not >>> perform >>> the operation in unsigned HOST_WIDE_INT instead? >> >> This part of the code transforms a comparison "x < CST" to "x <= CST - 1" >> and similar transformations. Fro what I understand the LT,LE,GT,GE RTL >> comparison >> operators operate on signed integers, so I'm not sure how valid it would be >> to do all this on unsigned HOST_WIDE_INT. > But then this is a wrong-code issue and you should see miscompiles > and thus can add a dg-do run testcase instead? I couldn't get it to miscompile anything, because the check: "actual_val == desired_val + 1" where desired_val + 1 has signed overflow doesn't return true, so the transformation doesn't happen anyway. I think whether a miscompilation can occur depends on whether the compiler used to compile GCC itself does anything funky with the undefined behaviour that's occurring, which is why we should fix it. I suppose the testcase in this patch only goes so far to show that GCC doesn't crash, but not much else. I can make it an execute testcase if you'd like, but I can't get it to fail on my setup (the generated assembly looks correct on inspection). Kyrill Kyrill > Richard. > >> Thanks, >> Kyrill >> >> >> >>> Richard. >>> >>>> Thanks, >>>> Kyrill >>>> >>>> 2016-05-23 Kyrylo Tkachov <kyrylo.tkachov@arm.com> >>>> >>>> PR rtl-optimization/66940 >>>> * ifcvt.c (noce_get_alt_condition): Check that incrementing or >>>> decrementing desired_val will not overflow before performing these >>>> operations. >>>> >>>> 2016-05-23 Kyrylo Tkachov <kyrylo.tkachov@arm.com> >>>> >>>> PR rtl-optimization/66940 >>>> * gcc.c-torture/compile/pr66940.c: New test. >>
On Mon, May 23, 2016 at 3:19 PM, Kyrill Tkachov <kyrylo.tkachov@foss.arm.com> wrote: > > On 23/05/16 13:46, Richard Biener wrote: >> >> n Mon, May 23, 2016 at 2:28 PM, Kyrill Tkachov >> <kyrylo.tkachov@foss.arm.com> wrote: >>> >>> On 23/05/16 12:27, Richard Biener wrote: >>>> >>>> On Mon, May 23, 2016 at 1:17 PM, Kyrill Tkachov >>>> <kyrylo.tkachov@foss.arm.com> wrote: >>>>> >>>>> Hi all, >>>>> >>>>> In this PR we end up hitting a signed overflow in >>>>> noce_get_alt_condition >>>>> when we try to >>>>> increment or decrement a HOST_WIDE_INT that might be HOST_WIDE_INT_MAX >>>>> or >>>>> HOST_WIDE_INT_MIN. >>>>> >>>>> I've confirmed the overflow by adding an assert before the operation: >>>>> gcc_assert (desired_val != HOST_WIDE_INT_MAX); >>>>> >>>>> This patch fixes those cases by catching the cases when desired_val has >>>>> the >>>>> extreme >>>>> value and avoids the transformation that function is trying to make. >>>>> >>>>> Bootstrapped and tested on arm, aarch64, x86_64. >>>>> >>>>> I've added the testcase that I used to trigger the assert mentioned >>>>> above >>>>> as >>>>> a compile test, >>>>> though I'm not sure how much value it has... >>>>> >>>>> Ok for trunk? >>>> >>>> If this isn't also a wrong-code issue (runtime testcase?) then why not >>>> perform >>>> the operation in unsigned HOST_WIDE_INT instead? >>> >>> >>> This part of the code transforms a comparison "x < CST" to "x <= CST - 1" >>> and similar transformations. Fro what I understand the LT,LE,GT,GE RTL >>> comparison >>> operators operate on signed integers, so I'm not sure how valid it would >>> be >>> to do all this on unsigned HOST_WIDE_INT. >> >> But then this is a wrong-code issue and you should see miscompiles >> and thus can add a dg-do run testcase instead? > > > I couldn't get it to miscompile anything, because the check: > "actual_val == desired_val + 1" where desired_val + 1 has signed > overflow doesn't return true, so the transformation doesn't happen anyway. > I think whether a miscompilation can occur depends on whether the compiler > used > to compile GCC itself does anything funky with the undefined behaviour > that's > occurring, which is why we should fix it. I suppose the testcase in this > patch only > goes so far to show that GCC doesn't crash, but not much else. I can make it > an execute > testcase if you'd like, but I can't get it to fail on my setup (the > generated assembly > looks correct on inspection). Please make it a execute testcase anyway. Ok with that change. Thanks, Richard. > Kyrill > > > Kyrill > > >> Richard. >> >>> Thanks, >>> Kyrill >>> >>> >>> >>>> Richard. >>>> >>>>> Thanks, >>>>> Kyrill >>>>> >>>>> 2016-05-23 Kyrylo Tkachov <kyrylo.tkachov@arm.com> >>>>> >>>>> PR rtl-optimization/66940 >>>>> * ifcvt.c (noce_get_alt_condition): Check that incrementing or >>>>> decrementing desired_val will not overflow before performing >>>>> these >>>>> operations. >>>>> >>>>> 2016-05-23 Kyrylo Tkachov <kyrylo.tkachov@arm.com> >>>>> >>>>> PR rtl-optimization/66940 >>>>> * gcc.c-torture/compile/pr66940.c: New test. >>> >>> >
On 05/23/2016 01:17 PM, Kyrill Tkachov wrote: > In this PR we end up hitting a signed overflow in noce_get_alt_condition > when we try to > increment or decrement a HOST_WIDE_INT that might be HOST_WIDE_INT_MAX > or HOST_WIDE_INT_MIN. > > I've confirmed the overflow by adding an assert before the operation: > gcc_assert (desired_val != HOST_WIDE_INT_MAX); Don't we have to check for overflow in whatever mode the comparison is in, rather than using HOST_WIDE_INT? I expect the compile test doesn't actually test anything without some sort of sanitizer enabled for the compiler? If this results in a miscompilation, can you construct an executable test? Bernd
On 02/06/16 10:08, Bernd Schmidt wrote: > On 05/23/2016 01:17 PM, Kyrill Tkachov wrote: >> In this PR we end up hitting a signed overflow in noce_get_alt_condition >> when we try to >> increment or decrement a HOST_WIDE_INT that might be HOST_WIDE_INT_MAX >> or HOST_WIDE_INT_MIN. >> >> I've confirmed the overflow by adding an assert before the operation: >> gcc_assert (desired_val != HOST_WIDE_INT_MAX); > > Don't we have to check for overflow in whatever mode the comparison is in, rather than using HOST_WIDE_INT? > The overflow happens because no the next line the code checks for: actual_val == desired_val + 1 so if desired_val is HOST_WIDE_INT_MAX the "+ 1" overflows it. I don't think the mode here is relevant as we're trying to avoid undefined behaviour in the compiler itself. > I expect the compile test doesn't actually test anything without some sort of sanitizer enabled for the compiler? If this results in a miscompilation, can you construct an executable test? > As described in https://gcc.gnu.org/ml/gcc-patches/2016-05/msg01790.html, I couldn't get it to miscompile anything. The bug was reported through a run instrumented with sanitisation. I agree the testcase is not very useful as it stands, at best it only checks that we don't ICE (which we didn't anyway) but since the problem here is undefined behaviour the effects of the bug depend on the compiler used to compile GCC itself. Thanks, Kyrill > Bernd
diff --git a/gcc/ifcvt.c b/gcc/ifcvt.c index 80af4a84363192879cc49ea45f777fc987fda555..05fac71409d401a08d01b7dc7cf164613f8477c4 100644 --- a/gcc/ifcvt.c +++ b/gcc/ifcvt.c @@ -2396,28 +2396,32 @@ noce_get_alt_condition (struct noce_if_info *if_info, rtx target, switch (code) { case LT: - if (actual_val == desired_val + 1) + if (desired_val != HOST_WIDE_INT_MAX + && actual_val == desired_val + 1) { code = LE; op_b = GEN_INT (desired_val); } break; case LE: - if (actual_val == desired_val - 1) + if (desired_val != HOST_WIDE_INT_MIN + && actual_val == desired_val - 1) { code = LT; op_b = GEN_INT (desired_val); } break; case GT: - if (actual_val == desired_val - 1) + if (desired_val != HOST_WIDE_INT_MIN + && actual_val == desired_val - 1) { code = GE; op_b = GEN_INT (desired_val); } break; case GE: - if (actual_val == desired_val + 1) + if (desired_val != HOST_WIDE_INT_MAX + && actual_val == desired_val + 1) { code = GT; op_b = GEN_INT (desired_val); diff --git a/gcc/testsuite/gcc.c-torture/compile/pr66940.c b/gcc/testsuite/gcc.c-torture/compile/pr66940.c new file mode 100644 index 0000000000000000000000000000000000000000..1f3586b49f4389b4a506774cf550a984073f03e6 --- /dev/null +++ b/gcc/testsuite/gcc.c-torture/compile/pr66940.c @@ -0,0 +1,8 @@ +long +foo (int cp, long ival) +{ + if (ival <= 0) + return -0x7fffffffffffffffL - 1; + + return 0x7fffffffffffffffL; +}
Hi all, In this PR we end up hitting a signed overflow in noce_get_alt_condition when we try to increment or decrement a HOST_WIDE_INT that might be HOST_WIDE_INT_MAX or HOST_WIDE_INT_MIN. I've confirmed the overflow by adding an assert before the operation: gcc_assert (desired_val != HOST_WIDE_INT_MAX); This patch fixes those cases by catching the cases when desired_val has the extreme value and avoids the transformation that function is trying to make. Bootstrapped and tested on arm, aarch64, x86_64. I've added the testcase that I used to trigger the assert mentioned above as a compile test, though I'm not sure how much value it has... Ok for trunk? Thanks, Kyrill 2016-05-23 Kyrylo Tkachov <kyrylo.tkachov@arm.com> PR rtl-optimization/66940 * ifcvt.c (noce_get_alt_condition): Check that incrementing or decrementing desired_val will not overflow before performing these operations. 2016-05-23 Kyrylo Tkachov <kyrylo.tkachov@arm.com> PR rtl-optimization/66940 * gcc.c-torture/compile/pr66940.c: New test.