Patchwork Fix PR middle-end/56474

login
register
mail settings
Submitter Eric Botcazou
Date April 14, 2013, 8:05 a.m.
Message ID <1630485.0zg5rbKfGZ@polaris>
Download mbox | patch
Permalink /patch/236412/
State New
Headers show

Comments

Eric Botcazou - April 14, 2013, 8:05 a.m.
Hi,

this is a regression present on the mainline and 4.8 branch and introduced by 
the latest series of sizetype changes.  Associated adjustments were made in 
the various front-ends for it, most notably Ada which was the most affected, 
but this issue slipped through the cracks in the form of a bogus overflow 
detection for 0-based arrays with variable upper bound included in a record 
with discriminant.

The proposed fix is to disable overflow detection in sizetype for one special 
case (0 - 1) in size_binop_loc.  An equivalent kludge was added to layout_type 
to disable overflow detection for the size expression of [0, -1] arrays.

Tested on x86_64-suse-linux, OK for the mainline and 4.8 branch?


2013-04-14  Eric Botcazou  <ebotcazou@adacore.com>

	PR middle-end/56474
	* fold-const.c (size_binop_loc): Disable overflow detection for 0 - 1.


2013-04-14  Eric Botcazou  <ebotcazou@adacore.com>

	* gnat.dg/specs/array3.ads: New test.
Richard Guenther - April 15, 2013, 9:50 a.m.
On Sun, Apr 14, 2013 at 10:05 AM, Eric Botcazou <ebotcazou@adacore.com> wrote:
> Hi,
>
> this is a regression present on the mainline and 4.8 branch and introduced by
> the latest series of sizetype changes.  Associated adjustments were made in
> the various front-ends for it, most notably Ada which was the most affected,
> but this issue slipped through the cracks in the form of a bogus overflow
> detection for 0-based arrays with variable upper bound included in a record
> with discriminant.
>
> The proposed fix is to disable overflow detection in sizetype for one special
> case (0 - 1) in size_binop_loc.  An equivalent kludge was added to layout_type
> to disable overflow detection for the size expression of [0, -1] arrays.
>
> Tested on x86_64-suse-linux, OK for the mainline and 4.8 branch?

I think I already rejected this and asked you to fix the users (like
layout_type is a user).  Clearly 0 - 1 in unsigned arithmetic overflows.
Not indicating this may cause bugs elsewhere as easily as it fixes
code not dealing with this fact.

Richard.

>
> 2013-04-14  Eric Botcazou  <ebotcazou@adacore.com>
>
>         PR middle-end/56474
>         * fold-const.c (size_binop_loc): Disable overflow detection for 0 - 1.
>
>
> 2013-04-14  Eric Botcazou  <ebotcazou@adacore.com>
>
>         * gnat.dg/specs/array3.ads: New test.
>
>
> --
> Eric Botcazou
Eric Botcazou - April 15, 2013, 10:04 a.m.
> I think I already rejected this and asked you to fix the users (like
> layout_type is a user).

Yes, but that would be a pain, there are too many users in the Ada front-end.

> Clearly 0 - 1 in unsigned arithmetic overflows.  Not indicating this may
> cause bugs elsewhere as easily as it fixes code not dealing with this fact.

!?? There is no overflow in unsigned arithmetics.  Instead size_binop forces 
overflows artificially and I don't see the problem in deciding that 0 - 1 is a 
special case, like [0, -1] is a special case for layout_type.  And note that 
this was the historical behavior, before the latest sizetype changes.
Richard Guenther - April 15, 2013, 12:26 p.m.
On Mon, Apr 15, 2013 at 12:04 PM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> I think I already rejected this and asked you to fix the users (like
>> layout_type is a user).
>
> Yes, but that would be a pain, there are too many users in the Ada front-end.

Users that care about the special casing of  0 - 1 and overflow detection?
For the C family I found exactly one - the layout_type case, and fixed
it in the FEs by making empty arrays use [1, 0] domains or signed domains
(I don't remember exactly).  I believe the layout_type change was to make
Ada happy.

>> Clearly 0 - 1 in unsigned arithmetic overflows.  Not indicating this may
>> cause bugs elsewhere as easily as it fixes code not dealing with this fact.
>
> !?? There is no overflow in unsigned arithmetics.  Instead size_binop forces
> overflows artificially and I don't see the problem in deciding that 0 - 1 is a
> special case, like [0, -1] is a special case for layout_type.  And note that
> this was the historical behavior, before the latest sizetype changes.

Historically sizetype didn't overflow because it was supposed to never overflow.
Well, unless we check for overflow.

It may be that enabling overflow detection for even unsigned sizetype was
because of Ada as well.  After all only Ada changed its sizetype sign recently.

I don't like special casing 0 - 1 in a general compute function.  Maybe
you want to use size_diffop for the computation?  That would result in
a signed result and thus no overflow for 0 - 1.

The other option is to for example disable overflow handling for _all_
constants and MINUS_EXPR (and then please PLUS_EXPR as well)
in size_binop.  Maybe it's only the MULT_EXPR overflow we want to
know (byte-to-bit conversion / element size scaling IIRC).

Richard.

> --
> Eric Botcazou
Eric Botcazou - April 16, 2013, 11:12 p.m.
> For the C family I found exactly one - the layout_type case, and fixed
> it in the FEs by making empty arrays use [1, 0] domains or signed domains
> (I don't remember exactly).  I believe the layout_type change was to make
> Ada happy.

I'm skeptical, I had to narrow down the initial kludge because it hurted Ada.

> It may be that enabling overflow detection for even unsigned sizetype was
> because of Ada as well.  After all only Ada changed its sizetype sign
> recently.

Not true, overflow detection has _always_ been enabled for sizetypes.
But sizetypes, including unsigned ones, were sign-extended so 0 -1 didn't 
overflow and we need that behavior back for Ada to work properly, 

> I don't like special casing 0 - 1 in a general compute function.  Maybe
> you want to use size_diffop for the computation?  That would result in
> a signed result and thus no overflow for 0 - 1.

But it's not a general compute function, it's size_binop which is meant to be 
used for sizetypes only and which forces overflow on unsigned types.  We need 
overflow detection for sizetypes but we can also tailor it to fit our needs.

> The other option is to for example disable overflow handling for _all_
> constants and MINUS_EXPR (and then please PLUS_EXPR as well)
> in size_binop.  Maybe it's only the MULT_EXPR overflow we want to
> know (byte-to-bit conversion / element size scaling IIRC).

Well, we just need 0 - 1 because of the way we compute size expressions for 
variable-sized arrays.
Richard Guenther - April 17, 2013, 8:11 a.m.
On Wed, Apr 17, 2013 at 1:12 AM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> For the C family I found exactly one - the layout_type case, and fixed
>> it in the FEs by making empty arrays use [1, 0] domains or signed domains
>> (I don't remember exactly).  I believe the layout_type change was to make
>> Ada happy.
>
> I'm skeptical, I had to narrow down the initial kludge because it hurted Ada.
>
>> It may be that enabling overflow detection for even unsigned sizetype was
>> because of Ada as well.  After all only Ada changed its sizetype sign
>> recently.
>
> Not true, overflow detection has _always_ been enabled for sizetypes.
> But sizetypes, including unsigned ones, were sign-extended so 0 -1 didn't
> overflow and we need that behavior back for Ada to work properly,

Yeah, well - they were effectively signed.

>> I don't like special casing 0 - 1 in a general compute function.  Maybe
>> you want to use size_diffop for the computation?  That would result in
>> a signed result and thus no overflow for 0 - 1.
>
> But it's not a general compute function, it's size_binop which is meant to be
> used for sizetypes only and which forces overflow on unsigned types.  We need
> overflow detection for sizetypes but we can also tailor it to fit our needs.

I'm not against tailoring it to fit our needs - I'm just against special casing
behavior for specific values.  That just sounds wrong.

Maybe we should detect overflow as if the input and output were signed
while computing an unsigned result.  As far as I can see int_const_binop_1
does detect overflow as if operations were signed (it passes 'false' as
uns to all double-int operations rather than TYPE_UNSIGNED).
For example sub_with_overflow simply does

  neg_double (b.low, b.high, &ret.low, &ret.high);
  add_double (low, high, ret.low, ret.high, &ret.low, &ret.high);
  *overflow = OVERFLOW_SUM_SIGN (ret.high, b.high, high);

which I believe is wrong.  Shouldn't it be

  neg_double (b.low, b.high, &ret.low, &ret.high);
  HOST_WIDE_INT tem = ret.high;
  add_double (low, high, ret.low, ret.high, &ret.low, &ret.high);
  *overflow = OVERFLOW_SUM_SIGN (ret.high, tem, high);

?  Because we are computing a + (-b) and thus OVERFLOW_SUM_SIGN
expects the sign of a and -b, not a and b to verify against the
sign of ret.

>> The other option is to for example disable overflow handling for _all_
>> constants and MINUS_EXPR (and then please PLUS_EXPR as well)
>> in size_binop.  Maybe it's only the MULT_EXPR overflow we want to
>> know (byte-to-bit conversion / element size scaling IIRC).
>
> Well, we just need 0 - 1 because of the way we compute size expressions for
> variable-sized arrays.

I'm sceptical.  Where do you compute the size expression for variable-sized
arrays?  I suppose with the testcase in the initial patch I can then inspect
myself what actually happens?

Thanks,
Richard.

> --
> Eric Botcazou
Eric Botcazou - April 19, 2013, 9:01 p.m.
> Maybe we should detect overflow as if the input and output were signed
> while computing an unsigned result.  As far as I can see int_const_binop_1
> does detect overflow as if operations were signed (it passes 'false' as
> uns to all double-int operations rather than TYPE_UNSIGNED).
> For example sub_with_overflow simply does
> 
>   neg_double (b.low, b.high, &ret.low, &ret.high);
>   add_double (low, high, ret.low, ret.high, &ret.low, &ret.high);
>   *overflow = OVERFLOW_SUM_SIGN (ret.high, b.high, high);
> 
> which I believe is wrong.  Shouldn't it be
> 
>   neg_double (b.low, b.high, &ret.low, &ret.high);
>   HOST_WIDE_INT tem = ret.high;
>   add_double (low, high, ret.low, ret.high, &ret.low, &ret.high);
>   *overflow = OVERFLOW_SUM_SIGN (ret.high, tem, high);
> 
> ?  Because we are computing a + (-b) and thus OVERFLOW_SUM_SIGN
> expects the sign of a and -b, not a and b to verify against the
> sign of ret.

But int_const_binop_1 is called from int_const_binop, so why would we want to 
introduce any overflow for unsigned types other than sizetypes?

> I'm sceptical.  Where do you compute the size expression for variable-sized
> arrays?  I suppose with the testcase in the initial patch I can then inspect
> myself what actually happens?

Sure, but we already went through this in the PR.  It's because of the formula 
used for the length of variable-sized arrays, which needs to handle the case 
of superflat arrays.
Richard Guenther - April 22, 2013, 1:35 p.m.
On Fri, Apr 19, 2013 at 11:01 PM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> Maybe we should detect overflow as if the input and output were signed
>> while computing an unsigned result.  As far as I can see int_const_binop_1
>> does detect overflow as if operations were signed (it passes 'false' as
>> uns to all double-int operations rather than TYPE_UNSIGNED).
>> For example sub_with_overflow simply does
>>
>>   neg_double (b.low, b.high, &ret.low, &ret.high);
>>   add_double (low, high, ret.low, ret.high, &ret.low, &ret.high);
>>   *overflow = OVERFLOW_SUM_SIGN (ret.high, b.high, high);
>>
>> which I believe is wrong.  Shouldn't it be
>>
>>   neg_double (b.low, b.high, &ret.low, &ret.high);
>>   HOST_WIDE_INT tem = ret.high;
>>   add_double (low, high, ret.low, ret.high, &ret.low, &ret.high);
>>   *overflow = OVERFLOW_SUM_SIGN (ret.high, tem, high);
>>
>> ?  Because we are computing a + (-b) and thus OVERFLOW_SUM_SIGN
>> expects the sign of a and -b, not a and b to verify against the
>> sign of ret.
>
> But int_const_binop_1 is called from int_const_binop, so why would we want to
> introduce any overflow for unsigned types other than sizetypes?
>
>> I'm sceptical.  Where do you compute the size expression for variable-sized
>> arrays?  I suppose with the testcase in the initial patch I can then inspect
>> myself what actually happens?
>
> Sure, but we already went through this in the PR.  It's because of the formula
> used for the length of variable-sized arrays, which needs to handle the case
> of superflat arrays.

Ah, indeed.  I added a comment there.

Richard.

> --
> Eric Botcazou

Patch

Index: fold-const.c
===================================================================
--- fold-const.c	(revision 197926)
+++ fold-const.c	(working copy)
@@ -1422,9 +1422,13 @@  size_binop_loc (location_t loc, enum tre
   gcc_assert (int_binop_types_match_p (code, TREE_TYPE (arg0),
                                        TREE_TYPE (arg1)));
 
-  /* Handle the special case of two integer constants faster.  */
+  /* Handle general case of two integer constants.  For sizetype constant
+     calculations, we always want to know about overflow, even in the
+     unsigned case.  */
   if (TREE_CODE (arg0) == INTEGER_CST && TREE_CODE (arg1) == INTEGER_CST)
     {
+      int overflowable = -1;
+
       /* And some specific cases even faster than that.  */
       if (code == PLUS_EXPR)
 	{
@@ -1437,6 +1441,11 @@  size_binop_loc (location_t loc, enum tre
 	{
 	  if (integer_zerop (arg1) && !TREE_OVERFLOW (arg1))
 	    return arg0;
+
+	  /* ??? We make an exception for 0 - 1 because it's an idiom
+	     used in length calculations for zero-based arrays.  */
+	  if (integer_zerop (arg0) && integer_onep (arg1))
+	    overflowable = 1;
 	}
       else if (code == MULT_EXPR)
 	{
@@ -1444,10 +1453,7 @@  size_binop_loc (location_t loc, enum tre
 	    return arg1;
 	}
 
-      /* Handle general case of two integer constants.  For sizetype
-         constant calculations we always want to know about overflow,
-	 even in the unsigned case.  */
-      return int_const_binop_1 (code, arg0, arg1, -1);
+      return int_const_binop_1 (code, arg0, arg1, overflowable);
     }
 
   return fold_build2_loc (loc, code, type, arg0, arg1);