| Message ID | 4D783E7D.2060409@redhat.com |
|---|---|
| State | New |
| Headers | show |
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: > >
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
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 >
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
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
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:
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.