[PATCH] s390: Fix up -Wpsabi diagnostcs + analysis [PR94704]

[PATCH] s390: Fix up -Wpsabi diagnostcs + analysis [PR94704]

[Jakub Jelinek]

Hi!

So, based on the yesterday's discussions, similarly to powerpc64le-linux
I've done some testing for s390x-linux too.

First of all, I found a bug in my patch from yesterday, it was printing
the wrong type like 'double' etc. rather than the class that contained such
the element.  Fix below.

For s390x-linux, I was using
struct X { };
struct Y { int : 0; };
struct Z { int : 0; Y y; };
struct U : public X { X q; };
struct A { double a; };
struct B : public X { double a; };
struct C : public Y { double a; };
struct D : public Z { double a; };
struct E : public U { double a; };
struct F { [[no_unique_address]] X x; double a; };
struct G { [[no_unique_address]] Y y; double a; };
struct H { [[no_unique_address]] Z z; double a; };
struct I { [[no_unique_address]] U u; double a; };
struct J { double a; [[no_unique_address]] X x; };
struct K { double a; [[no_unique_address]] Y y; };
struct L { double a; [[no_unique_address]] Z z; };
struct M { double a; [[no_unique_address]] U u; };
#define T(S, s) extern S s; extern void foo##s (S); int bar##s () { foo##s (s); return 0; }
T (A, a)
T (B, b)
T (C, c)
T (D, d)
T (E, e)
T (F, f)
T (G, g)
T (H, h)
T (I, i)
T (J, j)
T (K, k)
T (L, l)
T (M, m)
as testcase and looking for "\tld\t%f0,".
While g++ 9 with -std=c++17 used to pass in fpr just
A, g++ 9 -std=c++14, as well as current trunk -std=c++14 & 17
and clang++ from today -std=c++14 & 17 all pass A, B, C
in fpr and nothing else.  The intent stated by Jason seems to be
that A, B, C, F, G, J, K should all be passed in fpr.  So, shall
we change both GCC and clang?  Or is the published ABI clear on this?
Or does it need clarification?

2020-04-28  Jakub Jelinek  <jakub@redhat.com>

	PR target/94704
	* config/s390/s390.c (s390_function_arg_vector,
	s390_function_arg_float): In -Wpsabi diagnostics use the type
	passed to the function rather than the type of the single element.

--- gcc/config/s390/s390.c.jj	2020-04-28 10:26:08.499984223 +0200
+++ gcc/config/s390/s390.c	2020-04-28 14:01:31.813712290 +0200
@@ -11912,6 +11912,7 @@ s390_function_arg_vector (machine_mode m
   /* The ABI says that record types with a single member are treated
      just like that member would be.  */
   bool cxx17_empty_base_seen = false;
+  const_tree orig_type = type;
   while (TREE_CODE (type) == RECORD_TYPE)
     {
       tree field, single = NULL_TREE;
@@ -11958,7 +11959,7 @@ s390_function_arg_vector (machine_mode m
 	  last_reported_type_uid = uid;
 	  inform (input_location, "parameter passing for argument of type "
 				  "%qT when C++17 is enabled changed to match "
-				  "C++14 in GCC 10.1", type);
+				  "C++14 in GCC 10.1", orig_type);
 	}
     }
   return true;
@@ -11984,6 +11985,7 @@ s390_function_arg_float (machine_mode mo
   /* The ABI says that record types with a single member are treated
      just like that member would be.  */
   bool cxx17_empty_base_seen = false;
+  const_tree orig_type = type;
   while (TREE_CODE (type) == RECORD_TYPE)
     {
       tree field, single = NULL_TREE;
@@ -12022,7 +12024,7 @@ s390_function_arg_float (machine_mode mo
 	  last_reported_type_uid = uid;
 	  inform (input_location, "parameter passing for argument of type "
 				  "%qT when C++17 is enabled changed to match "
-				  "C++14 in GCC 10.1", type);
+				  "C++14 in GCC 10.1", orig_type);
 	}
     }
 

	Jakub

Re: [PATCH] s390: Fix up -Wpsabi diagnostcs + analysis [PR94704]

[Andreas Krebbel]

On 28.04.20 14:13, Jakub Jelinek wrote:
> Hi!
> 
> So, based on the yesterday's discussions, similarly to powerpc64le-linux
> I've done some testing for s390x-linux too.
> 
> First of all, I found a bug in my patch from yesterday, it was printing
> the wrong type like 'double' etc. rather than the class that contained such
> the element.  Fix below.
> 
> For s390x-linux, I was using
> struct X { };
> struct Y { int : 0; };
> struct Z { int : 0; Y y; };
> struct U : public X { X q; };
> struct A { double a; };
> struct B : public X { double a; };
> struct C : public Y { double a; };
> struct D : public Z { double a; };
> struct E : public U { double a; };
> struct F { [[no_unique_address]] X x; double a; };
> struct G { [[no_unique_address]] Y y; double a; };
> struct H { [[no_unique_address]] Z z; double a; };
> struct I { [[no_unique_address]] U u; double a; };
> struct J { double a; [[no_unique_address]] X x; };
> struct K { double a; [[no_unique_address]] Y y; };
> struct L { double a; [[no_unique_address]] Z z; };
> struct M { double a; [[no_unique_address]] U u; };
> #define T(S, s) extern S s; extern void foo##s (S); int bar##s () { foo##s (s); return 0; }
> T (A, a)
> T (B, b)
> T (C, c)
> T (D, d)
> T (E, e)
> T (F, f)
> T (G, g)
> T (H, h)
> T (I, i)
> T (J, j)
> T (K, k)
> T (L, l)
> T (M, m)
> as testcase and looking for "\tld\t%f0,".
> While g++ 9 with -std=c++17 used to pass in fpr just
> A, g++ 9 -std=c++14, as well as current trunk -std=c++14 & 17
> and clang++ from today -std=c++14 & 17 all pass A, B, C
> in fpr and nothing else.  The intent stated by Jason seems to be
> that A, B, C, F, G, J, K should all be passed in fpr.  So, shall
> we change both GCC and clang?  Or is the published ABI clear on this?
> Or does it need clarification?

Our ABI doesn't specify anything regarding C++ so there is nothing in there which really conflicts
with that. I assume these things will be part of a cross-platform C++ ABI instead? I don't see a way
to add this to our platform ABI without introducing C++ in general there.

Since "no_unique_address" is a new feature we want to pick whatever is most efficient and matches
what other archs do. Passing F, G, J, K in FPRs looks reasonable to me.

Given that this is something which hasn't been covered by the ABI so far I'm not sure we really need
a -Wpsabi warning for that.

I also checked with Ulrich. He agrees with that approach and will have a look at the LLVM side.

Btw. is the no_unique_address flag visible also in Dwarf? Probably it is enough for GDB to just
observe the effect of it to decide how to pass arguments?!

Thanks for doing these investigations!

Andreas


> 
> 2020-04-28  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR target/94704
> 	* config/s390/s390.c (s390_function_arg_vector,
> 	s390_function_arg_float): In -Wpsabi diagnostics use the type
> 	passed to the function rather than the type of the single element.

Ok. Thanks!

Andreas

> 
> --- gcc/config/s390/s390.c.jj	2020-04-28 10:26:08.499984223 +0200
> +++ gcc/config/s390/s390.c	2020-04-28 14:01:31.813712290 +0200
> @@ -11912,6 +11912,7 @@ s390_function_arg_vector (machine_mode m
>    /* The ABI says that record types with a single member are treated
>       just like that member would be.  */
>    bool cxx17_empty_base_seen = false;
> +  const_tree orig_type = type;
>    while (TREE_CODE (type) == RECORD_TYPE)
>      {
>        tree field, single = NULL_TREE;
> @@ -11958,7 +11959,7 @@ s390_function_arg_vector (machine_mode m
>  	  last_reported_type_uid = uid;
>  	  inform (input_location, "parameter passing for argument of type "
>  				  "%qT when C++17 is enabled changed to match "
> -				  "C++14 in GCC 10.1", type);
> +				  "C++14 in GCC 10.1", orig_type);
>  	}
>      }
>    return true;
> @@ -11984,6 +11985,7 @@ s390_function_arg_float (machine_mode mo
>    /* The ABI says that record types with a single member are treated
>       just like that member would be.  */
>    bool cxx17_empty_base_seen = false;
> +  const_tree orig_type = type;
>    while (TREE_CODE (type) == RECORD_TYPE)
>      {
>        tree field, single = NULL_TREE;
> @@ -12022,7 +12024,7 @@ s390_function_arg_float (machine_mode mo
>  	  last_reported_type_uid = uid;
>  	  inform (input_location, "parameter passing for argument of type "
>  				  "%qT when C++17 is enabled changed to match "
> -				  "C++14 in GCC 10.1", type);
> +				  "C++14 in GCC 10.1", orig_type);
>  	}
>      }
>  
> 
> 	Jakub
> 

Re: [PATCH] s390: Fix up -Wpsabi diagnostcs + analysis [PR94704]

[Jakub Jelinek]

On Tue, Apr 28, 2020 at 04:23:24PM +0200, Andreas Krebbel wrote:
> Our ABI doesn't specify anything regarding C++ so there is nothing in there which really conflicts
> with that. I assume these things will be part of a cross-platform C++ ABI instead? I don't see a way
> to add this to our platform ABI without introducing C++ in general there.
> 
> Since "no_unique_address" is a new feature we want to pick whatever is most efficient and matches
> what other archs do. Passing F, G, J, K in FPRs looks reasonable to me.

Ok, will tweak the patch then once the powerpc+generic one is finalized.

> Given that this is something which hasn't been covered by the ABI so far I'm not sure we really need
> a -Wpsabi warning for that.

But [[no_unique_address]] has been introduced already in GCC 9, so e.g. in
the powerpc64le patch I want to warn users about the ABI incompatibility
when padding those.
Your call though.

> Btw. is the no_unique_address flag visible also in Dwarf? Probably it is enough for GDB to just
> observe the effect of it to decide how to pass arguments?!

Dunno.

	Jakub

Re: [PATCH] s390: Fix up -Wpsabi diagnostcs + analysis [PR94704]

[Jason Merrill]

On 4/28/20 10:38 AM, Jakub Jelinek wrote:
> On Tue, Apr 28, 2020 at 04:23:24PM +0200, Andreas Krebbel wrote:
>> Our ABI doesn't specify anything regarding C++ so there is nothing in there which really conflicts
>> with that. I assume these things will be part of a cross-platform C++ ABI instead? I don't see a way
>> to add this to our platform ABI without introducing C++ in general there.
>>
>> Since "no_unique_address" is a new feature we want to pick whatever is most efficient and matches
>> what other archs do. Passing F, G, J, K in FPRs looks reasonable to me.
> 
> Ok, will tweak the patch then once the powerpc+generic one is finalized.
> 
>> Given that this is something which hasn't been covered by the ABI so far I'm not sure we really need
>> a -Wpsabi warning for that.
> 
> But [[no_unique_address]] has been introduced already in GCC 9, so e.g. in
> the powerpc64le patch I want to warn users about the ABI incompatibility
> when padding those.
> Your call though.

Though C++20 support is still highly experimental, we're not trying to 
maintain compatibility.

>> Btw. is the no_unique_address flag visible also in Dwarf? Probably it is enough for GDB to just
>> observe the effect of it to decide how to pass arguments?!

I agree that's probably enough.

Jason

Re: [PATCH] s390: Fix up -Wpsabi diagnostcs + analysis [PR94704]

[Jakub Jelinek]

[-- Attachment #1: Type: text/plain, Size: 615 bytes --]

On Tue, Apr 28, 2020 at 04:23:24PM +0200, Andreas Krebbel via Gcc-patches wrote:
> Given that this is something which hasn't been covered by the ABI so far I'm not sure we really need
> a -Wpsabi warning for that.

Attached are two (updated) versions of the patch on top of the
powerpc+middle-end patch just posted.

The first one emits two separate -Wpsabi warnings like powerpc, one for
the -std=c++14 vs. -std=c++17 ABI difference and one for GCC 9 vs. 10
[[no_unique_address]] passing changes, the other one is silent about the
second case.  Will bootstrap/regtest both and you can choose.

	Jakub

[-- Attachment #2: S396 --]
[-- Type: text/plain, Size: 13556 bytes --]

2020-04-28  Jakub Jelinek  <jakub@redhat.com>

	PR target/94704
	* config/s390/s390.c (s390_function_arg_vector,
	s390_function_arg_float): Use DECL_FIELD_ABI_IGNORED instead of
	cxx17_empty_base_field_p.  In -Wpsabi diagnostics use the type
	passed to the function rather than the type of the single element.
	Rename cxx17_empty_base_seen variable to empty_base_seen, change
	type to int, and adjust diagnostics depending on if the field
	has [[no_unique_attribute]] or not.

	* g++.target/s390/s390.exp: New file.
	* g++.target/s390/pr94704-1.C: New test.
	* g++.target/s390/pr94704-2.C: New test.
	* g++.target/s390/pr94704-3.C: New test.
	* g++.target/s390/pr94704-4.C: New test.

--- gcc/config/s390/s390.c.jj	2020-04-28 16:15:39.542541382 +0200
+++ gcc/config/s390/s390.c	2020-04-28 17:20:06.087872951 +0200
@@ -11911,7 +11911,8 @@ s390_function_arg_vector (machine_mode m
 
   /* The ABI says that record types with a single member are treated
      just like that member would be.  */
-  bool cxx17_empty_base_seen = false;
+  int empty_base_seen = 0;
+  const_tree orig_type = type;
   while (TREE_CODE (type) == RECORD_TYPE)
     {
       tree field, single = NULL_TREE;
@@ -11921,9 +11922,13 @@ s390_function_arg_vector (machine_mode m
 	  if (TREE_CODE (field) != FIELD_DECL)
 	    continue;
 
-	  if (cxx17_empty_base_field_p (field))
+	  if (DECL_FIELD_ABI_IGNORED (field))
 	    {
-	      cxx17_empty_base_seen = true;
+	      if (lookup_attribute ("no_unique_address",
+				    DECL_ATTRIBUTES (field)))
+		empty_base_seen |= 2;
+	      else
+		empty_base_seen |= 1;
 	      continue;
 	    }
 
@@ -11949,16 +11954,23 @@ s390_function_arg_vector (machine_mode m
   if (!VECTOR_TYPE_P (type))
     return false;
 
-  if (warn_psabi && cxx17_empty_base_seen)
+  if (warn_psabi && empty_base_seen)
     {
       static unsigned last_reported_type_uid;
-      unsigned uid = TYPE_UID (TYPE_MAIN_VARIANT (type));
+      unsigned uid = TYPE_UID (TYPE_MAIN_VARIANT (orig_type));
       if (uid != last_reported_type_uid)
 	{
 	  last_reported_type_uid = uid;
-	  inform (input_location, "parameter passing for argument of type "
-				  "%qT when C++17 is enabled changed to match "
-				  "C++14 in GCC 10.1", type);
+	  if (empty_base_seen & 1)
+	    inform (input_location,
+		    "parameter passing for argument of type %qT when C++17 "
+		    "is enabled changed to match C++14 in GCC 10.1",
+		    orig_type);
+	  else
+	    inform (input_location,
+		    "parameter passing for argument of type %qT with "
+		    "%<[[no_unique_address]]%> members changed in GCC 10.1",
+		    orig_type);
 	}
     }
   return true;
@@ -11983,7 +11995,8 @@ s390_function_arg_float (machine_mode mo
 
   /* The ABI says that record types with a single member are treated
      just like that member would be.  */
-  bool cxx17_empty_base_seen = false;
+  int empty_base_seen = 0;
+  const_tree orig_type = type;
   while (TREE_CODE (type) == RECORD_TYPE)
     {
       tree field, single = NULL_TREE;
@@ -11992,9 +12005,13 @@ s390_function_arg_float (machine_mode mo
 	{
 	  if (TREE_CODE (field) != FIELD_DECL)
 	    continue;
-	  if (cxx17_empty_base_field_p (field))
+	  if (DECL_FIELD_ABI_IGNORED (field))
 	    {
-	      cxx17_empty_base_seen = true;
+	      if (lookup_attribute ("no_unique_address",
+				    DECL_ATTRIBUTES (field)))
+		empty_base_seen |= 2;
+	      else
+		empty_base_seen |= 1;
 	      continue;
 	    }
 
@@ -12013,16 +12030,23 @@ s390_function_arg_float (machine_mode mo
   if (TREE_CODE (type) != REAL_TYPE)
     return false;
 
-  if (warn_psabi && cxx17_empty_base_seen)
+  if (warn_psabi && empty_base_seen)
     {
       static unsigned last_reported_type_uid;
-      unsigned uid = TYPE_UID (TYPE_MAIN_VARIANT (type));
+      unsigned uid = TYPE_UID (TYPE_MAIN_VARIANT (orig_type));
       if (uid != last_reported_type_uid)
 	{
 	  last_reported_type_uid = uid;
-	  inform (input_location, "parameter passing for argument of type "
-				  "%qT when C++17 is enabled changed to match "
-				  "C++14 in GCC 10.1", type);
+	  if (empty_base_seen & 1)
+	    inform (input_location,
+		    "parameter passing for argument of type %qT when C++17 "
+		    "is enabled changed to match C++14 in GCC 10.1",
+		    orig_type);
+	  else
+	    inform (input_location,
+		    "parameter passing for argument of type %qT with "
+		    "%<[[no_unique_address]]%> members changed in GCC 10.1",
+		    orig_type);
 	}
     }
 
--- gcc/testsuite/g++.target/s390/s390.exp.jj	2020-04-28 17:10:05.282726486 +0200
+++ gcc/testsuite/g++.target/s390/s390.exp	2020-04-28 17:10:37.933240905 +0200
@@ -0,0 +1,44 @@
+#  Specific regression driver for S390.
+#  Copyright (C) 2020 Free Software Foundation, Inc.
+#
+#  This file is part of GCC.
+#
+#  GCC is free software; you can redistribute it and/or modify it
+#  under the terms of the GNU General Public License as published by
+#  the Free Software Foundation; either version 3, or (at your option)
+#  any later version.
+#
+#  GCC is distributed in the hope that it will be useful, but
+#  WITHOUT ANY WARRANTY; without even the implied warranty of
+#  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+#  General Public License for more details.
+#
+#  You should have received a copy of the GNU General Public License
+#  along with GCC; see the file COPYING3.  If not see
+#  <http://www.gnu.org/licenses/>.  */
+
+# GCC testsuite that uses the `dg.exp' driver.
+
+# Exit immediately if this isn't a s390 target.
+if {![istarget s390*-*-*] } then {
+  return
+}
+
+# Load support procs.
+load_lib g++-dg.exp
+
+global DEFAULT_CXXFLAGS
+if ![info exists DEFAULT_CXXFLAGS] then {
+    set DEFAULT_CXXFLAGS " -pedantic-errors"
+}
+
+# Initialize `dg'.
+dg-init
+
+# Main loop.
+dg-runtest [lsort [glob -nocomplain $srcdir/$subdir/*.C]] \
+        "" $DEFAULT_CXXFLAGS
+
+# All done.
+dg-finish
+
--- gcc/testsuite/g++.target/s390/pr94704-1.C.jj	2020-04-28 16:33:53.431130852 +0200
+++ gcc/testsuite/g++.target/s390/pr94704-1.C	2020-04-28 17:30:04.542094168 +0200
@@ -0,0 +1,38 @@
+// PR target/94704
+// { dg-do compile }
+// { dg-options "-O2 -std=c++14" }
+// Test that for all the calls in this testcase the C++17 empty base
+// artificial fields and [[no_unique_address]] empty class non-static
+// data members are ignored in the decision whether passed arguments
+// have a single floating point field.
+// { dg-final { scan-assembler-times {(?n)^\s+ld\s+%f0,} 7 } }
+
+struct X { };
+struct Y { int : 0; };
+struct Z { int : 0; Y y; };
+struct U : public X { X q; };
+struct A { double a; };
+struct B : public X { double a; };
+struct C : public Y { double a; };
+struct D : public Z { double a; };
+struct E : public U { double a; };
+struct F { [[no_unique_address]] X x; double a; };
+struct G { [[no_unique_address]] Y y; double a; };
+struct H { [[no_unique_address]] Z z; double a; };
+struct I { [[no_unique_address]] U u; double a; };
+struct J { double a; [[no_unique_address]] X x; };
+struct K { double a; [[no_unique_address]] Y y; };
+struct L { double a; [[no_unique_address]] Z z; };
+struct M { double a; [[no_unique_address]] U u; };
+#define T(S, s) extern S s; extern void foo##s (S); int bar##s () { foo##s (s); return 0; }
+// { dg-message "parameter passing for argument of type 'F' with '\\\[\\\[no_unique_address\\\]\\\]' members changed in GCC 10.1" "" { target *-*-* } .-1 }
+// { dg-message "parameter passing for argument of type 'G' with '\\\[\\\[no_unique_address\\\]\\\]' members changed in GCC 10.1" "" { target *-*-* } .-2 }
+// { dg-message "parameter passing for argument of type 'J' with '\\\[\\\[no_unique_address\\\]\\\]' members changed in GCC 10.1" "" { target *-*-* } .-3 }
+// { dg-message "parameter passing for argument of type 'K' with '\\\[\\\[no_unique_address\\\]\\\]' members changed in GCC 10.1" "" { target *-*-* } .-4 }
+T (A, a)
+T (B, b)
+T (C, c)
+T (F, f)
+T (G, g)
+T (J, j)
+T (K, k)
--- gcc/testsuite/g++.target/s390/pr94704-2.C.jj	2020-04-28 16:33:53.431130852 +0200
+++ gcc/testsuite/g++.target/s390/pr94704-2.C	2020-04-28 17:30:38.568586476 +0200
@@ -0,0 +1,34 @@
+// PR target/94704
+// { dg-do compile }
+// { dg-options "-O2 -std=c++14" }
+// Test that for no calls in this testcase the C++17 empty base
+// artificial fields and [[no_unique_address]] empty class non-static
+// data members are ignored in the decision whether passed arguments
+// have a single floating point field.
+// { dg-final { scan-assembler-not {(?n)^\s+ld\s+%f0,} } }
+
+struct X { };
+struct Y { int : 0; };
+struct Z { int : 0; Y y; };
+struct U : public X { X q; };
+struct A { double a; };
+struct B : public X { double a; };
+struct C : public Y { double a; };
+struct D : public Z { double a; };
+struct E : public U { double a; };
+struct F { [[no_unique_address]] X x; double a; };
+struct G { [[no_unique_address]] Y y; double a; };
+struct H { [[no_unique_address]] Z z; double a; };
+struct I { [[no_unique_address]] U u; double a; };
+struct J { double a; [[no_unique_address]] X x; };
+struct K { double a; [[no_unique_address]] Y y; };
+struct L { double a; [[no_unique_address]] Z z; };
+struct M { double a; [[no_unique_address]] U u; };
+#define T(S, s) extern S s; extern void foo##s (S); int bar##s () { foo##s (s); return 0; }
+// { dg-bogus "parameter passing for argument of type" }
+T (D, d)
+T (E, e)
+T (H, h)
+T (I, i)
+T (L, l)
+T (M, m)
--- gcc/testsuite/g++.target/s390/pr94704-3.C.jj	2020-04-28 16:33:53.431130852 +0200
+++ gcc/testsuite/g++.target/s390/pr94704-3.C	2020-04-28 17:30:18.063892415 +0200
@@ -0,0 +1,40 @@
+// PR target/94704
+// { dg-do compile }
+// { dg-options "-O2 -std=c++17" }
+// Test that for all the calls in this testcase the C++17 empty base
+// artificial fields and [[no_unique_address]] empty class non-static
+// data members are ignored in the decision whether passed arguments
+// have a single floating point field.
+// { dg-final { scan-assembler-times {(?n)^\s+ld\s+%f0,} 7 } }
+
+struct X { };
+struct Y { int : 0; };
+struct Z { int : 0; Y y; };
+struct U : public X { X q; };
+struct A { double a; };
+struct B : public X { double a; };
+struct C : public Y { double a; };
+struct D : public Z { double a; };
+struct E : public U { double a; };
+struct F { [[no_unique_address]] X x; double a; };
+struct G { [[no_unique_address]] Y y; double a; };
+struct H { [[no_unique_address]] Z z; double a; };
+struct I { [[no_unique_address]] U u; double a; };
+struct J { double a; [[no_unique_address]] X x; };
+struct K { double a; [[no_unique_address]] Y y; };
+struct L { double a; [[no_unique_address]] Z z; };
+struct M { double a; [[no_unique_address]] U u; };
+#define T(S, s) extern S s; extern void foo##s (S); int bar##s () { foo##s (s); return 0; }
+// { dg-message "parameter passing for argument of type 'B' when C\\+\\+17 is enabled changed to match C\\+\\+14 in GCC 10.1" "" { target *-*-* } .-1 }
+// { dg-message "parameter passing for argument of type 'C' when C\\+\\+17 is enabled changed to match C\\+\\+14 in GCC 10.1" "" { target *-*-* } .-2 }
+// { dg-message "parameter passing for argument of type 'F' with '\\\[\\\[no_unique_address\\\]\\\]' members changed in GCC 10.1" "" { target *-*-* } .-3 }
+// { dg-message "parameter passing for argument of type 'G' with '\\\[\\\[no_unique_address\\\]\\\]' members changed in GCC 10.1" "" { target *-*-* } .-4 }
+// { dg-message "parameter passing for argument of type 'J' with '\\\[\\\[no_unique_address\\\]\\\]' members changed in GCC 10.1" "" { target *-*-* } .-5 }
+// { dg-message "parameter passing for argument of type 'K' with '\\\[\\\[no_unique_address\\\]\\\]' members changed in GCC 10.1" "" { target *-*-* } .-6 }
+T (A, a)
+T (B, b)
+T (C, c)
+T (F, f)
+T (G, g)
+T (J, j)
+T (K, k)
--- gcc/testsuite/g++.target/s390/pr94704-4.C.jj	2020-04-28 16:33:53.431130852 +0200
+++ gcc/testsuite/g++.target/s390/pr94704-4.C	2020-04-28 17:30:28.296739746 +0200
@@ -0,0 +1,34 @@
+// PR target/94704
+// { dg-do compile }
+// { dg-options "-O2 -std=c++17" }
+// Test that for no calls in this testcase the C++17 empty base
+// artificial fields and [[no_unique_address]] empty class non-static
+// data members are ignored in the decision whether passed arguments
+// have a single floating point field.
+// { dg-final { scan-assembler-not {(?n)^\s+ld\s+%f0,} } }
+
+struct X { };
+struct Y { int : 0; };
+struct Z { int : 0; Y y; };
+struct U : public X { X q; };
+struct A { double a; };
+struct B : public X { double a; };
+struct C : public Y { double a; };
+struct D : public Z { double a; };
+struct E : public U { double a; };
+struct F { [[no_unique_address]] X x; double a; };
+struct G { [[no_unique_address]] Y y; double a; };
+struct H { [[no_unique_address]] Z z; double a; };
+struct I { [[no_unique_address]] U u; double a; };
+struct J { double a; [[no_unique_address]] X x; };
+struct K { double a; [[no_unique_address]] Y y; };
+struct L { double a; [[no_unique_address]] Z z; };
+struct M { double a; [[no_unique_address]] U u; };
+#define T(S, s) extern S s; extern void foo##s (S); int bar##s () { foo##s (s); return 0; }
+// { dg-bogus "parameter passing for argument of type" }
+T (D, d)
+T (E, e)
+T (H, h)
+T (I, i)
+T (L, l)
+T (M, m)

[-- Attachment #3: S396b --]
[-- Type: text/plain, Size: 11359 bytes --]

2020-04-28  Jakub Jelinek  <jakub@redhat.com>

	PR target/94704
	* config/s390/s390.c (s390_function_arg_vector,
	s390_function_arg_float): Use DECL_FIELD_ABI_IGNORED instead of
	cxx17_empty_base_field_p.  In -Wpsabi diagnostics use the type
	passed to the function rather than the type of the single element.

	* g++.target/s390/s390.exp: New file.
	* g++.target/s390/pr94704-1.C: New test.
	* g++.target/s390/pr94704-2.C: New test.
	* g++.target/s390/pr94704-3.C: New test.
	* g++.target/s390/pr94704-4.C: New test.

--- gcc/config/s390/s390.c.jj	2020-04-28 16:15:39.542541382 +0200
+++ gcc/config/s390/s390.c	2020-04-28 17:38:17.037864167 +0200
@@ -11912,6 +11912,7 @@ s390_function_arg_vector (machine_mode m
   /* The ABI says that record types with a single member are treated
      just like that member would be.  */
   bool cxx17_empty_base_seen = false;
+  const_tree orig_type = type;
   while (TREE_CODE (type) == RECORD_TYPE)
     {
       tree field, single = NULL_TREE;
@@ -11921,9 +11922,11 @@ s390_function_arg_vector (machine_mode m
 	  if (TREE_CODE (field) != FIELD_DECL)
 	    continue;
 
-	  if (cxx17_empty_base_field_p (field))
+	  if (DECL_FIELD_ABI_IGNORED (field))
 	    {
-	      cxx17_empty_base_seen = true;
+	      if (!lookup_attribute ("no_unique_address",
+				     DECL_ATTRIBUTES (field)))
+		cxx17_empty_base_seen = true;
 	      continue;
 	    }
 
@@ -11952,13 +11955,14 @@ s390_function_arg_vector (machine_mode m
   if (warn_psabi && cxx17_empty_base_seen)
     {
       static unsigned last_reported_type_uid;
-      unsigned uid = TYPE_UID (TYPE_MAIN_VARIANT (type));
+      unsigned uid = TYPE_UID (TYPE_MAIN_VARIANT (orig_type));
       if (uid != last_reported_type_uid)
 	{
 	  last_reported_type_uid = uid;
-	  inform (input_location, "parameter passing for argument of type "
-				  "%qT when C++17 is enabled changed to match "
-				  "C++14 in GCC 10.1", type);
+	  inform (input_location,
+		  "parameter passing for argument of type %qT when C++17 "
+		  "is enabled changed to match C++14 in GCC 10.1",
+		  orig_type);
 	}
     }
   return true;
@@ -11984,6 +11988,7 @@ s390_function_arg_float (machine_mode mo
   /* The ABI says that record types with a single member are treated
      just like that member would be.  */
   bool cxx17_empty_base_seen = false;
+  const_tree orig_type = type;
   while (TREE_CODE (type) == RECORD_TYPE)
     {
       tree field, single = NULL_TREE;
@@ -11992,9 +11997,11 @@ s390_function_arg_float (machine_mode mo
 	{
 	  if (TREE_CODE (field) != FIELD_DECL)
 	    continue;
-	  if (cxx17_empty_base_field_p (field))
+	  if (DECL_FIELD_ABI_IGNORED (field))
 	    {
-	      cxx17_empty_base_seen = true;
+	      if (!lookup_attribute ("no_unique_address",
+				     DECL_ATTRIBUTES (field)))
+		cxx17_empty_base_seen = true;
 	      continue;
 	    }
 
@@ -12016,13 +12023,14 @@ s390_function_arg_float (machine_mode mo
   if (warn_psabi && cxx17_empty_base_seen)
     {
       static unsigned last_reported_type_uid;
-      unsigned uid = TYPE_UID (TYPE_MAIN_VARIANT (type));
+      unsigned uid = TYPE_UID (TYPE_MAIN_VARIANT (orig_type));
       if (uid != last_reported_type_uid)
 	{
 	  last_reported_type_uid = uid;
-	  inform (input_location, "parameter passing for argument of type "
-				  "%qT when C++17 is enabled changed to match "
-				  "C++14 in GCC 10.1", type);
+	  inform (input_location,
+		  "parameter passing for argument of type %qT when C++17 "
+		  "is enabled changed to match C++14 in GCC 10.1",
+		  orig_type);
 	}
     }
 
--- gcc/testsuite/g++.target/s390/s390.exp.jj	2020-04-28 17:10:05.282726486 +0200
+++ gcc/testsuite/g++.target/s390/s390.exp	2020-04-28 17:10:37.933240905 +0200
@@ -0,0 +1,44 @@
+#  Specific regression driver for S390.
+#  Copyright (C) 2020 Free Software Foundation, Inc.
+#
+#  This file is part of GCC.
+#
+#  GCC is free software; you can redistribute it and/or modify it
+#  under the terms of the GNU General Public License as published by
+#  the Free Software Foundation; either version 3, or (at your option)
+#  any later version.
+#
+#  GCC is distributed in the hope that it will be useful, but
+#  WITHOUT ANY WARRANTY; without even the implied warranty of
+#  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+#  General Public License for more details.
+#
+#  You should have received a copy of the GNU General Public License
+#  along with GCC; see the file COPYING3.  If not see
+#  <http://www.gnu.org/licenses/>.  */
+
+# GCC testsuite that uses the `dg.exp' driver.
+
+# Exit immediately if this isn't a s390 target.
+if {![istarget s390*-*-*] } then {
+  return
+}
+
+# Load support procs.
+load_lib g++-dg.exp
+
+global DEFAULT_CXXFLAGS
+if ![info exists DEFAULT_CXXFLAGS] then {
+    set DEFAULT_CXXFLAGS " -pedantic-errors"
+}
+
+# Initialize `dg'.
+dg-init
+
+# Main loop.
+dg-runtest [lsort [glob -nocomplain $srcdir/$subdir/*.C]] \
+        "" $DEFAULT_CXXFLAGS
+
+# All done.
+dg-finish
+
--- gcc/testsuite/g++.target/s390/pr94704-1.C.jj	2020-04-28 16:33:53.431130852 +0200
+++ gcc/testsuite/g++.target/s390/pr94704-1.C	2020-04-28 17:38:59.895236862 +0200
@@ -0,0 +1,34 @@
+// PR target/94704
+// { dg-do compile }
+// { dg-options "-O2 -std=c++14" }
+// Test that for all the calls in this testcase the C++17 empty base
+// artificial fields and [[no_unique_address]] empty class non-static
+// data members are ignored in the decision whether passed arguments
+// have a single floating point field.
+// { dg-final { scan-assembler-times {(?n)^\s+ld\s+%f0,} 7 } }
+
+struct X { };
+struct Y { int : 0; };
+struct Z { int : 0; Y y; };
+struct U : public X { X q; };
+struct A { double a; };
+struct B : public X { double a; };
+struct C : public Y { double a; };
+struct D : public Z { double a; };
+struct E : public U { double a; };
+struct F { [[no_unique_address]] X x; double a; };
+struct G { [[no_unique_address]] Y y; double a; };
+struct H { [[no_unique_address]] Z z; double a; };
+struct I { [[no_unique_address]] U u; double a; };
+struct J { double a; [[no_unique_address]] X x; };
+struct K { double a; [[no_unique_address]] Y y; };
+struct L { double a; [[no_unique_address]] Z z; };
+struct M { double a; [[no_unique_address]] U u; };
+#define T(S, s) extern S s; extern void foo##s (S); int bar##s () { foo##s (s); return 0; }
+T (A, a)
+T (B, b)
+T (C, c)
+T (F, f)
+T (G, g)
+T (J, j)
+T (K, k)
--- gcc/testsuite/g++.target/s390/pr94704-2.C.jj	2020-04-28 16:33:53.431130852 +0200
+++ gcc/testsuite/g++.target/s390/pr94704-2.C	2020-04-28 17:30:38.568586476 +0200
@@ -0,0 +1,34 @@
+// PR target/94704
+// { dg-do compile }
+// { dg-options "-O2 -std=c++14" }
+// Test that for no calls in this testcase the C++17 empty base
+// artificial fields and [[no_unique_address]] empty class non-static
+// data members are ignored in the decision whether passed arguments
+// have a single floating point field.
+// { dg-final { scan-assembler-not {(?n)^\s+ld\s+%f0,} } }
+
+struct X { };
+struct Y { int : 0; };
+struct Z { int : 0; Y y; };
+struct U : public X { X q; };
+struct A { double a; };
+struct B : public X { double a; };
+struct C : public Y { double a; };
+struct D : public Z { double a; };
+struct E : public U { double a; };
+struct F { [[no_unique_address]] X x; double a; };
+struct G { [[no_unique_address]] Y y; double a; };
+struct H { [[no_unique_address]] Z z; double a; };
+struct I { [[no_unique_address]] U u; double a; };
+struct J { double a; [[no_unique_address]] X x; };
+struct K { double a; [[no_unique_address]] Y y; };
+struct L { double a; [[no_unique_address]] Z z; };
+struct M { double a; [[no_unique_address]] U u; };
+#define T(S, s) extern S s; extern void foo##s (S); int bar##s () { foo##s (s); return 0; }
+// { dg-bogus "parameter passing for argument of type" }
+T (D, d)
+T (E, e)
+T (H, h)
+T (I, i)
+T (L, l)
+T (M, m)
--- gcc/testsuite/g++.target/s390/pr94704-3.C.jj	2020-04-28 16:33:53.431130852 +0200
+++ gcc/testsuite/g++.target/s390/pr94704-3.C	2020-04-28 17:39:07.759121755 +0200
@@ -0,0 +1,36 @@
+// PR target/94704
+// { dg-do compile }
+// { dg-options "-O2 -std=c++17" }
+// Test that for all the calls in this testcase the C++17 empty base
+// artificial fields and [[no_unique_address]] empty class non-static
+// data members are ignored in the decision whether passed arguments
+// have a single floating point field.
+// { dg-final { scan-assembler-times {(?n)^\s+ld\s+%f0,} 7 } }
+
+struct X { };
+struct Y { int : 0; };
+struct Z { int : 0; Y y; };
+struct U : public X { X q; };
+struct A { double a; };
+struct B : public X { double a; };
+struct C : public Y { double a; };
+struct D : public Z { double a; };
+struct E : public U { double a; };
+struct F { [[no_unique_address]] X x; double a; };
+struct G { [[no_unique_address]] Y y; double a; };
+struct H { [[no_unique_address]] Z z; double a; };
+struct I { [[no_unique_address]] U u; double a; };
+struct J { double a; [[no_unique_address]] X x; };
+struct K { double a; [[no_unique_address]] Y y; };
+struct L { double a; [[no_unique_address]] Z z; };
+struct M { double a; [[no_unique_address]] U u; };
+#define T(S, s) extern S s; extern void foo##s (S); int bar##s () { foo##s (s); return 0; }
+// { dg-message "parameter passing for argument of type 'B' when C\\+\\+17 is enabled changed to match C\\+\\+14 in GCC 10.1" "" { target *-*-* } .-1 }
+// { dg-message "parameter passing for argument of type 'C' when C\\+\\+17 is enabled changed to match C\\+\\+14 in GCC 10.1" "" { target *-*-* } .-2 }
+T (A, a)
+T (B, b)
+T (C, c)
+T (F, f)
+T (G, g)
+T (J, j)
+T (K, k)
--- gcc/testsuite/g++.target/s390/pr94704-4.C.jj	2020-04-28 16:33:53.431130852 +0200
+++ gcc/testsuite/g++.target/s390/pr94704-4.C	2020-04-28 17:30:28.296739746 +0200
@@ -0,0 +1,34 @@
+// PR target/94704
+// { dg-do compile }
+// { dg-options "-O2 -std=c++17" }
+// Test that for no calls in this testcase the C++17 empty base
+// artificial fields and [[no_unique_address]] empty class non-static
+// data members are ignored in the decision whether passed arguments
+// have a single floating point field.
+// { dg-final { scan-assembler-not {(?n)^\s+ld\s+%f0,} } }
+
+struct X { };
+struct Y { int : 0; };
+struct Z { int : 0; Y y; };
+struct U : public X { X q; };
+struct A { double a; };
+struct B : public X { double a; };
+struct C : public Y { double a; };
+struct D : public Z { double a; };
+struct E : public U { double a; };
+struct F { [[no_unique_address]] X x; double a; };
+struct G { [[no_unique_address]] Y y; double a; };
+struct H { [[no_unique_address]] Z z; double a; };
+struct I { [[no_unique_address]] U u; double a; };
+struct J { double a; [[no_unique_address]] X x; };
+struct K { double a; [[no_unique_address]] Y y; };
+struct L { double a; [[no_unique_address]] Z z; };
+struct M { double a; [[no_unique_address]] U u; };
+#define T(S, s) extern S s; extern void foo##s (S); int bar##s () { foo##s (s); return 0; }
+// { dg-bogus "parameter passing for argument of type" }
+T (D, d)
+T (E, e)
+T (H, h)
+T (I, i)
+T (L, l)
+T (M, m)

Re: [PATCH] s390: Fix up -Wpsabi diagnostcs + analysis [PR94704]

[Andreas Krebbel]

On 28.04.20 17:44, Jakub Jelinek wrote:
> On Tue, Apr 28, 2020 at 04:23:24PM +0200, Andreas Krebbel via Gcc-patches wrote:
>> Given that this is something which hasn't been covered by the ABI so far I'm not sure we really need
>> a -Wpsabi warning for that.
> 
> Attached are two (updated) versions of the patch on top of the
> powerpc+middle-end patch just posted.
> 
> The first one emits two separate -Wpsabi warnings like powerpc, one for
> the -std=c++14 vs. -std=c++17 ABI difference and one for GCC 9 vs. 10
> [[no_unique_address]] passing changes, the other one is silent about the
> second case.  Will bootstrap/regtest both and you can choose.

Ok for the first one having both warnings. I don't see a reason to be different from Power in that
regard.

Thank you very much!

Andreas

Re: [PATCH] s390: Fix up -Wpsabi diagnostcs + analysis [PR94704]

[Ulrich Weigand]

On Tue, Apr 28, 2020 at 02:13:02PM +0200, Jakub Jelinek wrote:

> struct X { };
> struct Y { int : 0; };
> struct Z { int : 0; Y y; };
> struct U : public X { X q; };
> struct A { double a; };
> struct B : public X { double a; };
> struct C : public Y { double a; };
> struct D : public Z { double a; };
> struct E : public U { double a; };

This is only testing the [[no_unique_address]] attribute in the
most-derived class, but I believe the attribute also affects
members in the base class.  Is this understanding correct?

Specifically, if we were to add two more tests to the above:
struct X2 { X x; };
struct X3 { [[no_unique_address]] X x; };
struct B2 : public X2 { double a; };
struct B3 : public X3 { double a; };
Then we should see that B2 does *not* count as single-element
struct, but B3 *does*.  (That's also what GCC currently does.)

Just trying to get clarity here as I'm about to work on this
for clang ...

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  Ulrich.Weigand@de.ibm.com

Re: [PATCH] s390: Fix up -Wpsabi diagnostcs + analysis [PR94704]

[Jason Merrill]

On 5/19/20 9:34 AM, Ulrich Weigand wrote:
> On Tue, Apr 28, 2020 at 02:13:02PM +0200, Jakub Jelinek wrote:
> 
>> struct X { };
>> struct Y { int : 0; };
>> struct Z { int : 0; Y y; };
>> struct U : public X { X q; };
>> struct A { double a; };
>> struct B : public X { double a; };
>> struct C : public Y { double a; };
>> struct D : public Z { double a; };
>> struct E : public U { double a; };
> 
> This is only testing the [[no_unique_address]] attribute in the
> most-derived class, but I believe the attribute also affects
> members in the base class.  Is this understanding correct?
> 
> Specifically, if we were to add two more tests to the above:
> struct X2 { X x; };
> struct X3 { [[no_unique_address]] X x; };
> struct B2 : public X2 { double a; };
> struct B3 : public X3 { double a; };
> Then we should see that B2 does *not* count as single-element
> struct, but B3 *does*.  (That's also what GCC currently does.)
> 
> Just trying to get clarity here as I'm about to work on this
> for clang ...

That sounds right to me, given that X3 counts as an empty class under

https://itanium-cxx-abi.github.io/cxx-abi/abi.html#definitions

empty class
A class with no non-static data members other than empty data members, 
no unnamed bit-fields other than zero-width bit-fields, no virtual 
functions, no virtual base classes, and no non-empty non-virtual proper 
base classes.

Jason