[Bug ada/50294] New: ICE in output_constructor_regular_field, at varasm.c:4751

[Bug ada/50294] New: ICE in output_constructor_regular_field, at varasm.c:4751

[rguenth at gcc dot gnu.org]

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

             Bug #: 50294
           Summary: ICE in output_constructor_regular_field, at
                    varasm.c:4751
    Classification: Unclassified
           Product: gcc
           Version: 4.7.0
            Status: UNCONFIRMED
          Keywords: ice-on-valid-code, wrong-code
          Severity: normal
          Priority: P3
         Component: ada
        AssignedTo: unassigned@gcc.gnu.org
        ReportedBy: rguenth@gcc.gnu.org
                CC: ebotcazou@gcc.gnu.org


A modified gnat.dg/loop_optimization3.adb, like below

-- { dg-do run }
-- { dg-options "-O" }

with Loop_Optimization3_Pkg; use Loop_Optimization3_Pkg;

procedure Loop_Optimization3 is

  type Unsigned_64_T is mod 2 ** 64;
  type Arr is array (Unsigned_64_T range 9223372036854775805 ..
9223372036854775811) of Integer;
  C : constant Arr := (1, others => F(2));

begin
  if C /= (1, 2, 2, 2, 2, 2, 2) then
    raise Program_Error;
  end if;
end;

ICEs like

> ./gnat1 -quiet loop_optimization3.adb 
loop_optimization3.adb:10:03: warning: "Storage_Error" will be raised at run
time
+===========================GNAT BUG DETECTED==============================+
| 4.7.0 20110905 (experimental) (x86_64-unknown-linux-gnu) GCC error:      |
| in output_constructor_regular_field, at varasm.c:4751                    |
| Error detected around loop_optimization3.adb:16:4                        |
| Please submit a bug report; see http://gcc.gnu.org/bugs.html.            |
| Use a subject line meaningful to you and us to track the bug.            |
| Include the entire contents of this bug box in the report.               |
| Include the exact gcc or gnatmake command that you entered.              |
| Also include sources listed below in gnatchop format                     |
| (concatenated together with no headers between files).                   |
+==========================================================================+


The program should either be rejected or should not ICE.

Emitting Storage_Error at runtime sounds odd as well for an array with
7 elements.  Thus, if the ICE is fixed the program will fail at runtime.


I run into exactly the same situation (well, not the ICE but the
Storage_Error) with the sizetype no-longer sign-extends for the
original loop_optimization3.adb testcase.

Thus the question - what should stor-layout do with domain types
that wrap the wrong way around (i.e. are of wrong type because Ada
turns everything into sizetype instead of [s]sizetype dependent on
the Ada type "signedness")?

[Bug ada/50294] ICE in output_constructor_regular_field, at varasm.c:4751

[rguenth at gcc dot gnu.org]

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

--- Comment #1 from Richard Guenther <rguenth at gcc dot gnu.org> 2011-09-05 13:59:44 UTC ---
http://gcc.gnu.org/ml/gcc-patches/2011-09/msg00280.html fixes the
Storage_Error issue but still ICEs because of the forced sign-extension
in output_constructor_regular_field which we do because of Ada having
"negative" DECL_FIELD_OFFSET values.

[Bug ada/50294] ICE in output_constructor_regular_field, at varasm.c:4751

[rguenth at gcc dot gnu.org]

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

--- Comment #2 from Richard Guenther <rguenth at gcc dot gnu.org> 2011-09-05 14:06:57 UTC ---
Index: gcc/varasm.c
===================================================================
--- gcc/varasm.c        (revision 178527)
+++ gcc/varasm.c        (working copy)
@@ -4746,9 +4746,13 @@ output_constructor_regular_field (oc_loc

   if (local->index != NULL_TREE)
     {
+      /* Perform the index calculation in modulo arithmetic but
+         sign-extend the result because Ada has negative DECL_FIELD_OFFSETs
+        but we are using an unsigned sizetype.  */
+      unsigned prec = TYPE_PRECISION (sizetype);
       double_int idx = double_int_sub (tree_to_double_int (local->index),
                                       tree_to_double_int (local->min_index));
-      gcc_assert (double_int_fits_in_shwi_p (idx));
+      idx = double_int_sext (idx, prec);
       fieldpos = (tree_low_cst (TYPE_SIZE_UNIT (TREE_TYPE (local->val)), 1)
                  * idx.low);
     }

fixes this, but loses the check for index - min_index overflowing a HWI
(arguably that should have been detected earlier in the FEs).  Fact is
we lose information when forcing offsets / indices to an unsigned type.

[Bug ada/50294] ICE in output_constructor_regular_field

[ebotcazou at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |ASSIGNED
   Last reconfirmed|                            |2011-09-05
                 CC|ebotcazou at gcc dot        |
                   |gnu.org                     |
         AssignedTo|unassigned at gcc dot       |ebotcazou at gcc dot
                   |gnu.org                     |gnu.org
            Summary|ICE in                      |ICE in
                   |output_constructor_regular_ |output_constructor_regular_
                   |field, at varasm.c:4751     |field
     Ever Confirmed|0                           |1

--- Comment #3 from Eric Botcazou <ebotcazou at gcc dot gnu.org> 2011-09-05 18:32:35 UTC ---
Arrays with large index are a pain to support, no doubt about that.

[Bug ada/50294] ICE in output_constructor_regular_field

[rguenth at gcc dot gnu.org]

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

--- Comment #4 from Richard Guenther <rguenth at gcc dot gnu.org> 2011-09-06 08:31:56 UTC ---
My idea with fixing the Ada issue would be to conditionally use a signed
or unsigned (sizetype) domain type.  Not sure if all of the middle-end
copes well with ssizetype domains (but well, we should fix it then).

Of course that wouldn't fix use of an even larger domain type
such as Unsigned_128_T is mod 2 ** 128 and appropriate large constants.
But I see several special-cases in the Gigi code that tries to handle
overflow situations.

Thus, I think we ideally should allow any kind of integer type in
TYPE_DOMAIN, not just sizetype variants.  But that's of course an even
bigger change.

Sidenote: my idea was to remove special-casings of TYPE_IS_SIZETYPE from
folders but have a few special-cases explicit in size_binop so we know
the definite context from which they are invoked (like from stor-layout).
That's much better than to expose the special-casing from random
optimization passes that happen to call into fold.

[Bug ada/50294] ICE in output_constructor_regular_field

[ebotcazou at gcc dot gnu.org]

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

--- Comment #5 from Eric Botcazou <ebotcazou at gcc dot gnu.org> 2011-09-06 09:14:28 UTC ---
> My idea with fixing the Ada issue would be to conditionally use a signed
> or unsigned (sizetype) domain type.  Not sure if all of the middle-end
> copes well with ssizetype domains (but well, we should fix it then).

Yes, IIRC I tried something like that at some point.  You also mentioned in an
earlier thread that you tried to make sizetype signed.  What kind of problems
did you run into?  This was the original setting of the Ada compiler, so the
middle-end was able to support it, at least until relatively recently.

> Of course that wouldn't fix use of an even larger domain type
> such as Unsigned_128_T is mod 2 ** 128 and appropriate large constants.
> But I see several special-cases in the Gigi code that tries to handle
> overflow situations.

GNAT doesn't support integral types bigger than 64-bit anyway.

> Thus, I think we ideally should allow any kind of integer type in
> TYPE_DOMAIN, not just sizetype variants.  But that's of course an even
> bigger change.

The signedness flip is already tricky...

> Sidenote: my idea was to remove special-casings of TYPE_IS_SIZETYPE from
> folders but have a few special-cases explicit in size_binop so we know
> the definite context from which they are invoked (like from stor-layout).
> That's much better than to expose the special-casing from random
> optimization passes that happen to call into fold.

OK, I see.

[Bug ada/50294] ICE in output_constructor_regular_field

[rguenther at suse dot de]

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

--- Comment #6 from rguenther at suse dot de <rguenther at suse dot de> 2011-09-06 09:52:38 UTC ---
On Tue, 6 Sep 2011, ebotcazou at gcc dot gnu.org wrote:

> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50294
> 
> --- Comment #5 from Eric Botcazou <ebotcazou at gcc dot gnu.org> 2011-09-06 09:14:28 UTC ---
> > My idea with fixing the Ada issue would be to conditionally use a signed
> > or unsigned (sizetype) domain type.  Not sure if all of the middle-end
> > copes well with ssizetype domains (but well, we should fix it then).
> 
> Yes, IIRC I tried something like that at some point.  You also mentioned in an
> earlier thread that you tried to make sizetype signed.  What kind of problems
> did you run into?  This was the original setting of the Ada compiler, so the
> middle-end was able to support it, at least until relatively recently.

I run into various diagnostic issues in the C family frontends, so I 
quickly gave up.

> > Of course that wouldn't fix use of an even larger domain type
> > such as Unsigned_128_T is mod 2 ** 128 and appropriate large constants.
> > But I see several special-cases in the Gigi code that tries to handle
> > overflow situations.
> 
> GNAT doesn't support integral types bigger than 64-bit anyway.

And 32-bit for 32-bit targets?  sizetype is 32bits there ...

> > Thus, I think we ideally should allow any kind of integer type in
> > TYPE_DOMAIN, not just sizetype variants.  But that's of course an even
> > bigger change.
> 
> The signedness flip is already tricky...

Yeah, I backed off when seeing the amount of code I'd need to change
in GiGi ... esp. as I don't know the code well.

> > Sidenote: my idea was to remove special-casings of TYPE_IS_SIZETYPE from
> > folders but have a few special-cases explicit in size_binop so we know
> > the definite context from which they are invoked (like from stor-layout).
> > That's much better than to expose the special-casing from random
> > optimization passes that happen to call into fold.
> 
> OK, I see.

[Bug ada/50294] ICE in output_constructor_regular_field

[ebotcazou at gcc dot gnu.org]

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

--- Comment #7 from Eric Botcazou <ebotcazou at gcc dot gnu.org> 2011-09-06 10:21:42 UTC ---
> And 32-bit for 32-bit targets?  sizetype is 32bits there ...

No, 64-bit type are supported universally.  Of course your mileage may vary for
array types indexed with a 64-bit type...

[Bug ada/50294] ICE in output_constructor_regular_field

[ebotcazou at gcc dot gnu.org]

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

--- Comment #8 from Eric Botcazou <ebotcazou at gcc dot gnu.org> 2011-11-19 21:25:17 UTC ---
Created attachment 25861
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=25861
Improved testcase

It should fail everywhere.

[Bug ada/50294] ICE in output_constructor_regular_field

[ebotcazou at gcc dot gnu.org]

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

--- Comment #9 from Eric Botcazou <ebotcazou at gcc dot gnu.org> 2011-11-19 21:35:48 UTC ---
> Thus the question - what should stor-layout do with domain types
> that wrap the wrong way around (i.e. are of wrong type because Ada
> turns everything into sizetype instead of [s]sizetype dependent on
> the Ada type "signedness")?

What do you mean by "Ada turns everything into sizetype" here?  AFAIK the Ada
compiler behaves like the other compilers, i.e. domain types are built with

/* Create a type of integers to be the TYPE_DOMAIN of an ARRAY_TYPE.
   MAXVAL should be the maximum value in the domain
   (one less than the length of the array).

   The maximum value that MAXVAL can have is INT_MAX for a HOST_WIDE_INT.
   We don't enforce this limit, that is up to caller (e.g. language front end).
   The limit exists because the result is a signed type and we don't handle
   sizes that use more than one HOST_WIDE_INT.  */

tree
build_index_type (tree maxval)
{
  return build_range_type (sizetype, size_zero_node, maxval);
}

so are all subtypes of sizetype (gigi has create_index_type instead because it
needs to have non-shared range types and lower bounds, but it's equivalent).

[Bug ada/50294] ICE in output_constructor_regular_field

[rguenther at suse dot de]

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

--- Comment #10 from rguenther at suse dot de <rguenther at suse dot de> 2011-12-01 09:34:31 UTC ---
On Sat, 19 Nov 2011, ebotcazou at gcc dot gnu.org wrote:

> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50294
> 
> --- Comment #9 from Eric Botcazou <ebotcazou at gcc dot gnu.org> 2011-11-19 21:35:48 UTC ---
> > Thus the question - what should stor-layout do with domain types
> > that wrap the wrong way around (i.e. are of wrong type because Ada
> > turns everything into sizetype instead of [s]sizetype dependent on
> > the Ada type "signedness")?
> 
> What do you mean by "Ada turns everything into sizetype" here?  AFAIK the Ada
> compiler behaves like the other compilers, i.e. domain types are built with
> 
> /* Create a type of integers to be the TYPE_DOMAIN of an ARRAY_TYPE.
>    MAXVAL should be the maximum value in the domain
>    (one less than the length of the array).
> 
>    The maximum value that MAXVAL can have is INT_MAX for a HOST_WIDE_INT.
>    We don't enforce this limit, that is up to caller (e.g. language front end).
>    The limit exists because the result is a signed type and we don't handle
>    sizes that use more than one HOST_WIDE_INT.  */
> 
> tree
> build_index_type (tree maxval)
> {
>   return build_range_type (sizetype, size_zero_node, maxval);
> }
> 
> so are all subtypes of sizetype (gigi has create_index_type instead because it
> needs to have non-shared range types and lower bounds, but it's equivalent).

Sure, but only Ada allows user specified index types (AFAIK).  At least
for the C family domains are always unsigned and fit in sizetype.  The
middle-end / stor-layout code should get a "proper" domain type from the
frontends - either they canonicalize it to one that fits sizetype or
they provide a different typed domain (not sure which one is best,
but both are probably managable).

Richard.

[Bug ada/50294] ICE in output_constructor_regular_field

[ebotcazou at gcc dot gnu.org]

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

--- Comment #11 from Eric Botcazou <ebotcazou at gcc dot gnu.org> 2012-05-26 13:26:01 UTC ---
Author: ebotcazou
Date: Sat May 26 13:25:55 2012
New Revision: 187914

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=187914
Log:
    PR ada/50294
    * gnat.dg/array21.adb: New test.

Added:
    trunk/gcc/testsuite/gnat.dg/array21.adb
Modified:
    trunk/gcc/testsuite/ChangeLog

[Bug ada/50294] ICE in output_constructor_regular_field

[ebotcazou at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
                URL|                            |http://gcc.gnu.org/ml/gcc-c
                   |                            |vs/2012-05/msg00038.html
         Resolution|                            |FIXED
   Target Milestone|---                         |4.8.0

--- Comment #12 from Eric Botcazou <ebotcazou at gcc dot gnu.org> 2012-05-26 13:33:00 UTC ---
Not clear whether this is the most appropriate wording, but...