[C++,RFC] Fix ICE with late attributes in templates (PR c++/83300)

Message ID 20171207164546.GL2353@tucnak
State New
Headers show
Series
  • [C++,RFC] Fix ICE with late attributes in templates (PR c++/83300)
Related show

Commit Message

Jakub Jelinek Dec. 7, 2017, 4:45 p.m.
Hi!

save_template_attributes ignored flags, when ATTR_FLAG_TYPE_IN_PLACE
wasn't set on a type, it would happily attach the attributes to some
existing type (in this case to integer_type_node).

My first approach was to just call build_type_attribute_variant, but
that ICEs on g++.dg/cpp0x/alias-decl-59.C, because there *decl_p is
UNDERLYING_TYPE, which the generic type_hash_canon
build_type_attribute_variant calls doesn't like.
This patch just creates a new variant type if *decl_p is dependent,
it passes bootstrap/regtest, but I'm not really sure if we need any
kind of FE type hashing for such types (when pt.c will handle it with
!processing_template_decl it will be built using
build_type_attribute_variant and afterwards we'll have the types hashed).
So, is this enough, or do we need to do something else (and what)?

2017-12-07  Jakub Jelinek  <jakub@redhat.com>

	PR c++/83300
	* decl2.c (save_template_attributes): Add flags argument, if
	not ATTR_FLAG_TYPE_IN_PLACE, *decl_p is a type and we want to
	modify TYPE_ATTRIBUTES, add them on type attribute variant.

	* g++.dg/ext/vector33.C: New test.


	Jakub

Comments

Jason Merrill Dec. 15, 2017, 8:02 p.m. | #1
On 12/07/2017 11:45 AM, Jakub Jelinek wrote:
> save_template_attributes ignored flags, when ATTR_FLAG_TYPE_IN_PLACE

> wasn't set on a type, it would happily attach the attributes to some

> existing type (in this case to integer_type_node).

> 

> My first approach was to just call build_type_attribute_variant, but

> that ICEs on g++.dg/cpp0x/alias-decl-59.C, because there *decl_p is

> UNDERLYING_TYPE, which the generic type_hash_canon

> build_type_attribute_variant calls doesn't like.


Ah, because it calls layout_type.  What if we did this?

Jason
diff --git a/gcc/tree.c b/gcc/tree.c
index ed1852b3e66..4883b711624 100644
--- a/gcc/tree.c
+++ b/gcc/tree.c
@@ -6445,7 +6445,8 @@ type_hash_canon (unsigned int hashcode, tree type)
 
   /* The TYPE_ALIGN field of a type is set by layout_type(), so we
      must call that routine before comparing TYPE_ALIGNs.  */
-  layout_type (type);
+  if (TREE_CODE (type) < NUM_TREE_CODES)
+    layout_type (type);
 
   in.hash = hashcode;
   in.type = type;
Jakub Jelinek Dec. 15, 2017, 8:11 p.m. | #2
On Fri, Dec 15, 2017 at 03:02:50PM -0500, Jason Merrill wrote:
> On 12/07/2017 11:45 AM, Jakub Jelinek wrote:

> > save_template_attributes ignored flags, when ATTR_FLAG_TYPE_IN_PLACE

> > wasn't set on a type, it would happily attach the attributes to some

> > existing type (in this case to integer_type_node).

> > 

> > My first approach was to just call build_type_attribute_variant, but

> > that ICEs on g++.dg/cpp0x/alias-decl-59.C, because there *decl_p is

> > UNDERLYING_TYPE, which the generic type_hash_canon

> > build_type_attribute_variant calls doesn't like.

> 

> Ah, because it calls layout_type.  What if we did this?

> 

> Jason


> diff --git a/gcc/tree.c b/gcc/tree.c

> index ed1852b3e66..4883b711624 100644

> --- a/gcc/tree.c

> +++ b/gcc/tree.c

> @@ -6445,7 +6445,8 @@ type_hash_canon (unsigned int hashcode, tree type)

>  

>    /* The TYPE_ALIGN field of a type is set by layout_type(), so we

>       must call that routine before comparing TYPE_ALIGNs.  */

> -  layout_type (type);

> +  if (TREE_CODE (type) < NUM_TREE_CODES)

> +    layout_type (type);

>  

>    in.hash = hashcode;

>    in.type = type;


I think that can't be sufficient, because type_cache_hasher::equal
has:
  switch (TREE_CODE (a->type))
    {
...
    default:
      return 0;
    }

  if (lang_hooks.types.type_hash_eq != NULL)
    return lang_hooks.types.type_hash_eq (a->type, b->type);

  return 1;
}

so for types it doesn't know about it will just always return 0.
Or is that what we want for the FE specific types?

Another possibility would be to return 0; for default only if
lang_hooks.types.type_hash_eq is NULL, and otherwise defer to
the langhook, plus changing the C++ and Ada langhooks to do
something with them if needed.

	Jakub
Jason Merrill Dec. 15, 2017, 9:15 p.m. | #3
On Fri, Dec 15, 2017 at 3:11 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Fri, Dec 15, 2017 at 03:02:50PM -0500, Jason Merrill wrote:

>> On 12/07/2017 11:45 AM, Jakub Jelinek wrote:

>> > save_template_attributes ignored flags, when ATTR_FLAG_TYPE_IN_PLACE

>> > wasn't set on a type, it would happily attach the attributes to some

>> > existing type (in this case to integer_type_node).

>> >

>> > My first approach was to just call build_type_attribute_variant, but

>> > that ICEs on g++.dg/cpp0x/alias-decl-59.C, because there *decl_p is

>> > UNDERLYING_TYPE, which the generic type_hash_canon

>> > build_type_attribute_variant calls doesn't like.

>>

>> Ah, because it calls layout_type.  What if we did this?

>>

>> Jason

>

>> diff --git a/gcc/tree.c b/gcc/tree.c

>> index ed1852b3e66..4883b711624 100644

>> --- a/gcc/tree.c

>> +++ b/gcc/tree.c

>> @@ -6445,7 +6445,8 @@ type_hash_canon (unsigned int hashcode, tree type)

>>

>>    /* The TYPE_ALIGN field of a type is set by layout_type(), so we

>>       must call that routine before comparing TYPE_ALIGNs.  */

>> -  layout_type (type);

>> +  if (TREE_CODE (type) < NUM_TREE_CODES)

>> +    layout_type (type);

>>

>>    in.hash = hashcode;

>>    in.type = type;

>

> I think that can't be sufficient, because type_cache_hasher::equal

> has:

>   switch (TREE_CODE (a->type))

>     {

> ...

>     default:

>       return 0;

>     }

>

>   if (lang_hooks.types.type_hash_eq != NULL)

>     return lang_hooks.types.type_hash_eq (a->type, b->type);

>

>   return 1;

> }

>

> so for types it doesn't know about it will just always return 0.

> Or is that what we want for the FE specific types?

>

> Another possibility would be to return 0; for default only if

> lang_hooks.types.type_hash_eq is NULL, and otherwise defer to

> the langhook, plus changing the C++ and Ada langhooks to do

> something with them if needed.


The latter sounds right to me. It's surprising that this hasn't been a
problem before.

But if you don't feel like messing with this now, your patch is OK,
just add a FIXME comment about why the else is necessary.

Jason

Patch

--- gcc/cp/decl2.c.jj	2017-12-06 23:48:08.205147975 +0100
+++ gcc/cp/decl2.c	2017-12-07 09:39:18.539996630 +0100
@@ -1244,7 +1244,7 @@  splice_template_attributes (tree *attr_p
    DECL_P.  */
 
 static void
-save_template_attributes (tree *attr_p, tree *decl_p)
+save_template_attributes (tree *attr_p, tree *decl_p, int flags)
 {
   tree *q;
 
@@ -1265,7 +1265,20 @@  save_template_attributes (tree *attr_p,
   /* Merge the late attributes at the beginning with the attribute
      list.  */
   late_attrs = merge_attributes (late_attrs, *q);
-  *q = late_attrs;
+  if (*q != late_attrs
+      && !DECL_P (*decl_p)
+      && !(flags & ATTR_FLAG_TYPE_IN_PLACE))
+    {
+      if (!dependent_type_p (*decl_p))
+	*decl_p = cp_build_type_attribute_variant (*decl_p, late_attrs);
+      else
+	{
+	  *decl_p = build_variant_type_copy (*decl_p);
+	  TYPE_ATTRIBUTES (*decl_p) = late_attrs;
+	}
+    }
+  else
+    *q = late_attrs;
 
   if (!DECL_P (*decl_p) && *decl_p == TYPE_MAIN_VARIANT (*decl_p))
     {
@@ -1466,7 +1479,7 @@  cplus_decl_attributes (tree *decl, tree
       if (check_for_bare_parameter_packs (attributes))
 	return;
 
-      save_template_attributes (&attributes, decl);
+      save_template_attributes (&attributes, decl, flags);
     }
 
   cp_check_const_attributes (attributes);
--- gcc/testsuite/g++.dg/ext/vector33.C.jj	2017-12-07 09:10:24.227635836 +0100
+++ gcc/testsuite/g++.dg/ext/vector33.C	2017-12-07 09:10:24.227635836 +0100
@@ -0,0 +1,10 @@ 
+// PR c++/83300
+// { dg-do compile { target c++11 } }
+
+template<int N>
+using T = int __attribute__((vector_size (sizeof(int) * N)));
+
+void
+f (T<4>)
+{
+}