[PATCH] x86_64: Issue -Wpsabi warning about C++ zero width bitfield ABI changes [PR102024]

Jakub Jelinek jakub@redhat.com
Tue Aug 31 08:05:58 GMT 2021


Hi!

This is an incremental patch to
https://gcc.gnu.org/pipermail/gcc-patches/2021-August/578447.html
for x86_64 ABI.
For zero-width bitfields current GCC classify_argument does:
                  if (DECL_BIT_FIELD (field))
                    {
                      for (i = (int_bit_position (field)
                                + (bit_offset % 64)) / 8 / 8;
                           i < ((int_bit_position (field) + (bit_offset % 64))
                                + tree_to_shwi (DECL_SIZE (field))
                                + 63) / 8 / 8; i++)
                        classes[i]
                          = merge_classes (X86_64_INTEGER_CLASS, classes[i]);
                    }
which I think means that if the zero-width bitfields are at bit-positions
(in the toplevel aggregate) which are multiples of 64 bits doesn't do
anything, (int_bit_position (field) + (bit_offset % 64)) / 64 and
(int_bit_position (field) + (bit_offset % 64) + 63) / 64 should be equal.
But for zero-width bitfields at other bit positions it will call
merge_classes once.  Now, the typical case is that the zero width bitfield
is surrounded by some bitfields and in that case, it doesn't change
anything, but it can be sandwitched in between floats too as the testcases
show.
In C we had this behavior, in C++ previously the FE was removing the
zero-width bitfields and therefore they were ignored.
LLVM and ICC seems to ignore those bitfields both in C and C++ (== passing
struct S in SSE register rather than in GPR).

The following patch assumes the current GCC C behavior of not ignoring them
when not at bitpositions divisible by 64 is right (though I'm really not
sure about that) and implements a -Wpsabi warning for that case.
The psABI IMHO should be clarified in any case.
The other option is to start ignoring them always (and issue -Wpsabi warning
if !DECL_FIELD_ABI_IGNORED on zero-width bitfield and it would change the
outcome).

I think libffi doesn't need changing, as I think it doesn't support
bitfields.

Bootstrapped/regtested on x86_64-linux and i686-linux.

2021-08-31  Jakub Jelinek  <jakub@redhat.com>

	PR target/102024
	* config/i386/i386.c (classify_argument): Add cxx_bitfields argument,
	when seeing DECL_FIELD_ABI_IGNORED bitfields either set it to 1 or
	if equal to 2 ignore it.  Pass it to recursive calls.  Add wrapper
	with old arguments and diagnose ABI differences for C++ structures
	with zero width bitfields.  Formatting fixes.

	* gcc.target/i386/pr102024.c: New test.
	* g++.target/i386/pr102024.C: New test.

--- gcc/config/i386/i386.c.jj	2021-08-30 08:36:11.070518705 +0200
+++ gcc/config/i386/i386.c	2021-08-30 15:06:07.339940410 +0200
@@ -2062,7 +2062,8 @@ merge_classes (enum x86_64_reg_class cla
 
 static int
 classify_argument (machine_mode mode, const_tree type,
-		   enum x86_64_reg_class classes[MAX_CLASSES], int bit_offset)
+		   enum x86_64_reg_class classes[MAX_CLASSES], int bit_offset,
+		   int &cxx_bitfields)
 {
   HOST_WIDE_INT bytes
     = mode == BLKmode ? int_size_in_bytes (type) : (int) GET_MODE_SIZE (mode);
@@ -2120,6 +2121,12 @@ classify_argument (machine_mode mode, co
 		     misaligned integers.  */
 		  if (DECL_BIT_FIELD (field))
 		    {
+		      if (DECL_FIELD_ABI_IGNORED (field))
+			{
+			  if (cxx_bitfields == 2)
+			    continue;
+			  cxx_bitfields = 1;
+			}
 		      for (i = (int_bit_position (field)
 				+ (bit_offset % 64)) / 8 / 8;
 			   i < ((int_bit_position (field) + (bit_offset % 64))
@@ -2157,7 +2164,8 @@ classify_argument (machine_mode mode, co
 		      num = classify_argument (TYPE_MODE (type), type,
 					       subclasses,
 					       (int_bit_position (field)
-						+ bit_offset) % 512);
+						+ bit_offset) % 512,
+					       cxx_bitfields);
 		      if (!num)
 			return 0;
 		      pos = (int_bit_position (field)
@@ -2175,7 +2183,8 @@ classify_argument (machine_mode mode, co
 	  {
 	    int num;
 	    num = classify_argument (TYPE_MODE (TREE_TYPE (type)),
-				     TREE_TYPE (type), subclasses, bit_offset);
+				     TREE_TYPE (type), subclasses, bit_offset,
+				     cxx_bitfields);
 	    if (!num)
 	      return 0;
 
@@ -2206,7 +2215,7 @@ classify_argument (machine_mode mode, co
 
 		  num = classify_argument (TYPE_MODE (TREE_TYPE (field)),
 					   TREE_TYPE (field), subclasses,
-					   bit_offset);
+					   bit_offset, cxx_bitfields);
 		  if (!num)
 		    return 0;
 		  for (i = 0; i < num && i < words; i++)
@@ -2226,7 +2235,7 @@ classify_argument (machine_mode mode, co
 	     X86_64_SSEUP_CLASS, everything should be passed in
 	     memory.  */
 	  if (classes[0] != X86_64_SSE_CLASS)
-	      return 0;
+	    return 0;
 
 	  for (i = 1; i < words; i++)
 	    if (classes[i] != X86_64_SSEUP_CLASS)
@@ -2252,8 +2261,8 @@ classify_argument (machine_mode mode, co
 	      classes[i] = X86_64_SSE_CLASS;
 	    }
 
-	  /*  If X86_64_X87UP_CLASS isn't preceded by X86_64_X87_CLASS,
-	       everything should be passed in memory.  */
+	  /* If X86_64_X87UP_CLASS isn't preceded by X86_64_X87_CLASS,
+	     everything should be passed in memory.  */
 	  if (classes[i] == X86_64_X87UP_CLASS
 	      && (classes[i - 1] != X86_64_X87_CLASS))
 	    {
@@ -2462,6 +2471,43 @@ classify_argument (machine_mode mode, co
     }
 }
 
+/* Wrapper around classify_argument with the extra cxx_bitfields argument,
+   to diagnose GCC 12.1 ABI differences for C++.  */
+
+static int
+classify_argument (machine_mode mode, const_tree type,
+		   enum x86_64_reg_class classes[MAX_CLASSES], int bit_offset)
+{
+  int cxx_bitfields = 0;
+  static bool warned = false;
+  int n = classify_argument (mode, type, classes, bit_offset, cxx_bitfields);
+  if (!cxx_bitfields || warned || !warn_psabi)
+    return n;
+  enum x86_64_reg_class alt_classes[MAX_CLASSES];
+  cxx_bitfields = 2;
+  if (classify_argument (mode, type, alt_classes, bit_offset,
+			 cxx_bitfields) != n)
+    cxx_bitfields = 3;
+  else
+    for (int i = 0; i < n; i++)
+      if (classes[i] != alt_classes[i])
+	{
+	  cxx_bitfields = 3;
+	  break;
+	}
+  if (cxx_bitfields == 3)
+    {
+      warned = true;
+      const char *url
+	= CHANGES_ROOT_URL "gcc-12/changes.html#cxx_zero_width_bitfields";
+
+      inform (input_location,
+	      "the ABI of passing C++ structures with zero-width bit-fields"
+	      " has changed in GCC %{12.1%}", url);
+    }
+  return n;
+}
+
 /* Examine the argument and return set number of register required in each
    class.  Return true iff parameter should be passed in memory.  */
 
--- gcc/testsuite/gcc.target/i386/pr102024.c.jj	2021-08-30 15:11:09.223718641 +0200
+++ gcc/testsuite/gcc.target/i386/pr102024.c	2021-08-30 15:11:45.426211495 +0200
@@ -0,0 +1,12 @@
+/* PR target/102024 */
+/* { dg-do compile } */
+
+struct S { float a; int : 0; float b; };
+void foo (struct S x);
+
+void
+bar (void)
+{
+  struct S s = { 0.0f, 0.0f };
+  foo (s);	/* { dg-bogus "the ABI of passing C\\+\\+ structures with zero-width bit-fields has changed in GCC 12.1" } */
+}
--- gcc/testsuite/g++.target/i386/pr102024.C.jj	2021-08-30 15:12:42.710409020 +0200
+++ gcc/testsuite/g++.target/i386/pr102024.C	2021-08-30 15:13:23.930831579 +0200
@@ -0,0 +1,12 @@
+// PR target/102024
+// { dg-do compile }
+
+struct S { float a; int : 0; float b; };
+void foo (struct S x);
+
+void
+bar (void)
+{
+  struct S s = { 0.0f, 0.0f };
+  foo (s);	// { dg-message "the ABI of passing C\\+\\+ structures with zero-width bit-fields has changed in GCC 12.1" "" { target { ! ia32 } } }
+}

	Jakub



More information about the Gcc-patches mailing list