From patchwork Sat Sep 10 08:34:18 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Tom de Vries X-Patchwork-Id: 668345 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 3sWS7304w2z9ssP for ; Sat, 10 Sep 2016 18:34:52 +1000 (AEST) Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=gcc.gnu.org header.i=@gcc.gnu.org header.b=L4csgSlC; dkim-atps=neutral DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :subject:to:references:cc:from:message-id:date:mime-version :in-reply-to:content-type; q=dns; s=default; b=XmCitbGNR/fWcBuwy wrxY+4lExC4VpQjBhQqxflkoeaNJPhoTFjCRb5+9xmJ0CDXZWuKakFuZgoSXKe9e fWYa+TbGiY8m16y7O5yWPLcR1RCkgtxtoEQ/z9PHiTBQ1rc0vSM/HbleWcpZyIuc lEkoGrSCXCYRhpQFXjOXbV8Phc= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :subject:to:references:cc:from:message-id:date:mime-version :in-reply-to:content-type; s=default; bh=UO8NLPoaFj8EX2PeuEFbbA9 gKNQ=; b=L4csgSlCmG0SZKzCJio/6Z8kKzqAExUNKSDPrrw3A1y8B5apwN521Z0 WP5GbEuF0MiElrLDKgW+nn/W1eJL0nehcO5HfUTIPpA5bA2QE69bcstLQS97r4Hv OsG6HTpJ0js5ba9dmTeMGsZ9KXJVgsEQp+/qz57CxJC42Xar5d8U= Received: (qmail 111873 invoked by alias); 10 Sep 2016 08:34:38 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org Received: (qmail 111852 invoked by uid 89); 10 Sep 2016 08:34:37 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=AWL, BAYES_00, RCVD_IN_DNSWL_NONE, SPF_PASS, URIBL_RED autolearn=ham version=3.3.2 spammy=1415, membership, ICEs, Initial X-HELO: relay1.mentorg.com Received: from relay1.mentorg.com (HELO relay1.mentorg.com) (192.94.38.131) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Sat, 10 Sep 2016 08:34:27 +0000 Received: from nat-ies.mentorg.com ([192.94.31.2] helo=SVR-IES-FEM-01.mgc.mentorg.com) by relay1.mentorg.com with esmtp id 1bidjy-0005qQ-U9 from Tom_deVries@mentor.com ; Sat, 10 Sep 2016 01:34:23 -0700 Received: from [127.0.0.1] (137.202.0.76) by SVR-IES-FEM-01.mgc.mentorg.com (137.202.0.104) with Microsoft SMTP Server id 14.3.224.2; Sat, 10 Sep 2016 09:34:21 +0100 Subject: Re: [PATCH, c++, PR77427 ] Set TYPE_STRUCTURAL_EQUALITY for sysv_abi va_list To: Richard Biener References: <0c832b5f-9fb8-7fb7-db89-2b504d88b0e9@mentor.com> <48b8f32b-c1e6-926e-1ed4-f50d8d4d02ef@mentor.com> <77ad05e5-f50a-9d00-09ee-5fd442c7278d@mentor.com> CC: GCC Patches From: Tom de Vries Message-ID: <5fc30b21-cd3c-0864-b9c9-3fbffe180d9b@mentor.com> Date: Sat, 10 Sep 2016 10:34:18 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.2.0 MIME-Version: 1.0 In-Reply-To: On 07-09-16 11:22, Richard Biener wrote: > On Mon, Sep 5, 2016 at 6:11 PM, Tom de Vries wrote: >> On 05/09/16 09:49, Richard Biener wrote: >>> >>> On Sun, Sep 4, 2016 at 11:30 PM, Tom de Vries >>> wrote: >>>> >>>>> On 04/09/16 16:08, Richard Biener wrote: >>>>> >>>>>>> >>>>>>> On September 4, 2016 12:33:02 PM GMT+02:00, Tom de Vries >>>>>>> wrote: >>>>>> >>>>>>>>> >>>>>>>>> On 04/09/16 08:12, Richard Biener wrote: >>>>>>> >>>>>>>>>>> >>>>>>>>>>> On September 3, 2016 5:23:35 PM GMT+02:00, Tom de Vries >>>>>> >>>>>>>>> >>>>>>>>> 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>>>>> >>>>>>>>> >>>>>>>>> vl_embed>**, >>>>>>>>> >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> bool, int) >>>>>>>>>>>>>>> src/gcc/cp/call.c:4143 >>>>>>>>>>>>>>> finish_call_expr(tree_node*, vec>>>>>>>>>>>>>> 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: ... ... 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 Build variant type copies for sysv_va_list_node and ms_va_list_type_node 2016-09-10 Tom de Vries PR c++/77427 * config/i386/i386.c (ix86_build_builtin_va_list): Build variant type copies for sysv_va_list_node and ms_va_list_type_node. * g++.dg/pr77427.C: New test. --- gcc/config/i386/i386.c | 12 ++++++++++++ gcc/testsuite/g++.dg/pr77427.C | 17 +++++++++++++++++ 2 files changed, 29 insertions(+) diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c index dc92f9b..a0d50a7 100644 --- a/gcc/config/i386/i386.c +++ b/gcc/config/i386/i386.c @@ -10591,6 +10591,18 @@ ix86_build_builtin_va_list (void) TYPE_ATTRIBUTES (char_ptr_type)); ms_va_list_type_node = build_type_attribute_variant (char_ptr_type, attr); + /* We'll return these type nodes via enum_va_list, and the caller + c_common_nodes_and_builtins will modify it by setting the TYPE_NAME. + This modification causes the type to move from one equivalence class + defined by type_cache_hasher::equal to another. As a consequence, when + building an unnamed version of the va_list, and trying to find the + cached variant, we'll won't find it, and the new type will have + TYPE_CANONICAL set to self and not match with the cached type. + So, we build a variant type copy, which is not in the type cache and + can be modified. */ + sysv_va_list_type_node = build_variant_type_copy (sysv_va_list_type_node); + ms_va_list_type_node = build_variant_type_copy (ms_va_list_type_node); + return ((ix86_abi == MS_ABI) ? ms_va_list_type_node : sysv_va_list_type_node); 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); +}