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

[Jason Merrill <jason at redhat dot com>]

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