This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH, c++, PR77427 ] Set TYPE_STRUCTURAL_EQUALITY for sysv_abi va_list

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
Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]