public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
* [PATCH] mips: Ignore zero width bitfields in arguments and issue -Wpsabi warning about C zero-width bit-field ABI changes [PR102024]
@ 2022-03-31 16:27 Xi Ruoyao
  2022-03-31 16:47 ` Jakub Jelinek
  0 siblings, 1 reply; 4+ messages in thread
From: Xi Ruoyao @ 2022-03-31 16:27 UTC (permalink / raw)
  To: gcc-patches; +Cc: Richard Sandiford, Jakub Jelinek, YunQiang Su

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



^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] mips: Ignore zero width bitfields in arguments and issue -Wpsabi warning about C zero-width bit-field ABI changes [PR102024]
  2022-03-31 16:27 [PATCH] mips: Ignore zero width bitfields in arguments and issue -Wpsabi warning about C zero-width bit-field ABI changes [PR102024] Xi Ruoyao
@ 2022-03-31 16:47 ` Jakub Jelinek
  2022-04-01 12:11   ` [PATCH v2] Ignore zero width fields in arguments and issue -Wpsabi warning about C zero-width field " Xi Ruoyao
  0 siblings, 1 reply; 4+ messages in thread
From: Jakub Jelinek @ 2022-03-31 16:47 UTC (permalink / raw)
  To: Xi Ruoyao; +Cc: gcc-patches, Richard Sandiford, Jakub Jelinek, YunQiang Su

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


^ permalink raw reply	[flat|nested] 4+ messages in thread

* [PATCH v2] Ignore zero width fields in arguments and issue -Wpsabi warning about C zero-width field ABI changes [PR102024]
  2022-03-31 16:47 ` Jakub Jelinek
@ 2022-04-01 12:11   ` Xi Ruoyao
  2022-04-01 12:38     ` Jakub Jelinek
  0 siblings, 1 reply; 4+ messages in thread
From: Xi Ruoyao @ 2022-04-01 12:11 UTC (permalink / raw)
  To: Jakub Jelinek; +Cc: gcc-patches, Richard Sandiford, Jakub Jelinek, YunQiang Su

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



^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v2] Ignore zero width fields in arguments and issue -Wpsabi warning about C zero-width field ABI changes [PR102024]
  2022-04-01 12:11   ` [PATCH v2] Ignore zero width fields in arguments and issue -Wpsabi warning about C zero-width field " Xi Ruoyao
@ 2022-04-01 12:38     ` Jakub Jelinek
  0 siblings, 0 replies; 4+ messages in thread
From: Jakub Jelinek @ 2022-04-01 12:38 UTC (permalink / raw)
  To: Xi Ruoyao; +Cc: gcc-patches, Richard Sandiford, Jakub Jelinek, YunQiang Su

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


^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2022-04-01 12:38 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-31 16:27 [PATCH] mips: Ignore zero width bitfields in arguments and issue -Wpsabi warning about C zero-width bit-field ABI changes [PR102024] Xi Ruoyao
2022-03-31 16:47 ` Jakub Jelinek
2022-04-01 12:11   ` [PATCH v2] Ignore zero width fields in arguments and issue -Wpsabi warning about C zero-width field " Xi Ruoyao
2022-04-01 12:38     ` Jakub Jelinek

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).