[PATCH, c++, PR77427 ] Set TYPE_STRUCTURAL_EQUALITY for sysv_abi va_list
Tom de Vries
Tom_deVries@mentor.com
Tue Sep 13 15:15:00 GMT 2016
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?
Thanks,
- Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Don-t-treat-array-as-builtin-type-in-set_underlying_type.patch
Type: text/x-patch
Size: 1247 bytes
Desc: not available
URL: <http://gcc.gnu.org/pipermail/gcc-patches/attachments/20160913/6bc82a8a/attachment.bin>
More information about the Gcc-patches
mailing list