| Message ID | 0c832b5f-9fb8-7fb7-db89-2b504d88b0e9@mentor.com |
|---|---|
| State | New |
| Headers | show |
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? Richard. >not the case, and we run into the ICE. > >The patch fixes the ICE by setting the canonical type of the va_list >struct and array to NULL, forcing structural comparison. > > >This fix causes a regression for test2.C: >... >template <typename T> >int >fn7 (T ap) >{ > return __builtin_va_arg(ap, int); >} > >int >fn8 (__builtin_va_list ap) >{ > return fn7<__builtin_va_list> (ap); >} >... > >It causes this warning to appear: >... >test2.C: In function ‘int fn8(__va_list_tag*)’: >test2.C:11:36: warning: ignoring attributes applied to ‘__va_list_tag’ >after definition [-Wattributes] > return fn7<__builtin_va_list> (ap); >... > >The patch removes the warning by considering the attribute on the >va_list struct type as engrained in apply_identity_attributes. > > >Bootstrapped and reg-tested on x86_64. > >OK for trunk, 6 branch? > >Thanks, >- Tom
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. Thanks, - Tom
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? Richard. >Thanks, >- Tom
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. Thanks, - Tom
Set TYPE_STRUCTURAL_EQUALITY for sysv_abi va_list 2016-09-03 Tom de Vries <tom@codesourcery.com> PR c++/77427 * config/i386/i386.c (ix86_build_builtin_va_list_64): Set TYPE_STRUCTURAL_EQUALITY for record and array. * tree.c (apply_identity_attributes): Consider attributes ingrained on records and unions. * g++.dg/pr77427.C: New test. --- gcc/config/i386/i386.c | 6 +++++- gcc/cp/tree.c | 3 ++- gcc/testsuite/g++.dg/pr77427.C | 17 +++++++++++++++++ 3 files changed, 24 insertions(+), 2 deletions(-) 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); /* 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); + + return res; } /* Setup the builtin va_list data type and for 64-bit the additional diff --git a/gcc/cp/tree.c b/gcc/cp/tree.c index 6d254dd..0259bbf 100644 --- a/gcc/cp/tree.c +++ b/gcc/cp/tree.c @@ -1244,7 +1244,8 @@ apply_identity_attributes (tree result, tree attribs, bool *remove_attributes) tree new_attribs = NULL_TREE; tree *p = &new_attribs; - if (OVERLOAD_TYPE_P (result)) + if (OVERLOAD_TYPE_P (result) + || RECORD_OR_UNION_TYPE_P (result)) { /* On classes and enums all attributes are ingrained. */ gcc_assert (attribs == TYPE_ATTRIBUTES (result)); 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); +}
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 not the case, and we run into the ICE. The patch fixes the ICE by setting the canonical type of the va_list struct and array to NULL, forcing structural comparison. This fix causes a regression for test2.C: ... template <typename T> int fn7 (T ap) { return __builtin_va_arg(ap, int); } int fn8 (__builtin_va_list ap) { return fn7<__builtin_va_list> (ap); } ... It causes this warning to appear: ... test2.C: In function ‘int fn8(__va_list_tag*)’: test2.C:11:36: warning: ignoring attributes applied to ‘__va_list_tag’ after definition [-Wattributes] return fn7<__builtin_va_list> (ap); ... The patch removes the warning by considering the attribute on the va_list struct type as engrained in apply_identity_attributes. Bootstrapped and reg-tested on x86_64. OK for trunk, 6 branch? Thanks, - Tom