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:
>
>