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

[Richard Guenther <richard dot guenther at gmail dot com>]

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