From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx2.suse.de (mx2.suse.de [195.135.220.15]) by sourceware.org (Postfix) with ESMTPS id BC01C3857C52 for ; Thu, 1 Apr 2021 07:56:24 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org BC01C3857C52 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=suse.de Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=rguenther@suse.de X-Virus-Scanned: by amavisd-new at test-mx.suse.de Received: from relay2.suse.de (unknown [195.135.221.27]) by mx2.suse.de (Postfix) with ESMTP id 9BE5AB1F1; Thu, 1 Apr 2021 07:56:23 +0000 (UTC) Date: Thu, 1 Apr 2021 09:56:23 +0200 (CEST) From: Richard Biener To: Jakub Jelinek cc: Jason Merrill , gcc-patches@gcc.gnu.org Subject: Re: [PATCH] c++, abi: Fix abi_tag attribute handling [PR98481] In-Reply-To: <20210401054729.GV1179226@tucnak> Message-ID: References: <20210107164718.GP725145@tucnak> <07f1435c-44ce-4b00-5491-a9f07a547148@redhat.com> <20210108192905.GY725145@tucnak> <03eaf1db-d40d-5aa7-fc34-478bb97c0efa@redhat.com> <20210401054729.GV1179226@tucnak> User-Agent: Alpine 2.21 (LSU 202 2017-01-01) MIME-Version: 1.0 X-Spam-Status: No, score=-11.4 required=5.0 tests=BAYES_00, GIT_PATCH_0, KAM_DMARC_STATUS, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: 8BIT X-Content-Filtered-By: Mailman/MimeDel 2.1.29 X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 01 Apr 2021 07:56:27 -0000 On Thu, 1 Apr 2021, Jakub Jelinek wrote: > On Thu, Apr 01, 2021 at 12:52:20AM -0400, Jason Merrill wrote: > > On 1/8/21 2:29 PM, Jakub Jelinek wrote: > > > On Fri, Jan 08, 2021 at 02:22:59PM -0500, Jason Merrill wrote: > > > > I like the idea to use *walk_subtrees to distinguish between walking > > > > syntactic subtrees and walking type-identity subtrees. But it should be > > > > more general; how does this look to you? > > > > > > LGTM, thanks. > > > > As discussed on IRC, we probably want to fix this in GCC 10 as well, but not > > by default. The first patch adds ABI v15 and fixes the bug for !v14, so > > -fabi-version=0 has the fix, but the default behavior does not. The second > > patch adds ABI v15 on trunk. > > > > It would be nice to make -Wabi/-fabi-compat-version handle this case, but > > that would require a bunch of new code unsuitable for this point in the > > process. > > > > Does this make sense to you? > > LGTM, thanks. LGTM as well. I'd like to roll RC1 today around 2pm CEST, so I'm going to give this a round of testing myself (also noticing the corresponding trunk patch isn't in yet) So if you can manage to push it before this time that would be great, otherwise I guess I'm going to push it to 10. Richard. > > commit 123f254cce416a4d50445465b88af6d8e754dc5e > > Author: Jakub Jelinek > > Date: Thu Jan 7 17:47:18 2021 +0100 > > > > c++, abi: Fix abi_tag attribute handling [PR98481] > > > > In GCC10 cp_walk_subtrees has been changed to walk template arguments. > > As the following testcase, that changed the mangling of some functions. > > I believe the previous behavior that find_abi_tags_r doesn't recurse into > > template args has been the correct one, but setting *walk_subtrees = 0 > > for the types and handling the types subtree walking manually in > > find_abi_tags_r looks too hard, there are a lot of subtrees and details what > > should and shouldn't be walked, both in tree.c (walk_type_fields there, > > which is static) and in cp_walk_subtrees itself. > > > > The following patch abuses the fact that *walk_subtrees is an int to > > tell cp_walk_subtrees it shouldn't walk the template args. > > > > But we don't want to introduce an ABI change in the middle of the GCC 10 > > cycle, so the GCC 10 version of this patch introduces ABI v15 for the fix, > > which will be available but not default in GCC 10.3. > > > > Co-authored-by: Jason Merrill > > > > gcc/cp/ChangeLog: > > > > PR c++/98481 > > * class.c (find_abi_tags_r): Set *walk_subtrees to 2 instead of 1 > > for types. > > (mark_abi_tags_r): Likewise. > > * tree.c (cp_walk_subtrees): If *walk_subtrees_p is 2, look through > > typedefs. > > > > gcc/testsuite/ChangeLog: > > > > PR c++/98481 > > * g++.dg/abi/abi-tag24.C: New test. > > * g++.dg/abi/abi-tag24a.C: New test. > > * g++.dg/abi/macro0.C: Adjust expected value. > > > > gcc/ChangeLog: > > > > PR c++/98481 > > * common.opt (fabi-version): Default to 14. > > > > gcc/c-family/ChangeLog: > > > > PR c++/98481 > > * c-opts.c (c_common_post_options): Bump latest_abi_version. > > > > diff --git a/gcc/common.opt b/gcc/common.opt > > index 9cc47b16cac..ec5235c3a41 100644 > > --- a/gcc/common.opt > > +++ b/gcc/common.opt > > @@ -956,10 +956,13 @@ Driver Undocumented > > ; 14: Corrects the mangling of nullptr expression. > > ; Default in G++ 10. > > ; > > +; 15: Corrects G++ 10 ABI tag regression [PR98481]. > > +; Available, but not default, in G++ 10.3. > > +; > > ; Additional positive integers will be assigned as new versions of > > ; the ABI become the default version of the ABI. > > fabi-version= > > -Common Joined RejectNegative UInteger Var(flag_abi_version) Init(0) > > +Common Joined RejectNegative UInteger Var(flag_abi_version) Init(14) > > The version of the C++ ABI in use. > > > > faggressive-loop-optimizations > > diff --git a/gcc/c-family/c-opts.c b/gcc/c-family/c-opts.c > > index 58ba0948e79..c51d6d34726 100644 > > --- a/gcc/c-family/c-opts.c > > +++ b/gcc/c-family/c-opts.c > > @@ -943,7 +943,7 @@ c_common_post_options (const char **pfilename) > > > > /* Change flag_abi_version to be the actual current ABI level, for the > > benefit of c_cpp_builtins, and to make comparison simpler. */ > > - const int latest_abi_version = 14; > > + const int latest_abi_version = 15; > > /* Generate compatibility aliases for ABI v11 (7.1) by default. */ > > const int abi_compat_default = 11; > > > > diff --git a/gcc/cp/class.c b/gcc/cp/class.c > > index ed8f9527929..c0101130ba3 100644 > > --- a/gcc/cp/class.c > > +++ b/gcc/cp/class.c > > @@ -1450,6 +1450,10 @@ mark_or_check_tags (tree t, tree *tp, abi_tag_data *p, bool val) > > static tree > > find_abi_tags_r (tree *tp, int *walk_subtrees, void *data) > > { > > + if (TYPE_P (*tp) && *walk_subtrees == 1 && flag_abi_version != 14) > > + /* Tell cp_walk_subtrees to look though typedefs. [PR98481] */ > > + *walk_subtrees = 2; > > + > > if (!OVERLOAD_TYPE_P (*tp)) > > return NULL_TREE; > > > > @@ -1470,6 +1474,10 @@ find_abi_tags_r (tree *tp, int *walk_subtrees, void *data) > > static tree > > mark_abi_tags_r (tree *tp, int *walk_subtrees, void *data) > > { > > + if (TYPE_P (*tp) && *walk_subtrees == 1 && flag_abi_version != 14) > > + /* Tell cp_walk_subtrees to look though typedefs. */ > > + *walk_subtrees = 2; > > + > > if (!OVERLOAD_TYPE_P (*tp)) > > return NULL_TREE; > > > > diff --git a/gcc/cp/tree.c b/gcc/cp/tree.c > > index b36ca4eddc0..10b818d1370 100644 > > --- a/gcc/cp/tree.c > > +++ b/gcc/cp/tree.c > > @@ -5055,8 +5055,18 @@ cp_walk_subtrees (tree *tp, int *walk_subtrees_p, walk_tree_fn func, > > while (0) > > > > if (TYPE_P (*tp)) > > - /* Walk into template args without looking through typedefs. */ > > - if (tree ti = TYPE_TEMPLATE_INFO_MAYBE_ALIAS (*tp)) > > + /* If *WALK_SUBTREES_P is 1, we're interested in the syntactic form of > > + the argument, so don't look through typedefs, but do walk into > > + template arguments for alias templates (and non-typedefed classes). > > + > > + If *WALK_SUBTREES_P > 1, we're interested in type identity or > > + equivalence, so look through typedefs, ignoring template arguments for > > + alias templates, and walk into template args of classes. > > + > > + See find_abi_tags_r for an example of setting *WALK_SUBTREES_P to 2 > > + when that's the behavior the walk_tree_fn wants. */ > > + if (tree ti = (*walk_subtrees_p > 1 ? TYPE_TEMPLATE_INFO (*tp) > > + : TYPE_TEMPLATE_INFO_MAYBE_ALIAS (*tp))) > > WALK_SUBTREE (TI_ARGS (ti)); > > > > /* Not one of the easy cases. We must explicitly go through the > > diff --git a/gcc/testsuite/g++.dg/abi/abi-tag24.C b/gcc/testsuite/g++.dg/abi/abi-tag24.C > > new file mode 100644 > > index 00000000000..2c5c542bfcd > > --- /dev/null > > +++ b/gcc/testsuite/g++.dg/abi/abi-tag24.C > > @@ -0,0 +1,18 @@ > > +// PR c++/98481 > > +// { dg-do compile { target c++11 } } > > +// { dg-additional-options -fabi-version=0 } > > +inline namespace N __attribute ((__abi_tag__ ("myabi"))) > > +{ > > + struct A {}; > > +} > > +template > > +struct B { typedef int size_type; }; > > +struct S1 { B::size_type foo () const { return 1; } }; > > +struct S2 { B::size_type foo () const; }; > > +int S2::foo () const { return 2; } > > +int (S1::*f1) () const = &S1::foo; > > +int (S2::*f2) () const = &S2::foo; > > + > > +// { dg-final { scan-assembler "_ZNK2S13fooEv" } } > > +// { dg-final { scan-assembler "_ZNK2S23fooEv" } } > > +// { dg-final { scan-assembler-not "_ZNK2S13fooB5myabiEv" } } > > diff --git a/gcc/testsuite/g++.dg/abi/abi-tag24a.C b/gcc/testsuite/g++.dg/abi/abi-tag24a.C > > new file mode 100644 > > index 00000000000..83f930dfdde > > --- /dev/null > > +++ b/gcc/testsuite/g++.dg/abi/abi-tag24a.C > > @@ -0,0 +1,18 @@ > > +// PR c++/98481 > > +// { dg-do compile { target c++11 } } > > +// { dg-additional-options -fabi-version=14 } > > +inline namespace N __attribute ((__abi_tag__ ("myabi"))) > > +{ > > + struct A {}; > > +} > > +template > > +struct B { typedef int size_type; }; > > +struct S1 { B::size_type foo () const { return 1; } }; > > +struct S2 { B::size_type foo () const; }; > > +int S2::foo () const { return 2; } > > +int (S1::*f1) () const = &S1::foo; > > +int (S2::*f2) () const = &S2::foo; > > + > > +// { dg-final { scan-assembler-not "_ZNK2S13fooEv" } } > > +// { dg-final { scan-assembler "_ZNK2S23fooEv" } } > > +// { dg-final { scan-assembler "_ZNK2S13fooB5myabiEv" } } > > diff --git a/gcc/testsuite/g++.dg/abi/macro0.C b/gcc/testsuite/g++.dg/abi/macro0.C > > index 08106004c4d..7c3c17051ed 100644 > > --- a/gcc/testsuite/g++.dg/abi/macro0.C > > +++ b/gcc/testsuite/g++.dg/abi/macro0.C > > @@ -1,6 +1,6 @@ > > // This testcase will need to be kept in sync with c_common_post_options. > > // { dg-options "-fabi-version=0" } > > > > -#if __GXX_ABI_VERSION != 1014 > > +#if __GXX_ABI_VERSION != 1015 > > #error "Incorrect value of __GXX_ABI_VERSION" > > #endif > > > commit 02ad075e2058c6c4f6cf05606653981c5efb4d0d > > Author: Jason Merrill > > Date: Wed Mar 31 17:48:50 2021 -0400 > > > > c++: Add ABI version for PR98481 fix > > > > The PR98481 fix corrects an ABI regression in GCC 10, but we don't want to > > introduce an ABI change in the middle of the GCC 10 cycle. This patch > > introduces ABI v15 for the fix, which will be available but not default in > > GCC 10.3; the broken behavior remains in ABI v14. Compatibility aliases > > will not be generated for this change. > > > > gcc/ChangeLog: > > > > PR c++/98481 > > * common.opt: Document v15 and v16. > > > > gcc/c-family/ChangeLog: > > > > PR c++/98481 > > * c-opts.c (c_common_post_options): Bump latest_abi_version. > > > > gcc/cp/ChangeLog: > > > > PR c++/98481 > > * mangle.c (write_expression): Adjust. > > * class.c (find_abi_tags_r): Disable PR98481 fix for ABI v14. > > (mark_abi_tags_r): Likewise. > > > > gcc/testsuite/ChangeLog: > > > > PR c++/98481 > > * g++.dg/abi/abi-tag24a.C: New test. > > * g++.dg/abi/macro0.C: Adjust expected value. > > > > diff --git a/gcc/common.opt b/gcc/common.opt > > index c75dd36843e..a75b44ee47e 100644 > > --- a/gcc/common.opt > > +++ b/gcc/common.opt > > @@ -960,7 +960,11 @@ Driver Undocumented > > ; 14: Corrects the mangling of nullptr expression. > > ; Default in G++ 10. > > ; > > -; 15: Changes the mangling of __alignof__ to be distinct from that of alignof. > > +; 15: Corrects G++ 10 ABI tag regression [PR98481]. > > +; Available, but not default, in G++ 10.3. > > +; > > +; 16: Changes the mangling of __alignof__ to be distinct from that of alignof. > > +; Adds missing 'on' in mangling of operator names in some cases. > > ; Default in G++ 11. > > ; > > ; Additional positive integers will be assigned as new versions of > > diff --git a/gcc/c-family/c-opts.c b/gcc/c-family/c-opts.c > > index bd15b9cd902..89e05a4c551 100644 > > --- a/gcc/c-family/c-opts.c > > +++ b/gcc/c-family/c-opts.c > > @@ -965,7 +965,7 @@ c_common_post_options (const char **pfilename) > > > > /* Change flag_abi_version to be the actual current ABI level, for the > > benefit of c_cpp_builtins, and to make comparison simpler. */ > > - const int latest_abi_version = 15; > > + const int latest_abi_version = 16; > > /* Generate compatibility aliases for ABI v11 (7.1) by default. */ > > const int abi_compat_default = 11; > > > > diff --git a/gcc/cp/class.c b/gcc/cp/class.c > > index 856e81e3d1a..4bffec4a707 100644 > > --- a/gcc/cp/class.c > > +++ b/gcc/cp/class.c > > @@ -1494,8 +1494,8 @@ mark_or_check_tags (tree t, tree *tp, abi_tag_data *p, bool val) > > static tree > > find_abi_tags_r (tree *tp, int *walk_subtrees, void *data) > > { > > - if (TYPE_P (*tp) && *walk_subtrees == 1) > > - /* Tell cp_walk_subtrees to look though typedefs. */ > > + if (TYPE_P (*tp) && *walk_subtrees == 1 && flag_abi_version != 14) > > + /* Tell cp_walk_subtrees to look though typedefs. [PR98481] */ > > *walk_subtrees = 2; > > > > if (!OVERLOAD_TYPE_P (*tp)) > > @@ -1518,7 +1518,7 @@ find_abi_tags_r (tree *tp, int *walk_subtrees, void *data) > > static tree > > mark_abi_tags_r (tree *tp, int *walk_subtrees, void *data) > > { > > - if (TYPE_P (*tp) && *walk_subtrees == 1) > > + if (TYPE_P (*tp) && *walk_subtrees == 1 && flag_abi_version != 14) > > /* Tell cp_walk_subtrees to look though typedefs. */ > > *walk_subtrees = 2; > > > > diff --git a/gcc/cp/mangle.c b/gcc/cp/mangle.c > > index 57ce9a6710f..6c111342b97 100644 > > --- a/gcc/cp/mangle.c > > +++ b/gcc/cp/mangle.c > > @@ -3119,9 +3119,9 @@ write_expression (tree expr) > > { > > if (!ALIGNOF_EXPR_STD_P (expr)) > > { > > - if (abi_warn_or_compat_version_crosses (15)) > > + if (abi_warn_or_compat_version_crosses (16)) > > G.need_abi_warning = true; > > - if (abi_version_at_least (15)) > > + if (abi_version_at_least (16)) > > { > > /* We used to mangle __alignof__ like alignof. */ > > write_string ("u11__alignof__"); > > @@ -3350,9 +3350,9 @@ write_expression (tree expr) > > tree name = dependent_name (expr); > > if (IDENTIFIER_ANY_OP_P (name)) > > { > > - if (abi_version_at_least (15)) > > + if (abi_version_at_least (16)) > > write_string ("on"); > > - if (abi_warn_or_compat_version_crosses (15)) > > + if (abi_warn_or_compat_version_crosses (16)) > > G.need_abi_warning = 1; > > } > > write_unqualified_id (name); > > diff --git a/gcc/testsuite/g++.dg/abi/abi-tag24a.C b/gcc/testsuite/g++.dg/abi/abi-tag24a.C > > new file mode 100644 > > index 00000000000..83f930dfdde > > --- /dev/null > > +++ b/gcc/testsuite/g++.dg/abi/abi-tag24a.C > > @@ -0,0 +1,18 @@ > > +// PR c++/98481 > > +// { dg-do compile { target c++11 } } > > +// { dg-additional-options -fabi-version=14 } > > +inline namespace N __attribute ((__abi_tag__ ("myabi"))) > > +{ > > + struct A {}; > > +} > > +template > > +struct B { typedef int size_type; }; > > +struct S1 { B::size_type foo () const { return 1; } }; > > +struct S2 { B::size_type foo () const; }; > > +int S2::foo () const { return 2; } > > +int (S1::*f1) () const = &S1::foo; > > +int (S2::*f2) () const = &S2::foo; > > + > > +// { dg-final { scan-assembler-not "_ZNK2S13fooEv" } } > > +// { dg-final { scan-assembler "_ZNK2S23fooEv" } } > > +// { dg-final { scan-assembler "_ZNK2S13fooB5myabiEv" } } > > diff --git a/gcc/testsuite/g++.dg/abi/macro0.C b/gcc/testsuite/g++.dg/abi/macro0.C > > index 7c3c17051ed..f25f291dba6 100644 > > --- a/gcc/testsuite/g++.dg/abi/macro0.C > > +++ b/gcc/testsuite/g++.dg/abi/macro0.C > > @@ -1,6 +1,6 @@ > > // This testcase will need to be kept in sync with c_common_post_options. > > // { dg-options "-fabi-version=0" } > > > > -#if __GXX_ABI_VERSION != 1015 > > +#if __GXX_ABI_VERSION != 1016 > > #error "Incorrect value of __GXX_ABI_VERSION" > > #endif > > > Jakub > > -- Richard Biener SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg, Germany; GF: Felix Imendörffer; HRB 36809 (AG Nuernberg)