Re: [PATCH] c++, abi: Fix abi_tag attribute handling [PR98481]

[Richard Biener <rguenther@suse.de>]

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 <jakub@redhat.com>
> > 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 <jason@redhat.com>
> >     
> >     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 <typename T>
> > +struct B { typedef int size_type; };
> > +struct S1 { B<A>::size_type foo () const { return 1; } };
> > +struct S2 { B<A>::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 <typename T>
> > +struct B { typedef int size_type; };
> > +struct S1 { B<A>::size_type foo () const { return 1; } };
> > +struct S2 { B<A>::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 <jason@redhat.com>
> > 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 <typename T>
> > +struct B { typedef int size_type; };
> > +struct S1 { B<A>::size_type foo () const { return 1; } };
> > +struct S2 { B<A>::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 <rguenther@suse.de>
SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
Germany; GF: Felix Imendörffer; HRB 36809 (AG Nuernberg)