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

[Jakub Jelinek <jakub@redhat.com>]

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.

> 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