| Message ID | CAFiYyc0YDVgjsAOJMNaiM3emj+xYOc6akiVZjnE_ud8v1ij8jg@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
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. > (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. > Ah, right. > I'm fine with that part The easiest fix seems to be to add back the build_variant_type_copy. But that fix doesn't agree with my understanding of how a type cache should work. I'd expect for each type t, either: - TYPE_CANONICAL (t) == NULL_TREE, or - type_hash_canon (hash_code_of_type(t), t) == TYPE_CANONICAL (t) Am I mistaken to think that this should be an invariant, or do we exploit membership/non-membership of the cache to work around certain issues? Any comments appreciated. > but the cp/tree.c part looks very generic, excluding > all records from handling. I'm caught here between the following: - the sysv_abi va_list attribute is marked as not affecting type identity, otherwise we run into the mangle.c:write_type error ( https://gcc.gnu.org/ml/gcc-patches/2016-08/msg01804.html ) - because sysv_abi va_list attribute is marked as not affecting type identity, when canonicalize_type_argument is called with __builtin_va_list, it attempts to strip the attribute from the underlying struct. - when trying to construct a type variant of the struct without the attribute, we run into the warning (and return the type without the attributes stripped). I'm not sure how this should be fixed. Silence the warning? Really strip the attributes? Thanks, - Tom
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). >> (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. >> > > Ah, right. > >> I'm fine with that part > > > The easiest fix seems to be to add back the build_variant_type_copy. But > that fix doesn't agree with my understanding of how a type cache should > work. > > I'd expect for each type t, either: > - TYPE_CANONICAL (t) == NULL_TREE, or > - type_hash_canon (hash_code_of_type(t), t) == TYPE_CANONICAL (t) > > Am I mistaken to think that this should be an invariant, or do we exploit > membership/non-membership of the cache to work around certain issues? Any > comments appreciated. TYPE_CANONICAL isn't really related to the type cache. type_hash_canon is used to avoid having two copies of _exactly_ the same type (type tree nodes are shared). >> but the cp/tree.c part looks very generic, excluding >> >> all records from handling. > > > I'm caught here between the following: > - the sysv_abi va_list attribute is marked as not affecting type > identity, otherwise we run into the mangle.c:write_type error > ( https://gcc.gnu.org/ml/gcc-patches/2016-08/msg01804.html ) > - because sysv_abi va_list attribute is marked as not affecting type > identity, when canonicalize_type_argument is called with > __builtin_va_list, it attempts to strip the attribute from the > underlying struct. > - when trying to construct a type variant of the struct without the > attribute, we run into the warning (and return the type without > the attributes stripped). > > I'm not sure how this should be fixed. Silence the warning? Really strip the > attributes? Maybe it needs to be a new "kind" of attribute then ... it's part of the type and you can't really "strip" it. Which is also why it is on the main variant. So maybe we can use that fact to avoid the stripping and thus the warning? That is, never strip anything for the main variant? Richard. > Thanks, > - Tom
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); +