This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

[PATCH] RISC-V: Fix C ABI for flattened struct with 0-length bitfield.


This fixes a problem with the GCC implementation of the ABI, where we are
accidentally emitting different code for the same struct when compiled by the
C and C++ compilers.  This was found by LLVM developers comparing the LLVM ABI
to the GCC ABI.

This affects any struct with one or more zero-length bitfields, exactly two
non-zero length fields, one of which must have an FP type that can fit in an
FP register, and the other can be an FP type that fits in an FP register or
an integer type that fits in an integer register or a integer bit-field that
can fit in an integer register.  Also, the target must have FP register
support.

The affected structures are a bit obscure and not very useful, so it is hoped
no real code will be affected.  I've done an open-embedded world build with an
instrumented compiler, and I didn't see any case that triggered my code.  Not
everything built though, since some stuff still doesn't have RISC-V support
yet.  I did have over 30,000 tasks run, so quite a bit of stuff did build.

The fundamental problem is that the RISC-V backend is not checking for
zero-length bit-fields when deciding if a struct field can be passed in a FP
register or not.  Meanwhile, the C++ front end is stripping zero-length
bit-fields after struct layout.  So when compiling as C++ we decide that the
FP struct fields can be passed in FP regs.  But when compiling as C we decide
that there are too many struct fields and they all get passed in integer
registers.

This issue has already been discussed with the RISC-V psABI group and the
RISC-V LLVM developers and this is how we have agreed to move forward, by
changing the ABI implemented by the GCC C (but not C++) compiler.

I will wait a few days before committing in case someone wants to discuss this.

Jim

	gcc/
	PR target/91229
	* config/riscv/riscv.c (riscv_flatten_aggregate_field): New arg
	ignore_zero_width_bit_field_p.  Skip zero size bitfields when true.
	Pass into recursive call.
	(riscv_flatten_aggregate_argument): New arg.  Pass to
	riscv_flatten_aggregate_field.
	(riscv_pass_aggregate_in_fpr_pair_p): New local warned.  Call
	riscv_flatten_aggregate_argument twice, with false and true as last
	arg.  Process result twice.  Compare results and warn if different.
	(riscv_pass_aggregate_in_fpr_and_gpr_p): Likewise.

	gcc/testsuite/
	* gcc.target/riscv/flattened-struct-abi-1.c: New test.
	* gcc.target/riscv/flattened-struct-abi-2.c: New test.
---
 gcc/config/riscv/riscv.c                      | 92 +++++++++++++++----
 .../gcc.target/riscv/flattened-struct-abi-1.c |  9 ++
 .../gcc.target/riscv/flattened-struct-abi-2.c |  9 ++
 3 files changed, 94 insertions(+), 16 deletions(-)
 create mode 100644 gcc/testsuite/gcc.target/riscv/flattened-struct-abi-1.c
 create mode 100644 gcc/testsuite/gcc.target/riscv/flattened-struct-abi-2.c

diff --git a/gcc/config/riscv/riscv.c b/gcc/config/riscv/riscv.c
index e274f1bd10f..78a87e2b1d6 100644
--- a/gcc/config/riscv/riscv.c
+++ b/gcc/config/riscv/riscv.c
@@ -2383,7 +2383,8 @@ typedef struct {
 static int
 riscv_flatten_aggregate_field (const_tree type,
 			       riscv_aggregate_field fields[2],
-			       int n, HOST_WIDE_INT offset)
+			       int n, HOST_WIDE_INT offset,
+			       bool ignore_zero_width_bit_field_p)
 {
   switch (TREE_CODE (type))
     {
@@ -2400,8 +2401,21 @@ riscv_flatten_aggregate_field (const_tree type,
 	    if (!TYPE_P (TREE_TYPE (f)))
 	      return -1;
 
-	    HOST_WIDE_INT pos = offset + int_byte_position (f);
-	    n = riscv_flatten_aggregate_field (TREE_TYPE (f), fields, n, pos);
+	    /* The C++ front end strips zero-length bit-fields from structs.
+	       So we need to ignore them in the C front end to make C code
+	       compatible with C++ code.  */
+	    if (ignore_zero_width_bit_field_p
+		&& DECL_BIT_FIELD (f)
+		&& (DECL_SIZE (f) == NULL_TREE
+		    || integer_zerop (DECL_SIZE (f))))
+	      ;
+	    else
+	      {
+		HOST_WIDE_INT pos = offset + int_byte_position (f);
+		n = riscv_flatten_aggregate_field (TREE_TYPE (f),
+						   fields, n, pos,
+						   ignore_zero_width_bit_field_p);
+	      }
 	    if (n < 0)
 	      return -1;
 	  }
@@ -2414,7 +2428,8 @@ riscv_flatten_aggregate_field (const_tree type,
 	tree index = TYPE_DOMAIN (type);
 	tree elt_size = TYPE_SIZE_UNIT (TREE_TYPE (type));
 	int n_subfields = riscv_flatten_aggregate_field (TREE_TYPE (type),
-							 subfields, 0, offset);
+							 subfields, 0, offset,
+							 ignore_zero_width_bit_field_p);
 
 	/* Can't handle incomplete types nor sizes that are not fixed.  */
 	if (n_subfields <= 0
@@ -2487,12 +2502,14 @@ riscv_flatten_aggregate_field (const_tree type,
 
 static int
 riscv_flatten_aggregate_argument (const_tree type,
-				  riscv_aggregate_field fields[2])
+				  riscv_aggregate_field fields[2],
+				  bool ignore_zero_width_bit_field_p)
 {
   if (!type || TREE_CODE (type) != RECORD_TYPE)
     return -1;
 
-  return riscv_flatten_aggregate_field (type, fields, 0, 0);
+  return riscv_flatten_aggregate_field (type, fields, 0, 0,
+					ignore_zero_width_bit_field_p);
 }
 
 /* See whether TYPE is a record whose fields should be returned in one or
@@ -2502,13 +2519,34 @@ static unsigned
 riscv_pass_aggregate_in_fpr_pair_p (const_tree type,
 				    riscv_aggregate_field fields[2])
 {
-  int n = riscv_flatten_aggregate_argument (type, fields);
+  static int warned = 0;
 
-  for (int i = 0; i < n; i++)
+  /* This is the old ABI, which differs for C++ and C.  */
+  int n_old = riscv_flatten_aggregate_argument (type, fields, false);
+  for (int i = 0; i < n_old; i++)
     if (!SCALAR_FLOAT_TYPE_P (fields[i].type))
-      return 0;
+      {
+	n_old = -1;
+	break;
+      }
+
+  /* This is the new ABI, which is the same for C++ and C.  */
+  int n_new = riscv_flatten_aggregate_argument (type, fields, true);
+  for (int i = 0; i < n_new; i++)
+    if (!SCALAR_FLOAT_TYPE_P (fields[i].type))
+      {
+	n_new = -1;
+	break;
+      }
 
-  return n > 0 ? n : 0;
+  if ((n_old != n_new) && (warned == 0))
+    {
+      warning (0, "ABI for flattened struct with zero-length bit-fields "
+	       "changed in GCC 10");
+      warned = 1;
+    }
+
+  return n_new > 0 ? n_new : 0;
 }
 
 /* See whether TYPE is a record whose fields should be returned in one or
@@ -2519,16 +2557,38 @@ static bool
 riscv_pass_aggregate_in_fpr_and_gpr_p (const_tree type,
 				       riscv_aggregate_field fields[2])
 {
-  unsigned num_int = 0, num_float = 0;
-  int n = riscv_flatten_aggregate_argument (type, fields);
+  static int warned = 0;
+
+  /* This is the old ABI, which differs for C++ and C.  */
+  unsigned num_int_old = 0, num_float_old = 0;
+  int n_old = riscv_flatten_aggregate_argument (type, fields, false);
+  for (int i = 0; i < n_old; i++)
+    {
+      num_float_old += SCALAR_FLOAT_TYPE_P (fields[i].type);
+      num_int_old += INTEGRAL_TYPE_P (fields[i].type);
+    }
+
+  /* This is the new ABI, which is the same for C++ and C.  */
+  unsigned num_int_new = 0, num_float_new = 0;
+  int n_new = riscv_flatten_aggregate_argument (type, fields, true);
+  for (int i = 0; i < n_new; i++)
+    {
+      num_float_new += SCALAR_FLOAT_TYPE_P (fields[i].type);
+      num_int_new += INTEGRAL_TYPE_P (fields[i].type);
+    }
 
-  for (int i = 0; i < n; i++)
+  if (((num_int_old == 1 && num_float_old == 1
+	&& (num_int_old != num_int_new || num_float_old != num_float_new))
+       || (num_int_new == 1 && num_float_new == 1
+	   && (num_int_old != num_int_new || num_float_old != num_float_new)))
+      && (warned == 0))
     {
-      num_float += SCALAR_FLOAT_TYPE_P (fields[i].type);
-      num_int += INTEGRAL_TYPE_P (fields[i].type);
+      warning (0, "ABI for flattened struct with zero-length bit-fields "
+	       "changed in GCC 10");
+      warned = 1;
     }
 
-  return num_int == 1 && num_float == 1;
+  return num_int_new == 1 && num_float_new == 1;
 }
 
 /* Return the representation of an argument passed or returned in an FPR
diff --git a/gcc/testsuite/gcc.target/riscv/flattened-struct-abi-1.c b/gcc/testsuite/gcc.target/riscv/flattened-struct-abi-1.c
new file mode 100644
index 00000000000..f6a3c51b3fb
--- /dev/null
+++ b/gcc/testsuite/gcc.target/riscv/flattened-struct-abi-1.c
@@ -0,0 +1,9 @@
+/* { dg-do compile } */
+/* { dg-options "-march=rv32gc -mabi=ilp32d" } */
+struct s1 { int : 0; float f; int i; int : 0; };
+
+void dummy(float, int);
+
+void f(struct s1 s) { /* { dg-warning "flattened struct" } */
+  dummy(s.f + 1.0, s.i + 1);
+}
diff --git a/gcc/testsuite/gcc.target/riscv/flattened-struct-abi-2.c b/gcc/testsuite/gcc.target/riscv/flattened-struct-abi-2.c
new file mode 100644
index 00000000000..760826a42f1
--- /dev/null
+++ b/gcc/testsuite/gcc.target/riscv/flattened-struct-abi-2.c
@@ -0,0 +1,9 @@
+/* { dg-do compile } */
+/* { dg-options "-march=rv32gc -mabi=ilp32d" } */
+struct s1 { int : 0; float f; float g; int : 0; };
+
+void dummy(float, float);
+
+void f(struct s1 s) { /* { dg-warning "flattened struct" } */
+  dummy(s.f + 1.0, s.g + 2.0);
+}
-- 
2.17.1


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]