[PATCH] mips: Ignore zero width bitfields in arguments and issue -Wpsabi warning about C zero-width bit-field ABI changes [PR102024]

[PATCH] mips: Ignore zero width bitfields in arguments and issue -Wpsabi warning about C zero-width bit-field ABI changes [PR102024]

[Xi Ruoyao]

Part 2/2 of PR 102024 fix for MIPS.

The ABI says:

"Regardless of the struct field structure, it is treated as a sequence
of 64-bit chunks. If a chunk consists solely of a double float field
(but not a double, which is part of a union), it is passed in a floating
point register. Any other chunk is passed in an integer register."

It's not clear that if a zero-width field is a part of any 64-bit chunk,
and which 64-bit chunk it shall belong to when its on the boundary of
two chunks.  In previous GCC releases, the C++ FE removes all zero-width
bit-fields but otherwise a zero-width field is considered a part of the
next 64-bit chunk:

struct A
{
  /* first chunk */
  double a;  /* into FPR */
  /* second chunk */
  int : 0;
  double b;  /* into GPR with current GCC trunk because "the chunk
                contains a bit-field" */
};

But clang does not account a zero-width bit-field on the boundary into
any "64-bit chunk", so its behavior is same as the C++ FE of previous
GCC releases.

I think we should not make an arbitary assumption like "a zero-width
bit-field is a part of the next chunk, but not a part of the previous
chunk".  So a consistent behavior is either consider it a part of both
chunks, or consider it not a part of any chunks.  As we are changing
psABI anyway, it seems OK to be compatible with clang.

gcc/
	PR target/102024
	* mips.cc (mips_function_arg): Ignore zero-width bit-fields, and
	inform if it causes a psABI change.

gcc/testsuite/
	PR target/102024
	* gcc.target/mips/pr102024.c: New test.
---
 gcc/config/mips/mips.cc                  | 45 +++++++++++++++++++++---
 gcc/testsuite/gcc.target/mips/pr102024.c | 20 +++++++++++
 2 files changed, 61 insertions(+), 4 deletions(-)
 create mode 100644 gcc/testsuite/gcc.target/mips/pr102024.c

diff --git a/gcc/config/mips/mips.cc b/gcc/config/mips/mips.cc
index 3284cf71f6f..5d1637e7b2f 100644
--- a/gcc/config/mips/mips.cc
+++ b/gcc/config/mips/mips.cc
@@ -6042,11 +6042,26 @@ mips_function_arg (cumulative_args_t cum_v, const function_arg_info &arg)
 	  for (i = 0; i < info.reg_words; i++)
 	    {
 	      rtx reg;
+	      int has_zero_width_bf_abi_change = 0;
 
 	      for (; field; field = DECL_CHAIN (field))
-		if (TREE_CODE (field) == FIELD_DECL
-		    && int_bit_position (field) >= bitpos)
-		  break;
+		{
+		  if (TREE_CODE (field) != FIELD_DECL)
+		    continue;
+
+		  /* Ignore zero-width bit-fields.  And, if the ignored
+		     field is not from C++, it may be an ABI change.  */
+		  if (DECL_FIELD_CXX_ZERO_WIDTH_BIT_FIELD (field))
+		    continue;
+		  if (integer_zerop (DECL_SIZE (field)))
+		    {
+		      has_zero_width_bf_abi_change = 1;
+		      continue;
+		    }
+
+		  if (int_bit_position (field) >= bitpos)
+		    break;
+		}
 
 	      if (field
 		  && int_bit_position (field) == bitpos
@@ -6054,7 +6069,29 @@ mips_function_arg (cumulative_args_t cum_v, const function_arg_info &arg)
 		  && TYPE_PRECISION (TREE_TYPE (field)) == BITS_PER_WORD)
 		reg = gen_rtx_REG (DFmode, FP_ARG_FIRST + info.reg_offset + i);
 	      else
-		reg = gen_rtx_REG (DImode, GP_ARG_FIRST + info.reg_offset + i);
+		{
+		  reg = gen_rtx_REG (DImode,
+				     GP_ARG_FIRST + info.reg_offset + i);
+		  has_zero_width_bf_abi_change = 0;
+		}
+
+	      if (has_zero_width_bf_abi_change && warn_psabi)
+		{
+		  static unsigned last_reported_type_uid;
+		  unsigned uid = TYPE_UID (TYPE_MAIN_VARIANT (arg.type));
+		  if (uid != last_reported_type_uid)
+		    {
+		      static const char *url =
+			CHANGES_ROOT_URL
+			"gcc-12/changes.html#zero_width_bitfields";
+		      inform (input_location,
+			      "the ABI for passing a value containing "
+			      "zero-width bit-fields before an adjacent "
+			      "64-bit floating-point field was retconned "
+			      "in GCC %{12.1%}", url);
+		      last_reported_type_uid = uid;
+		    }
+		}
 
 	      XVECEXP (ret, 0, i)
 		= gen_rtx_EXPR_LIST (VOIDmode, reg,
diff --git a/gcc/testsuite/gcc.target/mips/pr102024.c b/gcc/testsuite/gcc.target/mips/pr102024.c
new file mode 100644
index 00000000000..c45d01315d6
--- /dev/null
+++ b/gcc/testsuite/gcc.target/mips/pr102024.c
@@ -0,0 +1,20 @@
+// PR target/102024
+// { dg-do compile }
+// { dg-options "-mabi=64 -mhard-float" }
+// { dg-final { scan-assembler "\\\$f12" } }
+
+struct foo
+{
+  int : 0;
+  double a;
+};
+
+extern void func(struct foo);
+
+void
+pass_foo(void)
+{
+  struct foo test;
+  test.a = 114;
+  func(test); // { dg-message "the ABI for passing a value containing zero-width bit-fields before an adjacent 64-bit floating-point field was retconned in GCC 12.1" }
+}
-- 
2.35.1

Re: [PATCH] mips: Ignore zero width bitfields in arguments and issue -Wpsabi warning about C zero-width bit-field ABI changes [PR102024]

[Jakub Jelinek]

On Fri, Apr 01, 2022 at 12:27:45AM +0800, Xi Ruoyao wrote:
> --- a/gcc/config/mips/mips.cc
> +++ b/gcc/config/mips/mips.cc
> @@ -6042,11 +6042,26 @@ mips_function_arg (cumulative_args_t cum_v, const function_arg_info &arg)
>  	  for (i = 0; i < info.reg_words; i++)
>  	    {
>  	      rtx reg;
> +	      int has_zero_width_bf_abi_change = 0;

If something has just 0/1 value, perhaps better use bool and false/true
for the values?

>  	      for (; field; field = DECL_CHAIN (field))
> -		if (TREE_CODE (field) == FIELD_DECL
> -		    && int_bit_position (field) >= bitpos)
> -		  break;
> +		{
> +		  if (TREE_CODE (field) != FIELD_DECL)
> +		    continue;
> +
> +		  /* Ignore zero-width bit-fields.  And, if the ignored
> +		     field is not from C++, it may be an ABI change.  */
> +		  if (DECL_FIELD_CXX_ZERO_WIDTH_BIT_FIELD (field))
> +		    continue;

While the above is only about zero width bit-fields from the C++ FE,

> +		  if (integer_zerop (DECL_SIZE (field)))

this matches both zero width bitfields and zero sized structures
and zero length arrays.  While I believe the same
arguments as for zero width bitfields apply for those and it is
interesting to see what say LLVM does with those (see the
compat testsuite change I've posted today), either the diagnostics
should be worded so that it covers both, or this could be
a 0/1/2 value flag or enum which then decides how to word the
diagnostics.
Including the _bf in the name is misleading.
> +		    {
> +		      has_zero_width_bf_abi_change = 1;
> +		      continue;
> +		    }

> +		      static const char *url =
> +			CHANGES_ROOT_URL
> +			"gcc-12/changes.html#zero_width_bitfields";

Formatting guidelines say that the = should go on the next line.

> +		      inform (input_location,
> +			      "the ABI for passing a value containing "
> +			      "zero-width bit-fields before an adjacent "
> +			      "64-bit floating-point field was retconned "
> +			      "in GCC %{12.1%}", url);

Not a native speaker, but retconned seems weird and would be better to
match the wording in other targets.

> +		      last_reported_type_uid = uid;
> +		    }
> +		}
>  
>  	      XVECEXP (ret, 0, i)
>  		= gen_rtx_EXPR_LIST (VOIDmode, reg,

	Jakub

[PATCH v2] Ignore zero width fields in arguments and issue -Wpsabi warning about C zero-width field ABI changes [PR102024]

[Xi Ruoyao]

v1 -> v2:

* "int has_zero_width_bf_abi_change" -> "bool
zero_width_field_abi_change".  "int" -> "bool" because it's only 0/1,
"bf" -> "field" because the change also affects zero-length arrays and
empty structs/unions, etc.

* Add tests with zero-length array and empty struct.

* Coding style fix.

* "#zero_width_bitfields" -> "#mips_zero_width_fields" because this is
not the exactly same change documented by #zero_width_bitfields.  I'll
send a wwwdoc patch after this is approved.

gcc/
	PR target/102024
	* mips.cc (mips_function_arg): Ignore zero-width fields, and
	inform if it causes a psABI change.

gcc/testsuite/
	PR target/102024
	* gcc.target/mips/pr102024-1.c: New test.
	* gcc.target/mips/pr102024-2.c: New test.
	* gcc.target/mips/pr102024-3.c: New test.
---
 gcc/config/mips/mips.cc                    | 46 ++++++++++++++++++++--
 gcc/testsuite/gcc.target/mips/pr102024-1.c | 20 ++++++++++
 gcc/testsuite/gcc.target/mips/pr102024-2.c | 20 ++++++++++
 gcc/testsuite/gcc.target/mips/pr102024-3.c | 20 ++++++++++
 4 files changed, 102 insertions(+), 4 deletions(-)
 create mode 100644 gcc/testsuite/gcc.target/mips/pr102024-1.c
 create mode 100644 gcc/testsuite/gcc.target/mips/pr102024-2.c
 create mode 100644 gcc/testsuite/gcc.target/mips/pr102024-3.c

diff --git a/gcc/config/mips/mips.cc b/gcc/config/mips/mips.cc
index 83860b5d4b7..7681983186c 100644
--- a/gcc/config/mips/mips.cc
+++ b/gcc/config/mips/mips.cc
@@ -6042,11 +6042,27 @@ mips_function_arg (cumulative_args_t cum_v, const function_arg_info &arg)
 	  for (i = 0; i < info.reg_words; i++)
 	    {
 	      rtx reg;
+	      bool zero_width_field_abi_change = false;
 
 	      for (; field; field = DECL_CHAIN (field))
-		if (TREE_CODE (field) == FIELD_DECL
-		    && int_bit_position (field) >= bitpos)
-		  break;
+		{
+		  if (TREE_CODE (field) != FIELD_DECL)
+		    continue;
+
+		  /* Ignore zero-width fields.  And, if the ignored
+		     field is not a C++ zero-width bit-field, it may be
+		     an ABI change.  */
+		  if (DECL_FIELD_CXX_ZERO_WIDTH_BIT_FIELD (field))
+		    continue;
+		  if (integer_zerop (DECL_SIZE (field)))
+		    {
+		      zero_width_field_abi_change = true;
+		      continue;
+		    }
+
+		  if (int_bit_position (field) >= bitpos)
+		    break;
+		}
 
 	      if (field
 		  && int_bit_position (field) == bitpos
@@ -6054,7 +6070,29 @@ mips_function_arg (cumulative_args_t cum_v, const function_arg_info &arg)
 		  && TYPE_PRECISION (TREE_TYPE (field)) == BITS_PER_WORD)
 		reg = gen_rtx_REG (DFmode, FP_ARG_FIRST + info.reg_offset + i);
 	      else
-		reg = gen_rtx_REG (DImode, GP_ARG_FIRST + info.reg_offset + i);
+		{
+		  reg = gen_rtx_REG (DImode,
+				     GP_ARG_FIRST + info.reg_offset + i);
+		  zero_width_field_abi_change = false;
+		}
+
+	      if (zero_width_field_abi_change && warn_psabi)
+		{
+		  static unsigned last_reported_type_uid;
+		  unsigned uid = TYPE_UID (TYPE_MAIN_VARIANT (arg.type));
+		  if (uid != last_reported_type_uid)
+		    {
+		      static const char *url
+			= CHANGES_ROOT_URL
+			  "gcc-12/changes.html#mips_zero_width_fields";
+		      inform (input_location,
+			      "the ABI for passing a value containing "
+			      "zero-width fields before an adjacent "
+			      "64-bit floating-point field was changed "
+			      "in GCC %{12.1%}", url);
+		      last_reported_type_uid = uid;
+		    }
+		}
 
 	      XVECEXP (ret, 0, i)
 		= gen_rtx_EXPR_LIST (VOIDmode, reg,
diff --git a/gcc/testsuite/gcc.target/mips/pr102024-1.c b/gcc/testsuite/gcc.target/mips/pr102024-1.c
new file mode 100644
index 00000000000..cf442863fc2
--- /dev/null
+++ b/gcc/testsuite/gcc.target/mips/pr102024-1.c
@@ -0,0 +1,20 @@
+// PR target/102024
+// { dg-do compile }
+// { dg-options "-mabi=64 -mhard-float" }
+// { dg-final { scan-assembler "\\\$f12" } }
+
+struct foo
+{
+  int : 0;
+  double a;
+};
+
+extern void func(struct foo);
+
+void
+pass_foo(void)
+{
+  struct foo test;
+  test.a = 114;
+  func(test); // { dg-message "the ABI for passing a value containing zero-width fields before an adjacent 64-bit floating-point field was changed in GCC 12.1" }
+}
diff --git a/gcc/testsuite/gcc.target/mips/pr102024-2.c b/gcc/testsuite/gcc.target/mips/pr102024-2.c
new file mode 100644
index 00000000000..89b26f86038
--- /dev/null
+++ b/gcc/testsuite/gcc.target/mips/pr102024-2.c
@@ -0,0 +1,20 @@
+// PR target/102024
+// { dg-do compile }
+// { dg-options "-mabi=64 -mhard-float" }
+// { dg-final { scan-assembler "\\\$f12" } }
+
+struct foo
+{
+  char empty[0];
+  double a;
+};
+
+extern void func(struct foo);
+
+void
+pass_foo(void)
+{
+  struct foo test;
+  test.a = 114;
+  func(test); // { dg-message "the ABI for passing a value containing zero-width fields before an adjacent 64-bit floating-point field was changed in GCC 12.1" }
+}
diff --git a/gcc/testsuite/gcc.target/mips/pr102024-3.c b/gcc/testsuite/gcc.target/mips/pr102024-3.c
new file mode 100644
index 00000000000..477f07055a1
--- /dev/null
+++ b/gcc/testsuite/gcc.target/mips/pr102024-3.c
@@ -0,0 +1,20 @@
+// PR target/102024
+// { dg-do compile }
+// { dg-options "-mabi=64 -mhard-float" }
+// { dg-final { scan-assembler "\\\$f12" } }
+
+struct foo
+{
+  struct {} empty;
+  double a;
+};
+
+extern void func(struct foo);
+
+void
+pass_foo(void)
+{
+  struct foo test;
+  test.a = 114;
+  func(test); // { dg-message "the ABI for passing a value containing zero-width fields before an adjacent 64-bit floating-point field was changed in GCC 12.1" }
+}
-- 
2.35.1

Re: [PATCH v2] Ignore zero width fields in arguments and issue -Wpsabi warning about C zero-width field ABI changes [PR102024]

[Jakub Jelinek]

On Fri, Apr 01, 2022 at 08:11:43PM +0800, Xi Ruoyao wrote:
> v1 -> v2:
> 
> * "int has_zero_width_bf_abi_change" -> "bool
> zero_width_field_abi_change".  "int" -> "bool" because it's only 0/1,
> "bf" -> "field" because the change also affects zero-length arrays and
> empty structs/unions, etc.
> 
> * Add tests with zero-length array and empty struct.
> 
> * Coding style fix.
> 
> * "#zero_width_bitfields" -> "#mips_zero_width_fields" because this is
> not the exactly same change documented by #zero_width_bitfields.  I'll
> send a wwwdoc patch after this is approved.
> 
> gcc/
> 	PR target/102024
> 	* mips.cc (mips_function_arg): Ignore zero-width fields, and
> 	inform if it causes a psABI change.
> 
> gcc/testsuite/
> 	PR target/102024
> 	* gcc.target/mips/pr102024-1.c: New test.
> 	* gcc.target/mips/pr102024-2.c: New test.
> 	* gcc.target/mips/pr102024-3.c: New test.

LGTM, thanks.

	Jakub