This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH][RTL ifcvt] PR rtl-optimization/66940: Avoid signed overflow in noce_get_alt_condition
- From: Richard Biener <richard dot guenther at gmail dot com>
- To: Kyrill Tkachov <kyrylo dot tkachov at foss dot arm dot com>
- Cc: GCC Patches <gcc-patches at gcc dot gnu dot org>
- Date: Mon, 23 May 2016 16:06:20 +0200
- Subject: Re: [PATCH][RTL ifcvt] PR rtl-optimization/66940: Avoid signed overflow in noce_get_alt_condition
- Authentication-results: sourceware.org; auth=none
- References: <5742E6B2 dot 7050904 at foss dot arm dot com> <CAFiYyc1MF4_j4Rn=5bUbiQxjh8sFe8n30uY-6SyhOb4kh_d7wA at mail dot gmail dot com> <5742F75B dot 30802 at foss dot arm dot com> <CAFiYyc1aEeZod7mzqh+fN-h9i=BB4+bvgCPx=pJGNn9BppDiCA at mail dot gmail dot com> <5743034E dot 1070208 at foss dot arm dot com>
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.
>>>
>>>
>