[Bug ada/56474] New: GNAT computes size of the object to be allocated incorrectly

[Bug ada/56474] New: GNAT computes size of the object to be allocated incorrectly

[pavel at zhukoff dot net]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56474

             Bug #: 56474
           Summary: GNAT computes size of the object to be allocated
                    incorrectly
    Classification: Unclassified
           Product: gcc
           Version: 4.8.0
            Status: UNCONFIRMED
          Severity: critical
          Priority: P3
         Component: ada
        AssignedTo: unassigned@gcc.gnu.org
        ReportedBy: pavel@zhukoff.net


100% reproducible

Smaller reproducer:

with Ada.Streams;

package Pkg is
   use type Ada.Streams.Stream_Element_Offset;

   type Vector (Size : Ada.Streams.Stream_Element_Offset) is record
      Value : Ada.Streams.Stream_Element_Array (0 .. Size);
   end record;

   Empty_Vector : Vector (-1);

end Pkg;

with Pkg;
procedure Bbb is
begin
   null;
end Bbb;

$ gnatmake bbb
gcc -c bbb.adb
gcc -c pkg.ads
pkg.ads:10:04: warning: "Storage_Error" will be raised at run time
gnatbind -x bbb.ali
gnatlink bbb.ali

$ ./bbb

raised STORAGE_ERROR : object too large

[Bug ada/56474] [4.8 regression] bogus Storage_Error raised for record containing empty zero-based array

[ebotcazou at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56474

Eric Botcazou <ebotcazou at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2013-02-28
                 CC|                            |ebotcazou at gcc dot
                   |                            |gnu.org
   Target Milestone|---                         |4.8.0
            Summary|GNAT computes size of the   |[4.8 regression] bogus
                   |object to be allocated      |Storage_Error raised for
                   |incorrectly                 |record containing empty
                   |                            |zero-based array
     Ever Confirmed|0                           |1
           Severity|critical                    |normal

--- Comment #1 from Eric Botcazou <ebotcazou at gcc dot gnu.org> 2013-02-28 11:01:37 UTC ---
Strange, we would rather lose warnings than issue bogus ones here.

In any case, the workaround is easy: use 1-based arrays instead.

[Bug ada/56474] [4.8 regression] bogus Storage_Error raised for record containing empty zero-based array

[ebotcazou at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56474

Eric Botcazou <ebotcazou at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|ebotcazou at gcc dot        |
                   |gnu.org                     |
         AssignedTo|unassigned at gcc dot       |ebotcazou at gcc dot
                   |gnu.org                     |gnu.org

--- Comment #2 from Eric Botcazou <ebotcazou at gcc dot gnu.org> 2013-02-28 11:03:22 UTC ---
Investigating.

[Bug ada/56474] [4.8 regression] bogus Storage_Error raised for record containing empty zero-based array

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56474

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |rguenth at gcc dot gnu.org

--- Comment #3 from Richard Biener <rguenth at gcc dot gnu.org> 2013-03-01 11:26:59 UTC ---
Probably caused by my sizetype changes.  For frontends that support arbitrary
array domains the only good reliable way out is to support and use both
signed and unsigned TYPE_DOMAINs.

[Bug ada/56474] [4.8 regression] bogus Storage_Error raised for record containing empty zero-based array

[ebotcazou at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56474

--- Comment #4 from Eric Botcazou <ebotcazou at gcc dot gnu.org> 2013-03-01 14:25:56 UTC ---
This happens because 0 - 1 overflows in sizetype:

#6  0x0000000000ac3e3d in size_binop_loc (loc=0, code=MINUS_EXPR, 
    arg0=0x7ffff6d65d60, arg1=0x7ffff6d65f00)
    at /home/eric/svn/gcc/gcc/fold-const.c:1417
1417          return int_const_binop_1 (code, arg0, arg1, -1);
(gdb) p debug_tree(arg0)
 <integer_cst 0x7ffff6d65d60 type <integer_type 0x7ffff6d710a8 sizetype>
constant visited 0>
$28 = void
(gdb) p debug_tree(arg1)
 <integer_cst 0x7ffff6d65f00 type <integer_type 0x7ffff6d710a8 sizetype>
constant visited 1>
$29 = void
(gdb) frame 4
#4  0x0000000000f28804 in force_fit_type_double (type=0x7ffff6d710a8, cst=..., 
    overflowable=-1, overflowed=false) at /home/eric/svn/gcc/gcc/tree.c:1109
1109              tree t = make_node (INTEGER_CST);
(gdb) p cst
$30 = {low = 18446744073709551615, high = -1}

Now it didn't overflow on the 4.7 branch:

Breakpoint 2, int_const_binop (code=MINUS_EXPR, arg1=0x7ffff6d66f00, 
    arg2=0x7ffff6d790a0) at /home/eric/svn/gcc-4_7-branch/gcc/fold-const.c:1085
1085      return t;
(gdb) p debug_tree(arg1)
 <integer_cst 0x7ffff6d66f00 type <integer_type 0x7ffff6d77000 sizetype>
constant visited 0>
$4 = void
(gdb) p debug_tree(arg2)
 <integer_cst 0x7ffff6d790a0 type <integer_type 0x7ffff6d77000 sizetype>
constant visited 1>
$5 = void
(gdb) p debug_tree(t)
 <integer_cst 0x7ffff6d66f20 type <integer_type 0x7ffff6d77000 sizetype>
constant visited -1>

This low_bound - 1 idiom is pervasive in Ada and it will probably overflow only
for 0 so we could add a kludge to size_binop_loc, similarly to the kludge that
exists in layout_type:

Index: fold-const.c
===================================================================
--- fold-const.c        (revision 196253)
+++ fold-const.c        (working copy)
@@ -1389,9 +1389,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)
        {
@@ -1404,6 +1408,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)
        {
@@ -1411,10 +1420,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);


What do you think Richard?

[Bug middle-end/56474] [4.8 regression] bogus Storage_Error raised for record containing empty zero-based array

[ebotcazou at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56474

Eric Botcazou <ebotcazou at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
          Component|ada                         |middle-end

--- Comment #5 from Eric Botcazou <ebotcazou at gcc dot gnu.org> 2013-03-01 14:32:50 UTC ---
Recategorizing.

[Bug middle-end/56474] [4.8 regression] bogus Storage_Error raised for record containing empty zero-based array

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56474

--- Comment #6 from Richard Biener <rguenth at gcc dot gnu.org> 2013-03-04 09:53:39 UTC ---
(In reply to comment #4)
> This happens because 0 - 1 overflows in sizetype:
> 
> #6  0x0000000000ac3e3d in size_binop_loc (loc=0, code=MINUS_EXPR, 
>     arg0=0x7ffff6d65d60, arg1=0x7ffff6d65f00)
>     at /home/eric/svn/gcc/gcc/fold-const.c:1417
> 1417          return int_const_binop_1 (code, arg0, arg1, -1);
> (gdb) p debug_tree(arg0)
>  <integer_cst 0x7ffff6d65d60 type <integer_type 0x7ffff6d710a8 sizetype>
> constant visited 0>
> $28 = void
> (gdb) p debug_tree(arg1)
>  <integer_cst 0x7ffff6d65f00 type <integer_type 0x7ffff6d710a8 sizetype>
> constant visited 1>
> $29 = void
> (gdb) frame 4
> #4  0x0000000000f28804 in force_fit_type_double (type=0x7ffff6d710a8, cst=..., 
>     overflowable=-1, overflowed=false) at /home/eric/svn/gcc/gcc/tree.c:1109
> 1109              tree t = make_node (INTEGER_CST);
> (gdb) p cst
> $30 = {low = 18446744073709551615, high = -1}
> 
> Now it didn't overflow on the 4.7 branch:
> 
> Breakpoint 2, int_const_binop (code=MINUS_EXPR, arg1=0x7ffff6d66f00, 
>     arg2=0x7ffff6d790a0) at /home/eric/svn/gcc-4_7-branch/gcc/fold-const.c:1085
> 1085      return t;
> (gdb) p debug_tree(arg1)
>  <integer_cst 0x7ffff6d66f00 type <integer_type 0x7ffff6d77000 sizetype>
> constant visited 0>
> $4 = void
> (gdb) p debug_tree(arg2)
>  <integer_cst 0x7ffff6d790a0 type <integer_type 0x7ffff6d77000 sizetype>
> constant visited 1>
> $5 = void
> (gdb) p debug_tree(t)
>  <integer_cst 0x7ffff6d66f20 type <integer_type 0x7ffff6d77000 sizetype>
> constant visited -1>
> 
> This low_bound - 1 idiom is pervasive in Ada and it will probably overflow only
> for 0 so we could add a kludge to size_binop_loc, similarly to the kludge that
> exists in layout_type:
> 
> Index: fold-const.c
> ===================================================================
> --- fold-const.c        (revision 196253)
> +++ fold-const.c        (working copy)
> @@ -1389,9 +1389,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)
>         {
> @@ -1404,6 +1408,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)
>         {
> @@ -1411,10 +1420,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);
> 
> 
> What do you think Richard?

What will the result be used for in this case?  The result, usizetype_max,
is certainly not 0 - 1 == -1 as it is unsigned.

Adding kludges like that might work, but I'd rather try to fix callers
to "ask more intelligent questions".  That is,

+         /* ??? 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;

the length of an array is max-index - min-index + 1.  What's the call
stack of this testcases case triggering the overflow?

[Bug middle-end/56474] [4.8 regression] bogus Storage_Error raised for record containing empty zero-based array

[ebotcazou at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56474

--- Comment #7 from Eric Botcazou <ebotcazou at gcc dot gnu.org> 2013-03-04 11:29:16 UTC ---
> What will the result be used for in this case?  The result, usizetype_max,
> is certainly not 0 - 1 == -1 as it is unsigned.

It's used for the upper bound of variable-sized arrays:

          /* Finally we use (hb >= lb) ? hb : lb - 1 for the upper bound
         in all the other cases.  Note that, here as well as above,
         the condition used in the comparison must be equivalent to
         the condition (length != 0).  This is relied upon in order
         to optimize array comparisons in compare_arrays.  */
          else
        gnu_high
          = build_cond_expr (sizetype,
                     build_binary_op (GE_EXPR,
                              boolean_type_node,
                              gnu_orig_max,
                              gnu_orig_min),
                     gnu_max,
                     size_binop (MINUS_EXPR, gnu_min,
                         size_one_node));

The result wraps around but that's fine; we just don't want the overflow.

> Adding kludges like that might work, but I'd rather try to fix callers
> to "ask more intelligent questions".  That is,
> 
> +         /* ??? 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;
> 
> the length of an array is max-index - min-index + 1.  What's the call
> stack of this testcases case triggering the overflow?

Yes, the formula for the upper bound is designed to yield a length of zero if
the upper bound is lower than the lower bound.

size_binop is called from ada/gcc-interface/decl.c:gnat_to_gnu_entity.

[Bug middle-end/56474] [4.8 regression] bogus Storage_Error raised for record containing empty zero-based array

[rguenther at suse dot de]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56474

--- Comment #8 from rguenther at suse dot de <rguenther at suse dot de> 2013-03-04 15:12:25 UTC ---
On Mon, 4 Mar 2013, ebotcazou at gcc dot gnu.org wrote:

> 
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56474
> 
> --- Comment #7 from Eric Botcazou <ebotcazou at gcc dot gnu.org> 2013-03-04 11:29:16 UTC ---
> > What will the result be used for in this case?  The result, usizetype_max,
> > is certainly not 0 - 1 == -1 as it is unsigned.
> 
> It's used for the upper bound of variable-sized arrays:
> 
>           /* Finally we use (hb >= lb) ? hb : lb - 1 for the upper bound
>          in all the other cases.  Note that, here as well as above,
>          the condition used in the comparison must be equivalent to
>          the condition (length != 0).  This is relied upon in order
>          to optimize array comparisons in compare_arrays.  */
>           else
>         gnu_high
>           = build_cond_expr (sizetype,
>                      build_binary_op (GE_EXPR,
>                               boolean_type_node,
>                               gnu_orig_max,
>                               gnu_orig_min),
>                      gnu_max,
>                      size_binop (MINUS_EXPR, gnu_min,
>                          size_one_node));
> 
> The result wraps around but that's fine; we just don't want the overflow.

Hm, if hb < lb - what kind of pair do you expect?  The only case
where lb - 1 is the "upper bound" (whatever upper bound is for an
empty array ...) - isn't lb - 1 always equal to hb in that case,
thus are not hb and lb one element apart for empty arrays (length == 0)?

So just do ...

  gnu_high = gnu_orig_max;

?

Maybe I'm missign something ...

Richard.

[Bug middle-end/56474] [4.8 regression] bogus Storage_Error raised for record containing empty zero-based array

[ebotcazou at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56474

--- Comment #9 from Eric Botcazou <ebotcazou at gcc dot gnu.org> 2013-03-04 15:38:53 UTC ---
> Hm, if hb < lb - what kind of pair do you expect?  The only case
> where lb - 1 is the "upper bound" (whatever upper bound is for an
> empty array ...) - isn't lb - 1 always equal to hb in that case,
> thus are not hb and lb one element apart for empty arrays (length == 0)?

No, you can have "superflat" arrays and they need to have zero length (instead
of a negative one) in Ada, hence the formula.

[Bug middle-end/56474] [4.8 regression] bogus Storage_Error raised for record containing empty zero-based array

[rguenther at suse dot de]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56474

--- Comment #10 from rguenther at suse dot de <rguenther at suse dot de> 2013-03-04 15:45:09 UTC ---
On Mon, 4 Mar 2013, ebotcazou at gcc dot gnu.org wrote:

> 
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56474
> 
> --- Comment #9 from Eric Botcazou <ebotcazou at gcc dot gnu.org> 2013-03-04 15:38:53 UTC ---
> > Hm, if hb < lb - what kind of pair do you expect?  The only case
> > where lb - 1 is the "upper bound" (whatever upper bound is for an
> > empty array ...) - isn't lb - 1 always equal to hb in that case,
> > thus are not hb and lb one element apart for empty arrays (length == 0)?
> 
> No, you can have "superflat" arrays and they need to have zero length (instead
> of a negative one) in Ada, hence the formula.

Btw, it's easy to not get the overflow for this formula by using
fold_build2 instead of size_binop.

[Bug middle-end/56474] [4.8 regression] bogus Storage_Error raised for record containing empty zero-based array

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56474

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jakub at gcc dot gnu.org

--- Comment #11 from Jakub Jelinek <jakub at gcc dot gnu.org> 2013-03-04 15:49:35 UTC ---
So, can the Ada FE be changed to use that?  Hacks like this belong with a
comment in the FE that needs them IMHO.

[Bug middle-end/56474] [4.8 regression] bogus Storage_Error raised for record containing empty zero-based array

[ebotcazou at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56474

--- Comment #12 from Eric Botcazou <ebotcazou at gcc dot gnu.org> 2013-03-04 16:18:21 UTC ---
> So, can the Ada FE be changed to use that?  Hacks like this belong with a
> comment in the FE that needs them IMHO.

The 2 other related hacks are in stor-layout.c line 2214 and 2236 though and
there are a lot of calls to size_binop in the Ada front-end.  IMO this one is
really benign compared to the others.

[Bug middle-end/56474] [4.8 regression] bogus Storage_Error raised for record containing empty zero-based array

[rguenther at suse dot de]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56474

--- Comment #13 from rguenther at suse dot de <rguenther at suse dot de> 2013-03-05 09:05:04 UTC ---
On Mon, 4 Mar 2013, ebotcazou at gcc dot gnu.org wrote:

> 
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56474
> 
> --- Comment #12 from Eric Botcazou <ebotcazou at gcc dot gnu.org> 2013-03-04 16:18:21 UTC ---
> > So, can the Ada FE be changed to use that?  Hacks like this belong with a
> > comment in the FE that needs them IMHO.
> 
> The 2 other related hacks are in stor-layout.c line 2214 and 2236 though and
> there are a lot of calls to size_binop in the Ada front-end.  IMO this one is
> really benign compared to the others.

Note the hacks all boil down to the fact that FEs use signed array
domains but unsigned sizetype TYPE_DOMAIN.  The C and C++ FE were
adjusted to use (unsigned) [1, 0] for zero-sized arrays - I believe
the current hacks are all because of Ada (and yes, I invented them
to not need to fiddle with the Ada FE when removing TYPE_IS_SIZETYPE).

There is still the Ada FE bug I opened that shows that arrays
with [0, USIZETYPE_MAX] vs. [SSIZETYPE_MIN, SSIZETYPE_MAX] do not
work.  Independent of any hacks to fix the current error to address
that you need to work to fix the signedness issue (which may of
course require middle-end adjustments).

If you don't want overflow you can as well re-set TREE_OVERFLOW
on the result ... (just to give another option).

[Bug middle-end/56474] [4.8 regression] bogus Storage_Error raised for record containing empty zero-based array

[ebotcazou at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56474

--- Comment #14 from Eric Botcazou <ebotcazou at gcc dot gnu.org> 2013-03-06 17:19:42 UTC ---
> Note the hacks all boil down to the fact that FEs use signed array
> domains but unsigned sizetype TYPE_DOMAIN.  The C and C++ FE were
> adjusted to use (unsigned) [1, 0] for zero-sized arrays - I believe
> the current hacks are all because of Ada (and yes, I invented them
> to not need to fiddle with the Ada FE when removing TYPE_IS_SIZETYPE).

Exactly, that's my point: all the hacks are there to cope with overflow issues
introduced by the removal of TYPE_IS_SIZETYPE.  Mine is another small one.

> There is still the Ada FE bug I opened that shows that arrays
> with [0, USIZETYPE_MAX] vs. [SSIZETYPE_MIN, SSIZETYPE_MAX] do not
> work.  Independent of any hacks to fix the current error to address
> that you need to work to fix the signedness issue (which may of
> course require middle-end adjustments).

Sure.

> If you don't want overflow you can as well re-set TREE_OVERFLOW
> on the result ... (just to give another option).

That's exactly what the hack at stor-layout.c 2234 does for C and C++!

[Bug middle-end/56474] [4.8 regression] bogus Storage_Error raised for record containing empty zero-based array

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56474

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Priority|P3                          |P4

[Bug middle-end/56474] [4.8/4.9 regression] bogus Storage_Error raised for record containing empty zero-based array

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56474

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|4.8.0                       |4.8.1

--- Comment #15 from Jakub Jelinek <jakub at gcc dot gnu.org> 2013-03-22 14:42:11 UTC ---
GCC 4.8.0 is being released, adjusting target milestone.

[Bug middle-end/56474] [4.8/4.9 regression] bogus Storage_Error raised for record containing empty zero-based array

[ebotcazou at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56474

--- Comment #17 from Eric Botcazou <ebotcazou at gcc dot gnu.org> 2013-05-06 09:33:53 UTC ---
> is the testcase when changed to 'Empty_Vector : Vector (-2);' valid?

Yes, this is the superflat case.

> In any case, in 0 .. Size, Size seems to be signed.

Yes, Stream_Element_Offset is signed.

> Also, there is the special-case
> 
>               /* Similarly, if one of the values overflows in sizetype and the
>                  range is null, use 1..0 for the sizetype bounds.  */
>               else if (TREE_CODE (gnu_min) == INTEGER_CST
>                        && TREE_CODE (gnu_max) == INTEGER_CST
>                        && (TREE_OVERFLOW (gnu_min) || TREE_OVERFLOW (gnu_max))
>                        && tree_int_cst_lt (gnu_orig_max, gnu_orig_min))
>                 {
>                   gnu_min = size_one_node;
>                   gnu_max = size_zero_node;
>                   gnu_high = gnu_max;
>                 }
> 
> that simply chooses [1, 0] as "empty" domain, regardless of the original
> min or max.  So both cases (of which you fix only one),
> 
>               /* Otherwise, if the high bound is constant but the low bound is
>                  not, we use the expression (hb >= lb) ? lb : hb + 1 for the
>                  lower bound.  Note that the comparison must be done in the
>                  original type to avoid any overflow during the conversion.  */
>               else if (TREE_CODE (gnu_max) == INTEGER_CST
>                        && TREE_CODE (gnu_min) != INTEGER_CST)
>                 {
>                   gnu_high = gnu_max;
>                   gnu_min
>                     = build_cond_expr (sizetype,
>                                        build_binary_op (GE_EXPR,
>                                                         boolean_type_node,
>                                                         gnu_orig_max,
>                                                         gnu_orig_min),
>                                        gnu_min,
>                                        size_binop (PLUS_EXPR, gnu_max,
>                                                    size_one_node));
>                 }
> 
>               /* Finally we use (hb >= lb) ? hb : lb - 1 for the upper bound
>                  in all the other cases.  Note that, here as well as above,
>                  the condition used in the comparison must be equivalent to
>                  the condition (length != 0).  This is relied upon in order
>                  to optimize array comparisons in compare_arrays.  */
>               else
>                 gnu_high
>                   = build_cond_expr (sizetype,
>                                      build_binary_op (GE_EXPR,
>                                                       boolean_type_node,
>                                                       gnu_orig_max,
>                                                       gnu_orig_min),
>                                      gnu_max,
>                                      size_binop (MINUS_EXPR, gnu_min,
>                                                  size_one_node));
> 
> could be combined to
> 
>              else
>                {
>                   gnu_min
>                     = build_cond_expr (sizetype,
>                                        build_binary_op (GE_EXPR,
>                                                         boolean_type_node,
>                                                         gnu_orig_max,
>                                                         gnu_orig_min),
>                                        gnu_min,
>                                        size_one_node));
>                 gnu_high
>                   = build_cond_expr (sizetype,
>                                      build_binary_op (GE_EXPR,
>                                                       boolean_type_node,
>                                                       gnu_orig_max,
>                                                       gnu_orig_min),
>                                      gnu_max,
>                                      size_zero_node);
>                 }
> 
> which fixes the issue for me.

Right, at the expense of pessimizing a lot the common case of constant low
bound and variable high bound.  Not acceptable I'm afraid.

> I can also easily guard the existing size_binop (MINUS_EXPR, gnu_min,
> size_one_node) for gnu_min being zero, or I can simply use
> int_const_binop here instead, as you expect that operation to _never_
> set TREE_OVERFLOW if not gnu_min had TREE_OVERFLOW set.

Adding a guard would also pessimize the common case, albeit less severely.
But using int_const_binop looks workable, I'll give it a whirl.

> Note that if gnu_max - 1 is not computable as compile-time constant here,
> but is retained as tree expression and folded later overflow will not
> be considered either (it doesn't go the size_* API way, which is supposed
> to be the interface giving overflow hints to the FEs via means of
> TREE_OVERFLOW which better should not appear later during optimization).

Yes, but these expressions appear in bounds and size of integer and array types
so they cannot be arbitrarily complex, otherwise things can quickly blow up.

[Bug ada/56474] [4.8/4.9 regression] bogus Storage_Error raised for record containing empty zero-based array

[ebotcazou at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56474

Eric Botcazou <ebotcazou at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
          Component|middle-end                  |ada

--- Comment #18 from Eric Botcazou <ebotcazou at gcc dot gnu.org> 2013-05-07 07:50:39 UTC ---
Recategorizing.

[Bug ada/56474] [4.8/4.9 regression] bogus Storage_Error raised for record containing empty zero-based array

[ebotcazou at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56474

--- Comment #19 from Eric Botcazou <ebotcazou at gcc dot gnu.org> 2013-05-07 08:07:27 UTC ---
Author: ebotcazou
Date: Tue May  7 07:59:37 2013
New Revision: 198663

URL: http://gcc.gnu.org/viewcvs?rev=198663&root=gcc&view=rev
Log:
    PR ada/56474
    * gcc-interface/decl.c (gnat_to_gnu_entity) <E_Array_Subtype>: Use
    int_const_binop to shift bounds by 1 when they are integer constants.

Added:
    trunk/gcc/testsuite/gnat.dg/specs/array3.ads
Modified:
    trunk/gcc/ada/ChangeLog
    trunk/gcc/ada/gcc-interface/decl.c
    trunk/gcc/testsuite/ChangeLog

[Bug ada/56474] [4.8/4.9 regression] bogus Storage_Error raised for record containing empty zero-based array

[ebotcazou at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56474

--- Comment #20 from Eric Botcazou <ebotcazou at gcc dot gnu.org> 2013-05-07 08:07:45 UTC ---
Author: ebotcazou
Date: Tue May  7 08:03:15 2013
New Revision: 198664

URL: http://gcc.gnu.org/viewcvs?rev=198664&root=gcc&view=rev
Log:
    PR ada/56474
    * gcc-interface/decl.c (gnat_to_gnu_entity) <E_Array_Subtype>: Use
    int_const_binop to shift bounds by 1 when they are integer constants.

Added:
    branches/gcc-4_8-branch/gcc/testsuite/gnat.dg/specs/array3.ads
      - copied unchanged from r198663,
trunk/gcc/testsuite/gnat.dg/specs/array3.ads
Modified:
    branches/gcc-4_8-branch/gcc/ada/ChangeLog
    branches/gcc-4_8-branch/gcc/ada/gcc-interface/decl.c
    branches/gcc-4_8-branch/gcc/testsuite/ChangeLog

[Bug ada/56474] [4.8/4.9 regression] bogus Storage_Error raised for record containing empty zero-based array

[ebotcazou at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56474

Eric Botcazou <ebotcazou at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|                            |FIXED

--- Comment #21 from Eric Botcazou <ebotcazou at gcc dot gnu.org> 2013-05-07 08:08:18 UTC ---
Fixed at last.