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

[Jason Merrill <jason at redhat dot com>]

On Tue, Sep 13, 2016 at 11:10 AM, Tom de Vries <Tom_deVries@mentor.com> wrote:
> On 13/09/16 03:36, Jason Merrill wrote:
>>
>> 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:
>>
>>
>> set_und.diff
>>
>>
>
> Bootstrapped and reg-tested the patch on x86_64.
>
> OK for trunk, 6 branch?

Yes.

Jason