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

[Jason Merrill <jason@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 764 bytes --]

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?

[-- Attachment #2: pr98481-gcc10.diff --]
[-- Type: text/x-patch, Size: 7388 bytes --]

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

[-- Attachment #3: pr98481-abi15.diff --]
[-- Type: text/x-patch, Size: 5567 bytes --]

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