[PATCH, c++, PR77427 ] Set TYPE_STRUCTURAL_EQUALITY for sysv_abi va_list
Tom de Vries
Tom_deVries@mentor.com
Sat Sep 10 08:58:00 GMT 2016
On 07-09-16 11:22, Richard Biener 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?
>
I don't know what the reason is to split out type creation and naming like this.
But AFAIU it's not the core issue, so I'm leaving it like that for the moment.
We need an unnamed version of the type in the type cache in order for the cache
lookup of the type constructed during grok_declarator to succeed. So assigning
the name directly to the constructed type is not going to fix that.
> 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.
At least for these two va_list types, it does not change the hash value
(TYPE_NAME is not taken into account), but indeed it does change in which
equivalence class defined by type_cache_hasher::equal it falls, which should not
happen. [ Well, strictly speaking, if we add a type to the cache, change it's
name immediately, ensure that the hash is still the same, it's allowed, because
the type hasn't been used yet. ]
FWIW, I've created a simple patch to detect that the typename of a cached type
changes:
...
diff --git a/gcc/tree.c b/gcc/tree.c
index 33e6f97..f003ae1 100644
--- a/gcc/tree.c
+++ b/gcc/tree.c
@@ -162,6 +162,7 @@ static GTY(()) int next_debug_decl_uid;
struct GTY((for_user)) type_hash {
unsigned long hash;
tree type;
+ tree orig_type;
};
/* Initial size of the hash table (rounded to next prime). */
@@ -6987,6 +6988,14 @@ type_hash_list (const_tree list, inchash::hash &hstate)
bool
type_cache_hasher::equal (type_hash *a, type_hash *b)
{
+ if (a->type != a->orig_type
+ && TREE_CODE (a->type) != COMPLEX_TYPE)
+ gcc_assert (TYPE_NAME(a->type) == TYPE_NAME(a->orig_type));
+
+ if (b->type != b->orig_type
+ && TREE_CODE (b->type) != COMPLEX_TYPE)
+ gcc_assert (TYPE_NAME(b->type) == TYPE_NAME(b->orig_type));
+
/* First test the things that are the same for all types. */
if (a->hash != b->hash
|| TREE_CODE (a->type) != TREE_CODE (b->type)
@@ -7118,6 +7127,7 @@ type_hash_canon (unsigned int hashcode, tree type)
in.hash = hashcode;
in.type = type;
+ in.orig_type = type;
loc = type_hash_table->find_slot_with_hash (&in, hashcode, INSERT);
if (*loc)
@@ -7134,6 +7144,7 @@ type_hash_canon (unsigned int hashcode, tree type)
h = ggc_alloc<type_hash> ();
h->hash = hashcode;
h->type = type;
+ h->orig_type = copy_node (type);
*loc = h;
return type;
...
The patch triggered for both va_list types, but also for JUMPBUF_T in ada, and
I'm not sure how serious that one is.
> 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).
>
I see, thanks.
>>> 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?
>
I've now managed to convince myself that doing a build_variant_type_copy is a
good fix.
It's similar to what is done in build_common_tree_nodes:
...
{
tree t = targetm.build_builtin_va_list ();
/* Many back-ends define record types without setting TYPE_NAME.
If we copied the record type here, we'd keep the original
record type without a name. This breaks name mangling. So,
don't copy record types and let c_common_nodes_and_builtins()
declare the type to be __builtin_va_list. */
if (TREE_CODE (t) != RECORD_TYPE)
t = build_variant_type_copy (t);
va_list_type_node = t;
}
...
The patch adds a build_variant_type_copy for both va_list types, even though the
problem only occurred for sysv_va_list_type_node, because the problem as such
exists for both.
Bootstrapped and reg-tested on x86_64.
OK for trunk, 6 branch?
Thanks,
- Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Build-variant-type-copies-for-sysv_va_list_node-and-ms_va_list_type_node.patch
Type: text/x-patch
Size: 2031 bytes
Desc: not available
URL: <http://gcc.gnu.org/pipermail/gcc-patches/attachments/20160910/2520680a/attachment.bin>
More information about the Gcc-patches
mailing list