RFA (layout): PATCH for c++/48029 (ICE-on-valid with templates and arrays)

RFA (layout): PATCH for c++/48029 (ICE-on-valid with templates and arrays)

[Jason Merrill]

[-- Attachment #1: Type: text/plain, Size: 1256 bytes --]

In this testcase, when we first declare the myvectypes and mytype3, 
vector<string> has not been instantiated, so we mark the array, and the 
pointer to the array, for structural equality comparison.  When we 
actually go to instantiate mytype3, we complete vector<string> and 
rebuild the array and pointer types, and use those to look up the 
template specialization.  Which fails to find the one we already had 
because the new pointer type has TYPE_CANONICAL and therefore hashes
differently from the one that didn't.

We deal with ARRAY_TYPE specially in iterative_hash_template_arg, but 
that doesn't cover a compound type which uses an ARRAY_TYPE, such as the 
pointer in this case.

The business of having an array with the same element type and domain 
have different TYPE_CANONICAL dependending on whether or not the element 
type is complete has always seemed strange and fragile to me, so I tried 
removing the relevant code from layout_type; this produced only a single 
test failure, which was fixed by changing type_hash_eq to not trust 
TYPE_ALIGN if the type isn't complete yet.  I imagine that's what Doug 
was talking about in his comment about alignment.

Tested (c,c++,fortran,java,lto,objc) x86_64-pc-linux-gnu.  OK for 4.5 
and 4.6?

[-- Attachment #2: 48029.patch --]
[-- Type: text/plain, Size: 2677 bytes --]

commit 45deb1cd5953c5730e14e00c5a8f800dadea66bd
Author: Jason Merrill <jason@redhat.com>
Date:   Wed Mar 9 16:47:10 2011 -0500

    	PR c++/48029
    	* stor-layout.c (layout_type): Don't set structural equality
    	on arrays of incomplete type.
    	* tree.c (type_hash_eq): Handle comparing them properly.
    	* cp/pt.c (iterative_hash_template_arg): Remove special case for
    	ARRAY_TYPE.

diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c
index ac91698..ab2aea3 100644
--- a/gcc/cp/pt.c
+++ b/gcc/cp/pt.c
@@ -1569,13 +1569,6 @@ iterative_hash_template_arg (tree arg, hashval_t val)
       val = iterative_hash_object (code, val);
       return iterative_hash_template_arg (TREE_OPERAND (arg, 2), val);
 
-    case ARRAY_TYPE:
-      /* layout_type sets structural equality for arrays of
-	 incomplete type, so we can't rely on the canonical type
-	 for hashing.  */
-      val = iterative_hash_template_arg (TREE_TYPE (arg), val);
-      return iterative_hash_template_arg (TYPE_DOMAIN (arg), val);
-
     case LAMBDA_EXPR:
       /* A lambda can't appear in a template arg, but don't crash on
 	 erroneous input.  */
diff --git a/gcc/stor-layout.c b/gcc/stor-layout.c
index 9056d7e..ed36c5b 100644
--- a/gcc/stor-layout.c
+++ b/gcc/stor-layout.c
@@ -2028,11 +2028,6 @@ layout_type (tree type)
 #else
 	TYPE_ALIGN (type) = MAX (TYPE_ALIGN (element), BITS_PER_UNIT);
 #endif
-	if (!TYPE_SIZE (element))
-	  /* We don't know the size of the underlying element type, so
-	     our alignment calculations will be wrong, forcing us to
-	     fall back on structural equality. */
-	  SET_TYPE_STRUCTURAL_EQUALITY (type);
 	TYPE_USER_ALIGN (type) = TYPE_USER_ALIGN (element);
 	SET_TYPE_MODE (type, BLKmode);
 	if (TYPE_SIZE (type) != 0
diff --git a/gcc/tree.c b/gcc/tree.c
index c947072..61532db 100644
--- a/gcc/tree.c
+++ b/gcc/tree.c
@@ -5981,12 +5981,18 @@ type_hash_eq (const void *va, const void *vb)
       || TREE_TYPE (a->type) != TREE_TYPE (b->type)
       || !attribute_list_equal (TYPE_ATTRIBUTES (a->type),
 				 TYPE_ATTRIBUTES (b->type))
-      || TYPE_ALIGN (a->type) != TYPE_ALIGN (b->type)
-      || TYPE_MODE (a->type) != TYPE_MODE (b->type)
       || (TREE_CODE (a->type) != COMPLEX_TYPE
           && TYPE_NAME (a->type) != TYPE_NAME (b->type)))
     return 0;
 
+  /* Be careful about comparing arrays before and after the element type
+     has been completed; don't compare TYPE_ALIGN unless both types are
+     complete.  */
+  if (TYPE_SIZE (a->type) && TYPE_SIZE (b->type)
+      && (TYPE_ALIGN (a->type) != TYPE_ALIGN (b->type)
+	  || TYPE_MODE (a->type) != TYPE_MODE (b->type)))
+    return 0;
+
   switch (TREE_CODE (a->type))
     {
     case VOID_TYPE:

Re: RFA (layout): PATCH for c++/48029 (ICE-on-valid with templates and arrays)

[Richard Guenther]

On Thu, Mar 10, 2011 at 3:59 AM, Jason Merrill <jason@redhat.com> wrote:
> In this testcase, when we first declare the myvectypes and mytype3,
> vector<string> has not been instantiated, so we mark the array, and the
> pointer to the array, for structural equality comparison.  When we actually
> go to instantiate mytype3, we complete vector<string> and rebuild the array
> and pointer types, and use those to look up the template specialization.
>  Which fails to find the one we already had because the new pointer type has
> TYPE_CANONICAL and therefore hashes
> differently from the one that didn't.
>
> We deal with ARRAY_TYPE specially in iterative_hash_template_arg, but that
> doesn't cover a compound type which uses an ARRAY_TYPE, such as the pointer
> in this case.
>
> The business of having an array with the same element type and domain have
> different TYPE_CANONICAL dependending on whether or not the element type is
> complete has always seemed strange and fragile to me, so I tried removing
> the relevant code from layout_type; this produced only a single test
> failure, which was fixed by changing type_hash_eq to not trust TYPE_ALIGN if
> the type isn't complete yet.  I imagine that's what Doug was talking about
> in his comment about alignment.

Ugh.  Why do we call layout_type on arrays with incomplete element type
at all?  I suppose the array type is still considered un-layouted after
that finished (NULL TYPE_SIZE)?  So, what does layout_type provide
that the C++ FE relies on when layouting this kind of type?

Other than the above questions the patch looks ok if indeed layout_type
returns with a NULL TYPE_SIZE.

Thanks,
Richard.

> Tested (c,c++,fortran,java,lto,objc) x86_64-pc-linux-gnu.  OK for 4.5 and
> 4.6?
>
> commit 45deb1cd5953c5730e14e00c5a8f800dadea66bd
> Author: Jason Merrill <jason@redhat.com>
> Date:   Wed Mar 9 16:47:10 2011 -0500
>
>        PR c++/48029
>        * stor-layout.c (layout_type): Don't set structural equality
>        on arrays of incomplete type.
>        * tree.c (type_hash_eq): Handle comparing them properly.
>        * cp/pt.c (iterative_hash_template_arg): Remove special case for
>        ARRAY_TYPE.
>
> diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c
> index ac91698..ab2aea3 100644
> --- a/gcc/cp/pt.c
> +++ b/gcc/cp/pt.c
> @@ -1569,13 +1569,6 @@ iterative_hash_template_arg (tree arg, hashval_t val)
>       val = iterative_hash_object (code, val);
>       return iterative_hash_template_arg (TREE_OPERAND (arg, 2), val);
>
> -    case ARRAY_TYPE:
> -      /* layout_type sets structural equality for arrays of
> -        incomplete type, so we can't rely on the canonical type
> -        for hashing.  */
> -      val = iterative_hash_template_arg (TREE_TYPE (arg), val);
> -      return iterative_hash_template_arg (TYPE_DOMAIN (arg), val);
> -
>     case LAMBDA_EXPR:
>       /* A lambda can't appear in a template arg, but don't crash on
>         erroneous input.  */
> diff --git a/gcc/stor-layout.c b/gcc/stor-layout.c
> index 9056d7e..ed36c5b 100644
> --- a/gcc/stor-layout.c
> +++ b/gcc/stor-layout.c
> @@ -2028,11 +2028,6 @@ layout_type (tree type)
>  #else
>        TYPE_ALIGN (type) = MAX (TYPE_ALIGN (element), BITS_PER_UNIT);
>  #endif
> -       if (!TYPE_SIZE (element))
> -         /* We don't know the size of the underlying element type, so
> -            our alignment calculations will be wrong, forcing us to
> -            fall back on structural equality. */
> -         SET_TYPE_STRUCTURAL_EQUALITY (type);
>        TYPE_USER_ALIGN (type) = TYPE_USER_ALIGN (element);
>        SET_TYPE_MODE (type, BLKmode);
>        if (TYPE_SIZE (type) != 0
> diff --git a/gcc/tree.c b/gcc/tree.c
> index c947072..61532db 100644
> --- a/gcc/tree.c
> +++ b/gcc/tree.c
> @@ -5981,12 +5981,18 @@ type_hash_eq (const void *va, const void *vb)
>       || TREE_TYPE (a->type) != TREE_TYPE (b->type)
>       || !attribute_list_equal (TYPE_ATTRIBUTES (a->type),
>                                 TYPE_ATTRIBUTES (b->type))
> -      || TYPE_ALIGN (a->type) != TYPE_ALIGN (b->type)
> -      || TYPE_MODE (a->type) != TYPE_MODE (b->type)
>       || (TREE_CODE (a->type) != COMPLEX_TYPE
>           && TYPE_NAME (a->type) != TYPE_NAME (b->type)))
>     return 0;
>
> +  /* Be careful about comparing arrays before and after the element type
> +     has been completed; don't compare TYPE_ALIGN unless both types are
> +     complete.  */
> +  if (TYPE_SIZE (a->type) && TYPE_SIZE (b->type)
> +      && (TYPE_ALIGN (a->type) != TYPE_ALIGN (b->type)
> +         || TYPE_MODE (a->type) != TYPE_MODE (b->type)))
> +    return 0;
> +
>   switch (TREE_CODE (a->type))
>     {
>     case VOID_TYPE:
>
>

Re: RFA (layout): PATCH for c++/48029 (ICE-on-valid with templates and arrays)

[Jason Merrill]

On 03/10/2011 04:56 AM, Richard Guenther wrote:
> Ugh.  Why do we call layout_type on arrays with incomplete element type
> at all?

layout_type has been called from the language-independent 
build_array_type since the dawn of revision control.

> I suppose the array type is still considered un-layouted after
> that finished (NULL TYPE_SIZE)?

Yes, layout_type only sets TYPE_SIZE if the element has a size.

> So, what does layout_type provide
> that the C++ FE relies on when layouting this kind of type?

Nothing that the FE relies on.  It sets the size, alignment and mode of 
the array type (if the element type is complete) and also builds the 
pointer-to-element type.  None of this seems necessary for an incomplete 
element type, but it doesn't seem to do any harm either; we need the 
change to type_hash_eq either way.

While looking at the history, it occurred to me that 
COMPLETE_OR_UNBOUND_ARRAY_TYPE_P is a better test than TYPE_SIZE in the 
type_hash_eq change, so I'm going to make that tweak to the patch; I 
assume you don't object.

Jason

Re: RFA (layout): PATCH for c++/48029 (ICE-on-valid with templates and arrays)

[Richard Guenther]

On Thu, Mar 10, 2011 at 3:56 PM, Jason Merrill <jason@redhat.com> wrote:
> On 03/10/2011 04:56 AM, Richard Guenther wrote:
>>
>> Ugh.  Why do we call layout_type on arrays with incomplete element type
>> at all?
>
> layout_type has been called from the language-independent build_array_type
> since the dawn of revision control.

Ugh, indeed - now I remember.  Something I wanted to try remove at
some point ;)

>> I suppose the array type is still considered un-layouted after
>> that finished (NULL TYPE_SIZE)?
>
> Yes, layout_type only sets TYPE_SIZE if the element has a size.
>
>> So, what does layout_type provide
>> that the C++ FE relies on when layouting this kind of type?
>
> Nothing that the FE relies on.  It sets the size, alignment and mode of the
> array type (if the element type is complete) and also builds the
> pointer-to-element type.  None of this seems necessary for an incomplete
> element type, but it doesn't seem to do any harm either; we need the change
> to type_hash_eq either way.
>
> While looking at the history, it occurred to me that
> COMPLETE_OR_UNBOUND_ARRAY_TYPE_P is a better test than TYPE_SIZE in the
> type_hash_eq change, so I'm going to make that tweak to the patch; I assume
> you don't object.

No, that's fine.

Richard.

> Jason
>

Re: RFA (layout): PATCH for c++/48029 (ICE-on-valid with templates and arrays)

[H.J. Lu]

On Wed, Mar 9, 2011 at 6:59 PM, Jason Merrill <jason@redhat.com> wrote:
> In this testcase, when we first declare the myvectypes and mytype3,
> vector<string> has not been instantiated, so we mark the array, and the
> pointer to the array, for structural equality comparison.  When we actually
> go to instantiate mytype3, we complete vector<string> and rebuild the array
> and pointer types, and use those to look up the template specialization.
>  Which fails to find the one we already had because the new pointer type has
> TYPE_CANONICAL and therefore hashes
> differently from the one that didn't.
>
> We deal with ARRAY_TYPE specially in iterative_hash_template_arg, but that
> doesn't cover a compound type which uses an ARRAY_TYPE, such as the pointer
> in this case.
>
> The business of having an array with the same element type and domain have
> different TYPE_CANONICAL dependending on whether or not the element type is
> complete has always seemed strange and fragile to me, so I tried removing
> the relevant code from layout_type; this produced only a single test
> failure, which was fixed by changing type_hash_eq to not trust TYPE_ALIGN if
> the type isn't complete yet.  I imagine that's what Doug was talking about
> in his comment about alignment.
>
> Tested (c,c++,fortran,java,lto,objc) x86_64-pc-linux-gnu.  OK for 4.5 and
> 4.6?
>
> commit 45deb1cd5953c5730e14e00c5a8f800dadea66bd
> Author: Jason Merrill <jason@redhat.com>
> Date:   Wed Mar 9 16:47:10 2011 -0500
>
>        PR c++/48029
>        * stor-layout.c (layout_type): Don't set structural equality
>        on arrays of incomplete type.
>        * tree.c (type_hash_eq): Handle comparing them properly.
>        * cp/pt.c (iterative_hash_template_arg): Remove special case for
>        ARRAY_TYPE.
>

This caused:

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

-- 
H.J.

Re: RFA (layout): PATCH for c++/48029 (ICE-on-valid with templates and arrays)

[Jason Merrill]

On 03/10/2011 09:56 AM, Jason Merrill wrote:
> While looking at the history, it occurred to me that
> COMPLETE_OR_UNBOUND_ARRAY_TYPE_P is a better test than TYPE_SIZE in the
> type_hash_eq change, so I'm going to make that tweak to the patch

OK, apparently this was a bad idea; it caused 48069.  So I switched back 
to COMPLETE_TYPE_P.

Jason