diff mbox

[RTL,ifcvt] PR rtl-optimization/66940: Avoid signed overflow in noce_get_alt_condition

Message ID 5745C8CD.8080707@foss.arm.com
State New
Headers show

Commit Message

Kyrill Tkachov May 25, 2016, 3:46 p.m. UTC 
On 23/05/16 15:06, Richard Biener wrote:
> 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, here it is with the testcase made executable.

Committing to trunk.

Kyrill

2016-05-25  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-25  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>

     PR rtl-optimization/66940
     * gcc.c-torture/execute/pr66940.c: New test.

> 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.
>>>>
diff mbox

Patch

diff --git a/gcc/ifcvt.c b/gcc/ifcvt.c
index 6ce51ba70464ce8ec79e217d290c2ab14f317651..cab054889aea7fafab7b5310da45ef7b3fc7e50b 100644
--- a/gcc/ifcvt.c
+++ b/gcc/ifcvt.c
@@ -2416,28 +2416,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/execute/pr66940.c b/gcc/testsuite/gcc.c-torture/execute/pr66940.c
new file mode 100644
index 0000000000000000000000000000000000000000..fbd109dccc72c290d935d81a3f3dd219f11f908d
--- /dev/null
+++ b/gcc/testsuite/gcc.c-torture/execute/pr66940.c
@@ -0,0 +1,20 @@ 
+long long __attribute__ ((noinline, noclone))
+foo (long long ival)
+{
+ if (ival <= 0)
+    return -0x7fffffffffffffffL - 1;
+
+ return 0x7fffffffffffffffL;
+}
+
+int
+main (void)
+{
+  if (foo (-1) != (-0x7fffffffffffffffL - 1))
+    __builtin_abort ();
+
+  if (foo (1) != 0x7fffffffffffffffL)
+    __builtin_abort ();
+
+  return 0;
+}