Re: [PATCH, c++, PR77427 ] Set TYPE_STRUCTURAL_EQUALITY for sysv_abi va_list

[Jason Merrill <jason at redhat dot com>]

On Wed, Sep 7, 2016 at 5:22 AM, Richard Biener
<richard.guenther@gmail.com> wrote:
> On Mon, Sep 5, 2016 at 6:11 PM, Tom de Vries <Tom_deVries@mentor.com> wrote:
>> On 05/09/16 09:49, Richard Biener wrote:
>>>
>>> On Sun, Sep 4, 2016 at 11:30 PM, Tom de Vries <Tom_deVries@mentor.com>
>>> wrote:
>>>>
>>>> > On 04/09/16 16:08, Richard Biener wrote:
>>>>>
>>>>> >>
>>>>> >> On September 4, 2016 12:33:02 PM GMT+02:00, Tom de Vries
>>>>> >> <Tom_deVries@mentor.com> wrote:
>>>>>>
>>>>>> >>>
>>>>>> >>> On 04/09/16 08:12, Richard Biener wrote:
>>>>>>>
>>>>>>> >>>>
>>>>>>> >>>> On September 3, 2016 5:23:35 PM GMT+02:00, Tom de Vries
>>>>>>
>>>>>> >>>
>>>>>> >>> <Tom_deVries@mentor.com> wrote:
>>>>>>>>>
>>>>>>>>> >>>>>>
>>>>>>>>> >>>>>> Hi,
>>>>>>>>> >>>>>>
>>>>>>>>> >>>>>> this patch fixes a c++ ICE, a p1 6/7 regression.
>>>>>>>>> >>>>>>
>>>>>>>>> >>>>>>
>>>>>>>>> >>>>>> Consider test.C:
>>>>>>>>> >>>>>> ...
>>>>>>>>> >>>>>> void bar (__builtin_va_list &);
>>>>>>>>> >>>>>>
>>>>>>>>> >>>>>> struct c
>>>>>>>>> >>>>>> {
>>>>>>>>> >>>>>>   operator const __builtin_va_list &();
>>>>>>>>> >>>>>>   operator __builtin_va_list &();
>>>>>>>>> >>>>>> };
>>>>>>>>> >>>>>>
>>>>>>>>> >>>>>> void
>>>>>>>>> >>>>>> foo (void) {
>>>>>>>>> >>>>>>   struct c c1;
>>>>>>>>> >>>>>>
>>>>>>>>> >>>>>>   bar (c1);
>>>>>>>>> >>>>>> }
>>>>>>>>> >>>>>> ...
>>>>>>>>> >>>>>>
>>>>>>>>> >>>>>> The compiler ICEs as follows:
>>>>>>>>> >>>>>> ...
>>>>>>>>> >>>>>> test.C: In function ‘void foo()’:
>>>>>>>>> >>>>>> test.C:13:10: internal compiler error: canonical types differ
>>>>>>>>> >>>>>> for
>>>>>>>>> >>>>>> identical types __va_list_tag [1] and __va_list_tag [1]
>>>>>>>>> >>>>>>    bar (c1);
>>>>>>>>> >>>>>>           ^
>>>>>>>>> >>>>>> comptypes(tree_node*, tree_node*, int)
>>>>>>>>> >>>>>>         src/gcc/cp/typeck.c:1430
>>>>>>>>> >>>>>> reference_related_p(tree_node*, tree_node*)
>>>>>>>>> >>>>>>         src/gcc/cp/call.c:1415
>>>>>>>>> >>>>>> reference_binding
>>>>>>>>> >>>>>>         src/gcc/cp/call.c:1559
>>>>>>>>> >>>>>> implicit_conversion
>>>>>>>>> >>>>>>         src/gcc/cp/call.c:1805
>>>>>>>>> >>>>>> build_user_type_conversion_1
>>>>>>>>> >>>>>>         src/gcc/cp/call.c:3776
>>>>>>>>> >>>>>> reference_binding
>>>>>>>>> >>>>>>         src/gcc/cp/call.c:1664
>>>>>>>>> >>>>>> implicit_conversion
>>>>>>>>> >>>>>>         src/gcc/cp/call.c:1805
>>>>>>>>> >>>>>> add_function_candidate
>>>>>>>>> >>>>>>         src/gcc/cp/call.c:2141
>>>>>>>>> >>>>>> add_candidates
>>>>>>>>> >>>>>>         src/gcc/cp/call.c:5394
>>>>>>>>> >>>>>> perform_overload_resolution
>>>>>>>>> >>>>>>         src/gcc/cp/call.c:4066
>>>>>>>>> >>>>>> build_new_function_call(tree_node*, vec<tree_node*, va_gc,
>>>>>>
>>>>>> >>>
>>>>>> >>> vl_embed>**,
>>>>>>>>>
>>>>>>>>> >>>>>>
>>>>>>>>> >>>>>>   bool, int)
>>>>>>>>> >>>>>>         src/gcc/cp/call.c:4143
>>>>>>>>> >>>>>> finish_call_expr(tree_node*, vec<tree_node*, va_gc,
>>>>>>>>> >>>>>> vl_embed>**,
>>>>>>
>>>>>> >>>
>>>>>> >>> bool,
>>>>>>>>>
>>>>>>>>> >>>>>>
>>>>>>>>> >>>>>>   bool, int)
>>>>>>>>> >>>>>>         src/gcc/cp/semantics.c:2440
>>>>>>>>> >>>>>> ...
>>>>>>>>> >>>>>>
>>>>>>>>> >>>>>> The regression is caused by the commit for PR70955, that adds
>>>>>>>>> >>>>>> a
>>>>>>>>> >>>>>> "sysv_abi va_list" attribute to the struct in the va_list
>>>>>>>>> >>>>>> type
>>>>>>
>>>>>> >>>
>>>>>> >>> (which
>>>>>>>>>
>>>>>>>>> >>>>>>
>>>>>>>>> >>>>>> is
>>>>>>>>> >>>>>> an array of one of struct).
>>>>>>>>> >>>>>>
>>>>>>>>> >>>>>> The ICE in comptypes happens as follows: we're comparing two
>>>>>>
>>>>>> >>>
>>>>>> >>> versions
>>>>>>>>>
>>>>>>>>> >>>>>>
>>>>>>>>> >>>>>> of
>>>>>>>>> >>>>>> va_list type (with identical array element type), each with
>>>>>>>>> >>>>>> the
>>>>>>>>> >>>>>> canonical type set to themselves. Since the types are
>>>>>>>>> >>>>>> considered
>>>>>>>>> >>>>>> identical, they're supposed to have identical canonical
>>>>>>>>> >>>>>> types,
>>>>>>
>>>>>> >>>
>>>>>> >>> which is
>>>>>> >>>
>>>>>>>
>>>>>>> >>>> Did you figure out why they are not assigned the same canonical
>>>>>>> >>>> type?
>>>>>>
>>>>>> >>>
>>>>>> >>>
>>>>>> >>> When constructing the first type in ix86_build_builtin_va_list_64,
>>>>>> >>> it's
>>>>>> >>>
>>>>>> >>> cached.
>>>>>> >>>
>>>>>> >>> When constructing the second type in build_array_type_1 (with call
>>>>>> >>> stack: grokdeclarator -> cp_build_qualified_type_real ->
>>>>>> >>> build_cplus_array_type -> build_cplus_array_type ->
>>>>>> >>> build_array_type ->
>>>>>> >>>
>>>>>> >>> build_array_type_1), we call type_hash_canon.
>>>>>> >>>
>>>>>> >>> But the cached type has name __builtin_sysv_va_list, and the new
>>>>>> >>> type
>>>>>> >>> has no name, so we hit the clause 'TYPE_NAME (a->type) != TYPE_NAME
>>>>>> >>> (b->type)' in type_cache_hasher::equal.
>>>>>> >>>
>>>>>> >>> Consequently, TYPE_CANONICAL for the new type remain set to self.
>>>>>
>>>>> >>
>>>>> >>
>>>>> >> But how did it then work before the patch causing this?
>>>>
>>>> >
>>>> >
>>>> > Before the patch that introduced the attribute, rather than assigning
>>>> > the
>>>> > result of ix86_build_builtin_va_list_64 directly
>>>> > sysv_va_list_type_node, an
>>>> > intermediate build_variant_type_copy was used.
>>>> >
>>>> > This had as consequence that the copy was named and not added to the
>>>> > cache,
>>>> > and that the original in the type cache remained unnamed.
>>>> >
>>>> > Adding the build_variant_type_copy back fixes the ICE. But I'm not sure
>>>> > if
>>>> > that's a correct fix. The copy would have it's canonical type set to
>>>> > the
>>>> > original type. But if we'd search for the canonical type of the copy in
>>>> > the
>>>> > type cache, we wouldn't find it.
>>
>>
>>> Huh.  Can't see how making a copy would affect the type cache -- AFAIK
>>> nothing
>>> adds the record to the type hash.
>>
>>
>> Correct.
>>
>>>  The array type is there
>>
>>
>> First the array type is constructed by ix86_build_builtin_va_list_64, and
>> entered into the type cache. Then the type is assigned to
>> ms_va_list_type_node.
>>
>> Lateron, the ms_va_list_type_node is returned by ix86_enum_va_list, and
>> c_common_nodes_and_builtin calls lang_hooks.decls.pushdecl for the node:
>> ...
>>            lang_hooks.decls.pushdecl
>>             (build_decl (UNKNOWN_LOCATION,
>>                          TYPE_DECL, get_identifier (pname),
>>                          ptype));
>> ...
>> In that process it adds a name to the type node (to be precise, in
>> set_underlying_type, case DECL_IS_BUILTIN).
>>
>> If it adds the name to the original type, then we have a named type in the
>> type cache. If it adds the name to a copy of the type, then we have an
>> unnamed type in the type cache.
>
> Ok, as the backend controls what name it gets assigned in enum_va_list_p
> it seems to me the backend should assing the same name to the type in
> the first place to avoid the inconsistency?
>
> OTOH what set_underlying_type does for the DECL_IS_BUILTIN case is
> bogus if its type is already in the type cache as that changes its hash value.
> We do have a build_nonshared_array_type which ix86_build_builtin_va_list_64
> could use to avoid the type cache (not sure if that would do any good to the
> issue we face).

Yes.  It seems to me that this is the primary problem here;
set_underlying_type is treating this array type as though it's a
built-in type like 'int', but that doesn't make sense for an array
type like this that can be constructed independently, leading to the
problems we see here.  This patch seems to fix the issue:

Attachment: set_und.diff
Description: Text document