[PATCH] mips: Fix C++14 vs. C++17 ABI incompatibility on mips64

[PATCH] mips: Fix C++14 vs. C++17 ABI incompatibility on mips64

[Xi Ruoyao]

Another MIPS function return ABI fix.  Ok for trunk?

--

This fixes tmpdir-g++.dg-struct-layout-1/{t032,t059} failure.  Clang++
ignores C++17 empty bases in return values as well.

gcc/
	* config/mips/mips.cc (mips_fpr_return_fields): Ignore
	cxx17_empty_base_field_p fields.
---
 gcc/config/mips/mips.cc | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/gcc/config/mips/mips.cc b/gcc/config/mips/mips.cc
index 0f2492219f3..5010f99f761 100644
--- a/gcc/config/mips/mips.cc
+++ b/gcc/config/mips/mips.cc
@@ -6337,7 +6337,8 @@ mips_fpr_return_fields (const_tree valtype, tree *fields,
   i = 0;
   for (field = TYPE_FIELDS (valtype); field != 0; field = DECL_CHAIN (field))
     {
-      if (TREE_CODE (field) != FIELD_DECL)
+      if (TREE_CODE (field) != FIELD_DECL
+	  || cxx17_empty_base_field_p (field))
 	continue;
 
       if (DECL_FIELD_CXX_ZERO_WIDTH_BIT_FIELD (field))
-- 
2.35.1

Re: [PATCH] mips: Fix C++14 vs. C++17 ABI incompatibility on mips64

[Jakub Jelinek]

On Wed, Apr 06, 2022 at 08:33:40PM +0800, Xi Ruoyao via Gcc-patches wrote:
> Another MIPS function return ABI fix.  Ok for trunk?
> 
> --
> 
> This fixes tmpdir-g++.dg-struct-layout-1/{t032,t059} failure.  Clang++
> ignores C++17 empty bases in return values as well.
> 
> gcc/
> 	* config/mips/mips.cc (mips_fpr_return_fields): Ignore
> 	cxx17_empty_base_field_p fields.
> ---
>  gcc/config/mips/mips.cc | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/gcc/config/mips/mips.cc b/gcc/config/mips/mips.cc
> index 0f2492219f3..5010f99f761 100644
> --- a/gcc/config/mips/mips.cc
> +++ b/gcc/config/mips/mips.cc
> @@ -6337,7 +6337,8 @@ mips_fpr_return_fields (const_tree valtype, tree *fields,
>    i = 0;
>    for (field = TYPE_FIELDS (valtype); field != 0; field = DECL_CHAIN (field))
>      {
> -      if (TREE_CODE (field) != FIELD_DECL)
> +      if (TREE_CODE (field) != FIELD_DECL
> +	  || cxx17_empty_base_field_p (field))
>  	continue;
>  
>        if (DECL_FIELD_CXX_ZERO_WIDTH_BIT_FIELD (field))

Well, this won't diagnose the ABI change.
So, if cxx17_empty_base_field_p, it should set some flag before continuing
and if it is considered a fpr return and that flag is set, it should emit a
-Wpsabi warning too.

	Jakub

Re: [PATCH] mips: Fix C++14 vs. C++17 ABI incompatibility on mips64

[Xi Ruoyao]

On Wed, 2022-04-06 at 14:44 +0200, Jakub Jelinek wrote:
> On Wed, Apr 06, 2022 at 08:33:40PM +0800, Xi Ruoyao via Gcc-patches wrote:
> > Another MIPS function return ABI fix.  Ok for trunk?
> > 
> > --
> > 
> > This fixes tmpdir-g++.dg-struct-layout-1/{t032,t059} failure.  Clang++
> > ignores C++17 empty bases in return values as well.
> > 
> > gcc/
> >         * config/mips/mips.cc (mips_fpr_return_fields): Ignore
> >         cxx17_empty_base_field_p fields.
> > ---
> >  gcc/config/mips/mips.cc | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/gcc/config/mips/mips.cc b/gcc/config/mips/mips.cc
> > index 0f2492219f3..5010f99f761 100644
> > --- a/gcc/config/mips/mips.cc
> > +++ b/gcc/config/mips/mips.cc
> > @@ -6337,7 +6337,8 @@ mips_fpr_return_fields (const_tree valtype, tree *fields,
> >    i = 0;
> >    for (field = TYPE_FIELDS (valtype); field != 0; field = DECL_CHAIN (field))
> >      {
> > -      if (TREE_CODE (field) != FIELD_DECL)
> > +      if (TREE_CODE (field) != FIELD_DECL
> > +         || cxx17_empty_base_field_p (field))
> >         continue;
> >  
> >        if (DECL_FIELD_CXX_ZERO_WIDTH_BIT_FIELD (field))
> 
> Well, this won't diagnose the ABI change.
> So, if cxx17_empty_base_field_p, it should set some flag before continuing
> and if it is considered a fpr return and that flag is set, it should emit a
> -Wpsabi warning too.

Ok, will add it.

When I learnt from PR94704 fix I failed to notice the second commit
adding -Wpsabi warning :(.


-- 
Xi Ruoyao <xry111@mengyan1223.wang>
School of Aerospace Science and Technology, Xidian University

[PATCH v2] mips: Fix C++14 vs. C++17 ABI incompatibility on mips64

[Xi Ruoyao]

v2: Add psABI warning and test.

--

This fixes tmpdir-g++.dg-struct-layout-1/{t032,t059} failure.  Clang++
also ignores C++17 empty bases in return values.

gcc/
	* config/mips/mips.cc (mips_fpr_return_fields): Ignore
	cxx17_empty_base_field_p fields and set an indicator.
	(mips_return_in_msb): Adjust for mips_fpr_return_fields change.
	(mips_function_value_1): Inform psABI change about C++ 17 empty
	bases.

gcc/testsuite/
	* g++.target/mips/cxx17_empty_base.C: New test.
---
 gcc/config/mips/mips.cc                       | 58 +++++++++++++++++--
 .../g++.target/mips/cxx17_empty_base.C        | 20 +++++++
 2 files changed, 73 insertions(+), 5 deletions(-)
 create mode 100644 gcc/testsuite/g++.target/mips/cxx17_empty_base.C

diff --git a/gcc/config/mips/mips.cc b/gcc/config/mips/mips.cc
index 079bb03968a..57ce0884951 100644
--- a/gcc/config/mips/mips.cc
+++ b/gcc/config/mips/mips.cc
@@ -6346,12 +6346,21 @@ mips_callee_copies (cumulative_args_t, const function_arg_info &arg)
    The C++ FE used to remove zero-width bit-fields in GCC 11 and earlier.
    To make a proper diagnostic, this function will set
    HAS_CXX_ZERO_WIDTH_BF to true once a C++ zero-width bit-field shows up,
-   and then ignore it. Then the caller can determine if this zero-width
-   bit-field will make a difference and emit a -Wpsabi inform.  */
+   and then ignore it.
+
+   We had failed to ignore C++ 17 empty bases in GCC 7, 8, 9, 10, and 11.
+   This caused an ABI incompatibility between C++ 14 and C++ 17.  This is
+   fixed now and to make a proper diagnostic, this function will set
+   HAS_CXX17_EMPTY_BASE to true once a C++ 17 empty base shows up, and
+   then ignore it.
+
+   The caller should use the value of HAS_CXX17_EMPTY_BASE and/or
+   HAS_CXX_ZERO_WIDTH_BF to emit a proper -Wpsabi inform.  */
 
 static int
 mips_fpr_return_fields (const_tree valtype, tree *fields,
-			bool *has_cxx_zero_width_bf)
+			bool *has_cxx_zero_width_bf,
+			bool *has_cxx17_empty_base)
 {
   tree field;
   int i;
@@ -6368,6 +6377,12 @@ mips_fpr_return_fields (const_tree valtype, tree *fields,
       if (TREE_CODE (field) != FIELD_DECL)
 	continue;
 
+      if (cxx17_empty_base_field_p (field))
+	{
+	  *has_cxx17_empty_base = true;
+	  continue;
+	}
+
       if (DECL_FIELD_CXX_ZERO_WIDTH_BIT_FIELD (field))
 	{
 	  *has_cxx_zero_width_bf = true;
@@ -6403,8 +6418,13 @@ mips_return_in_msb (const_tree valtype)
 
   tree fields[2];
   bool has_cxx_zero_width_bf = false;
+
+  /* Its value is not used.  */
+  bool has_cxx17_empty_base = false;
+
   return (mips_fpr_return_fields (valtype, fields,
-				  &has_cxx_zero_width_bf) == 0
+				  &has_cxx_zero_width_bf,
+				  &has_cxx17_empty_base) == 0
 	  || has_cxx_zero_width_bf);
 }
 
@@ -6501,11 +6521,18 @@ mips_function_value_1 (const_tree valtype, const_tree fn_decl_or_type,
       mode = promote_function_mode (valtype, mode, &unsigned_p, func, 1);
 
       bool has_cxx_zero_width_bf = false;
+      bool has_cxx17_empty_base = false;
       int use_fpr = mips_fpr_return_fields (valtype, fields,
-					    &has_cxx_zero_width_bf);
+					    &has_cxx_zero_width_bf,
+					    &has_cxx17_empty_base);
+
+      /* If has_cxx_zero_width_bf and has_cxx17_empty_base are both
+	 true, it *happens* that there is no ABI change.  So we won't
+	 inform in this case.  */
       if (TARGET_HARD_FLOAT
 	  && warn_psabi
 	  && has_cxx_zero_width_bf
+	  && !has_cxx17_empty_base
 	  && use_fpr != 0)
 	{
 	  static unsigned last_reported_type_uid;
@@ -6527,6 +6554,27 @@ mips_function_value_1 (const_tree valtype, const_tree fn_decl_or_type,
       if (has_cxx_zero_width_bf)
 	use_fpr = 0;
 
+      if (TARGET_HARD_FLOAT
+	  && warn_psabi
+	  && use_fpr != 0
+	  && has_cxx17_empty_base)
+	{
+	  static unsigned last_reported_type_uid;
+	  unsigned uid = TYPE_UID (TYPE_MAIN_VARIANT (valtype));
+	  if (uid != last_reported_type_uid)
+	    {
+	      static const char *url
+		= CHANGES_ROOT_URL
+		  "gcc-12/changes.html#mips_cxx17_empty_bases";
+	      inform (input_location,
+		      "the ABI for returning a value with C++ 17 empty "
+		      "bases but otherwise an aggregate with only one or "
+		      "two floating-point fields was changed in GCC "
+		      "%{12.1%}", url);
+	      last_reported_type_uid = uid;
+	    }
+	}
+
       /* Handle structures whose fields are returned in $f0/$f2.  */
       switch (use_fpr)
 	{
diff --git a/gcc/testsuite/g++.target/mips/cxx17_empty_base.C b/gcc/testsuite/g++.target/mips/cxx17_empty_base.C
new file mode 100644
index 00000000000..da6d54b95d9
--- /dev/null
+++ b/gcc/testsuite/g++.target/mips/cxx17_empty_base.C
@@ -0,0 +1,20 @@
+// { dg-do compile }
+// { dg-options "-O2 -std=c++17 -mabi=64 -mhard-float" }
+// { dg-final { scan-assembler "\\\$f0" } }
+
+struct empty {};
+
+struct foo : empty
+{
+  double a;
+  double b;
+};
+
+struct foo
+make_foo(void) // { dg-message "the ABI for returning a value with C\\\+\\\+ 17 empty bases but otherwise an aggregate with only one or two floating-point fields was changed in GCC 12.1" }
+{
+  struct foo ret;
+  ret.a = 114;
+  ret.b = 514;
+  return ret;
+}
-- 
2.35.1

Re: [PATCH v2] mips: Fix C++14 vs. C++17 ABI incompatibility on mips64

[Jakub Jelinek]

On Wed, Apr 06, 2022 at 09:44:33PM +0800, Xi Ruoyao wrote:
> @@ -6527,6 +6554,27 @@ mips_function_value_1 (const_tree valtype, const_tree fn_decl_or_type,
>        if (has_cxx_zero_width_bf)
>  	use_fpr = 0;
>  
> +      if (TARGET_HARD_FLOAT
> +	  && warn_psabi
> +	  && use_fpr != 0
> +	  && has_cxx17_empty_base)
> +	{
> +	  static unsigned last_reported_type_uid;
> +	  unsigned uid = TYPE_UID (TYPE_MAIN_VARIANT (valtype));
> +	  if (uid != last_reported_type_uid)
> +	    {
> +	      static const char *url
> +		= CHANGES_ROOT_URL
> +		  "gcc-12/changes.html#mips_cxx17_empty_bases";
> +	      inform (input_location,
> +		      "the ABI for returning a value with C++ 17 empty "

Better write C++17 without space, that is what is used elsewhere too.
Ok with that change.

	Jakub

Re: [PATCH v2] mips: Fix C++14 vs. C++17 ABI incompatibility on mips64

[Xi Ruoyao]

On Wed, 2022-04-06 at 16:34 +0200, Jakub Jelinek wrote:
> On Wed, Apr 06, 2022 at 09:44:33PM +0800, Xi Ruoyao wrote:
> > @@ -6527,6 +6554,27 @@ mips_function_value_1 (const_tree valtype,
> > const_tree fn_decl_or_type,
> >        if (has_cxx_zero_width_bf)
> >         use_fpr = 0;
> >  
> > +      if (TARGET_HARD_FLOAT
> > +         && warn_psabi
> > +         && use_fpr != 0
> > +         && has_cxx17_empty_base)
> > +       {
> > +         static unsigned last_reported_type_uid;
> > +         unsigned uid = TYPE_UID (TYPE_MAIN_VARIANT (valtype));
> > +         if (uid != last_reported_type_uid)
> > +           {
> > +             static const char *url
> > +               = CHANGES_ROOT_URL
> > +                 "gcc-12/changes.html#mips_cxx17_empty_bases";
> > +             inform (input_location,
> > +                     "the ABI for returning a value with C++ 17
> > empty "
> 
> Better write C++17 without space, that is what is used elsewhere too.
> Ok with that change.

Pushed r12-8023 with the whitespace change.

-- 
Xi Ruoyao <xry111@mengyan1223.wang>
School of Aerospace Science and Technology, Xidian University