| Message ID | eb17281a-ce23-67c6-1ae5-33e314dc0c28@mentor.com |
|---|---|
| State | New |
| Headers | show |
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
Don't treat array as builtin type in set_underlying_type 2016-09-13 Jason Merrill <jason@redhat.com> Tom de Vries <tom@codesourcery.com> PR c++/77427 * c-common.c (set_underlying_type): Don't treat array as builtin type. * g++.dg/pr77427.C: New test. --- gcc/c-family/c-common.c | 2 +- gcc/testsuite/g++.dg/pr77427.C | 17 +++++++++++++++++ 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c index 9b5e016..825e63a 100644 --- a/gcc/c-family/c-common.c +++ b/gcc/c-family/c-common.c @@ -12303,7 +12303,7 @@ set_underlying_type (tree x) { if (x == error_mark_node) return; - if (DECL_IS_BUILTIN (x)) + if (DECL_IS_BUILTIN (x) && TREE_CODE (TREE_TYPE (x)) != ARRAY_TYPE) { if (TYPE_NAME (TREE_TYPE (x)) == 0) TYPE_NAME (TREE_TYPE (x)) = x; diff --git a/gcc/testsuite/g++.dg/pr77427.C b/gcc/testsuite/g++.dg/pr77427.C new file mode 100644 index 0000000..544946d --- /dev/null +++ b/gcc/testsuite/g++.dg/pr77427.C @@ -0,0 +1,17 @@ +// { dg-do compile } + +void bar (__builtin_va_list &); + +struct c +{ + operator const __builtin_va_list &(); + operator __builtin_va_list &(); +}; + +void +foo (void) +{ + struct c c1; + + bar (c1); +}