On Sun, Sep 4, 2016 at 11:30 PM, Tom de Vries <tom_devr...@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_devr...@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_devr...@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. The array type is there (and the
FEs have some
"interesting" code regarding qualifiers on arrays). Btw,
diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
index 4531647..4e00dee 100644
--- a/gcc/config/i386/i386.c
+++ b/gcc/config/i386/i386.c
@@ -10550,9 +10550,13 @@ ix86_build_builtin_va_list_64 (void)
TYPE_ATTRIBUTES (record) = tree_cons (get_identifier ("sysv_abi va_list"),
NULL_TREE, TYPE_ATTRIBUTES (record));
+ SET_TYPE_STRUCTURAL_EQUALITY (record);
This should be enough
/* The correct type is an array type of one element. */
- return build_array_type (record, build_index_type (size_zero_node));
+ tree res = build_array_type (record, build_index_type (size_zero_node));
+ SET_TYPE_STRUCTURAL_EQUALITY (res);
+
and this should be done automagically by build_array_type.
I'm fine with that part but the cp/tree.c part looks very generic, excluding
all records from handling.
Richard.
+ return res;
}
> Thanks,
> - Tom
>
