rs6000: Add psabi diagnostic for C++ zero-width bit field ABI change (PR102024)

  Hi!

Previously zero-width bit fields were removed from structs, so that otherwise
homogeneous aggregates were treated as such and passed in FPRs and VSRs.
This was incorrect behavior per the ELFv2 ABI.  Now that these fields are no
longer being removed, we generate the correct parameter passing code.  Alert
the unwary user in the rare cases where this behavior changes.

As noted in the PR, once the GCC 12 Changes page has text describing this issue,
we can update the diagnostic message to reference that URL.  I'll handle that
in a follow-up patch.

Bootstrapped and tested on powerpc64le-unknown-linux-gnu with no regressions.
Is this okay for trunk?

Thanks!
Bill


2021-09-21  Bill Schmidt  <wschmidt@linux.ibm.com>

gcc/
	PR target/102024
	* config/rs6000/rs6000-call.c (rs6000_aggregate_candidate): Detect
	zero-width bit fields and return indicator.
	(rs6000_discover_homogeneous_aggregate): Diagnose when the
	presence of a zero-width bit field changes parameter passing in
	GCC 12.

gcc/testsuite/
	PR target/102024
	* g++.target/powerpc/pr102024.C: New.
---
  gcc/config/rs6000/rs6000-call.c             | 39 ++++++++++++++++++---
  gcc/testsuite/g++.target/powerpc/pr102024.C | 23 ++++++++++++
  2 files changed, 57 insertions(+), 5 deletions(-)
  create mode 100644 gcc/testsuite/g++.target/powerpc/pr102024.C
  

On Tue, 2021-09-21 at 17:35 -0500, Bill Schmidt wrote:
> Hi!
> 
> Previously zero-width bit fields were removed from structs, so that otherwise
> homogeneous aggregates were treated as such and passed in FPRs and VSRs.
> This was incorrect behavior per the ELFv2 ABI.  Now that these fields are no
> longer being removed, we generate the correct parameter passing code.  Alert
> the unwary user in the rare cases where this behavior changes.
> 
> As noted in the PR, once the GCC 12 Changes page has text describing this issue,
> we can update the diagnostic message to reference that URL.  I'll handle that
> in a follow-up patch.
> 
> Bootstrapped and tested on powerpc64le-unknown-linux-gnu with no regressions.
> Is this okay for trunk?

How previously?  is this one that will need all the backports? 

> 
> Thanks!
> Bill
> 
> 
> 2021-09-21  Bill Schmidt  <wschmidt@linux.ibm.com>
> 
> gcc/
> 	PR target/102024
> 	* config/rs6000/rs6000-call.c (rs6000_aggregate_candidate): Detect
> 	zero-width bit fields and return indicator.
> 	(rs6000_discover_homogeneous_aggregate): Diagnose when the
> 	presence of a zero-width bit field changes parameter passing in
> 	GCC 12.
> 
> gcc/testsuite/
> 	PR target/102024
> 	* g++.target/powerpc/pr102024.C: New.


ok

> ---
>  gcc/config/rs6000/rs6000-call.c             | 39 ++++++++++++++++++---
>  gcc/testsuite/g++.target/powerpc/pr102024.C | 23 ++++++++++++
>  2 files changed, 57 insertions(+), 5 deletions(-)
>  create mode 100644 gcc/testsuite/g++.target/powerpc/pr102024.C
> 
> diff --git a/gcc/config/rs6000/rs6000-call.c b/gcc/config/rs6000/rs6000-call.c
> index 7d485480225..c02b202b0cd 100644
> --- a/gcc/config/rs6000/rs6000-call.c
> +++ b/gcc/config/rs6000/rs6000-call.c
> @@ -6227,7 +6227,7 @@ const struct altivec_builtin_types altivec_overloaded_builtins[] = {
>  
>  static int
>  rs6000_aggregate_candidate (const_tree type, machine_mode *modep,
> -			    int *empty_base_seen)
> +			    int *empty_base_seen, int *zero_width_bf_seen)
>  {
>    machine_mode mode;
>    HOST_WIDE_INT size;
> @@ -6298,7 +6298,8 @@ rs6000_aggregate_candidate (const_tree type, machine_mode *modep,
>  	  return -1;
>  
>  	count = rs6000_aggregate_candidate (TREE_TYPE (type), modep,
> -					    empty_base_seen);
> +					    empty_base_seen,
> +					    zero_width_bf_seen);
>  	if (count == -1
>  	    || !index
>  	    || !TYPE_MAX_VALUE (index)
> @@ -6336,6 +6337,12 @@ rs6000_aggregate_candidate (const_tree type, machine_mode *modep,
>  	    if (TREE_CODE (field) != FIELD_DECL)
>  	      continue;
>  
> +	    if (DECL_FIELD_CXX_ZERO_WIDTH_BIT_FIELD (field))
> +	      {
> +		*zero_width_bf_seen = 1;
> +		continue;
> +	      }
> +

Noting that the definition comes from tree.h and is 
#define SET_DECL_FIELD_CXX_ZERO_WIDTH_BIT_FIELD(NODE, VAL) \
  do {								\
    gcc_checking_assert (DECL_BIT_FIELD (NODE));		\
    FIELD_DECL_CHECK (NODE)->decl_common.decl_flag_0 = (VAL);	\
  } while (0)

ok.



>  	    if (DECL_FIELD_ABI_IGNORED (field))
>  	      {
>  		if (lookup_attribute ("no_unique_address",
> @@ -6347,7 +6354,8 @@ rs6000_aggregate_candidate (const_tree type, machine_mode *modep,
>  	      }
>  
>  	    sub_count = rs6000_aggregate_candidate (TREE_TYPE (field), modep,
> -						    empty_base_seen);
> +						    empty_base_seen,
> +						    zero_width_bf_seen);
>  	    if (sub_count < 0)
>  	      return -1;
>  	    count += sub_count;
> @@ -6381,7 +6389,8 @@ rs6000_aggregate_candidate (const_tree type, machine_mode *modep,
>  	      continue;
>  
>  	    sub_count = rs6000_aggregate_candidate (TREE_TYPE (field), modep,
> -						    empty_base_seen);
> +						    empty_base_seen,
> +						    zero_width_bf_seen);
>  	    if (sub_count < 0)
>  	      return -1;
>  	    count = count > sub_count ? count : sub_count;
> @@ -6423,8 +6432,10 @@ rs6000_discover_homogeneous_aggregate (machine_mode mode, const_tree type,
>      {
>        machine_mode field_mode = VOIDmode;
>        int empty_base_seen = 0;
> +      int zero_width_bf_seen = 0;
>        int field_count = rs6000_aggregate_candidate (type, &field_mode,
> -						    &empty_base_seen);
> +						    &empty_base_seen,
> +						    &zero_width_bf_seen);
>  

That appears to be all of the callers of rs6000_aggregate_candidate. 
(ok).

>        if (field_count > 0)
>  	{
> @@ -6460,6 +6471,24 @@ rs6000_discover_homogeneous_aggregate (machine_mode mode, const_tree type,
>  		      last_reported_type_uid = uid;
>  		    }
>  		}
> +	      if (zero_width_bf_seen && warn_psabi)
> +		{
> +		  static unsigned last_reported_type_uid;
> +		  unsigned uid = TYPE_UID (TYPE_MAIN_VARIANT (type));
> +		  if (uid != last_reported_type_uid)
> +		    {
> +		      inform (input_location,
> +			      "parameter passing for an argument containing "
> +			      "zero-width bit fields but that is otherwise a "
> +			      "homogeneous aggregate changed in GCC 12.1");
> +		      last_reported_type_uid = uid;
> +		    }
> +		  if (elt_mode)
> +		    *elt_mode = mode;
> +		  if (n_elts)
> +		    *n_elts = 1;
> +		  return false;
> +		}

In comparison to the other nearby warn_psabi logic, this seems
reasonable.  (ok)


>  	      return true;
>  	    }
>  	}
> diff --git a/gcc/testsuite/g++.target/powerpc/pr102024.C b/gcc/testsuite/g++.target/powerpc/pr102024.C
> new file mode 100644
> index 00000000000..29c9678acfd
> --- /dev/null
> +++ b/gcc/testsuite/g++.target/powerpc/pr102024.C
> @@ -0,0 +1,23 @@
> +// PR target/102024
> +// { dg-do compile { target powerpc_elfv2 } }
> +// { dg-options "-O2" }
> +
> +// Test that a zero-width bit field in an otherwise homogeneous aggregate
> +// generates a psabi warning and passes arguments in GPRs.
> +
> +// { dg-final { scan-assembler-times {\mstd\M} 4 } }
> +
> +struct a_thing
> +{
> +  double x;
> +  double y;
> +  double z;
> +  int : 0;
> +  double w;
> +};
> +
> +double
> +foo (a_thing a) // { dg-message "parameter passing for an argument containing zero-width bit fields but that is otherwise a homogeneous aggregate changed in GCC 12.1" }
> +{
> +  return a.x * a.y + a.z - a.w;
> +}

lgtm
thanks
-Will
  

On 9/22/21 9:35 AM, will schmidt wrote:
> On Tue, 2021-09-21 at 17:35 -0500, Bill Schmidt wrote:
>> Hi!
>>
>> Previously zero-width bit fields were removed from structs, so that otherwise
>> homogeneous aggregates were treated as such and passed in FPRs and VSRs.
>> This was incorrect behavior per the ELFv2 ABI.  Now that these fields are no
>> longer being removed, we generate the correct parameter passing code.  Alert
>> the unwary user in the rare cases where this behavior changes.
>>
>> As noted in the PR, once the GCC 12 Changes page has text describing this issue,
>> we can update the diagnostic message to reference that URL.  I'll handle that
>> in a follow-up patch.
>>
>> Bootstrapped and tested on powerpc64le-unknown-linux-gnu with no regressions.
>> Is this okay for trunk?
> How previously?  is this one that will need all the backports?

No, the change happened recently on trunk.

Thanks very much for the review!
Bill
>
>> Thanks!
>> Bill
>>
>>
>> 2021-09-21  Bill Schmidt  <wschmidt@linux.ibm.com>
>>
>> gcc/
>> 	PR target/102024
>> 	* config/rs6000/rs6000-call.c (rs6000_aggregate_candidate): Detect
>> 	zero-width bit fields and return indicator.
>> 	(rs6000_discover_homogeneous_aggregate): Diagnose when the
>> 	presence of a zero-width bit field changes parameter passing in
>> 	GCC 12.
>>
>> gcc/testsuite/
>> 	PR target/102024
>> 	* g++.target/powerpc/pr102024.C: New.
>
> ok
>
>> ---
>>   gcc/config/rs6000/rs6000-call.c             | 39 ++++++++++++++++++---
>>   gcc/testsuite/g++.target/powerpc/pr102024.C | 23 ++++++++++++
>>   2 files changed, 57 insertions(+), 5 deletions(-)
>>   create mode 100644 gcc/testsuite/g++.target/powerpc/pr102024.C
>>
>> diff --git a/gcc/config/rs6000/rs6000-call.c b/gcc/config/rs6000/rs6000-call.c
>> index 7d485480225..c02b202b0cd 100644
>> --- a/gcc/config/rs6000/rs6000-call.c
>> +++ b/gcc/config/rs6000/rs6000-call.c
>> @@ -6227,7 +6227,7 @@ const struct altivec_builtin_types altivec_overloaded_builtins[] = {
>>   
>>   static int
>>   rs6000_aggregate_candidate (const_tree type, machine_mode *modep,
>> -			    int *empty_base_seen)
>> +			    int *empty_base_seen, int *zero_width_bf_seen)
>>   {
>>     machine_mode mode;
>>     HOST_WIDE_INT size;
>> @@ -6298,7 +6298,8 @@ rs6000_aggregate_candidate (const_tree type, machine_mode *modep,
>>   	  return -1;
>>   
>>   	count = rs6000_aggregate_candidate (TREE_TYPE (type), modep,
>> -					    empty_base_seen);
>> +					    empty_base_seen,
>> +					    zero_width_bf_seen);
>>   	if (count == -1
>>   	    || !index
>>   	    || !TYPE_MAX_VALUE (index)
>> @@ -6336,6 +6337,12 @@ rs6000_aggregate_candidate (const_tree type, machine_mode *modep,
>>   	    if (TREE_CODE (field) != FIELD_DECL)
>>   	      continue;
>>   
>> +	    if (DECL_FIELD_CXX_ZERO_WIDTH_BIT_FIELD (field))
>> +	      {
>> +		*zero_width_bf_seen = 1;
>> +		continue;
>> +	      }
>> +
> Noting that the definition comes from tree.h and is
> #define SET_DECL_FIELD_CXX_ZERO_WIDTH_BIT_FIELD(NODE, VAL) \
>    do {								\
>      gcc_checking_assert (DECL_BIT_FIELD (NODE));		\
>      FIELD_DECL_CHECK (NODE)->decl_common.decl_flag_0 = (VAL);	\
>    } while (0)
>
> ok.
>
>
>
>>   	    if (DECL_FIELD_ABI_IGNORED (field))
>>   	      {
>>   		if (lookup_attribute ("no_unique_address",
>> @@ -6347,7 +6354,8 @@ rs6000_aggregate_candidate (const_tree type, machine_mode *modep,
>>   	      }
>>   
>>   	    sub_count = rs6000_aggregate_candidate (TREE_TYPE (field), modep,
>> -						    empty_base_seen);
>> +						    empty_base_seen,
>> +						    zero_width_bf_seen);
>>   	    if (sub_count < 0)
>>   	      return -1;
>>   	    count += sub_count;
>> @@ -6381,7 +6389,8 @@ rs6000_aggregate_candidate (const_tree type, machine_mode *modep,
>>   	      continue;
>>   
>>   	    sub_count = rs6000_aggregate_candidate (TREE_TYPE (field), modep,
>> -						    empty_base_seen);
>> +						    empty_base_seen,
>> +						    zero_width_bf_seen);
>>   	    if (sub_count < 0)
>>   	      return -1;
>>   	    count = count > sub_count ? count : sub_count;
>> @@ -6423,8 +6432,10 @@ rs6000_discover_homogeneous_aggregate (machine_mode mode, const_tree type,
>>       {
>>         machine_mode field_mode = VOIDmode;
>>         int empty_base_seen = 0;
>> +      int zero_width_bf_seen = 0;
>>         int field_count = rs6000_aggregate_candidate (type, &field_mode,
>> -						    &empty_base_seen);
>> +						    &empty_base_seen,
>> +						    &zero_width_bf_seen);
>>   
> That appears to be all of the callers of rs6000_aggregate_candidate.
> (ok).
>
>>         if (field_count > 0)
>>   	{
>> @@ -6460,6 +6471,24 @@ rs6000_discover_homogeneous_aggregate (machine_mode mode, const_tree type,
>>   		      last_reported_type_uid = uid;
>>   		    }
>>   		}
>> +	      if (zero_width_bf_seen && warn_psabi)
>> +		{
>> +		  static unsigned last_reported_type_uid;
>> +		  unsigned uid = TYPE_UID (TYPE_MAIN_VARIANT (type));
>> +		  if (uid != last_reported_type_uid)
>> +		    {
>> +		      inform (input_location,
>> +			      "parameter passing for an argument containing "
>> +			      "zero-width bit fields but that is otherwise a "
>> +			      "homogeneous aggregate changed in GCC 12.1");
>> +		      last_reported_type_uid = uid;
>> +		    }
>> +		  if (elt_mode)
>> +		    *elt_mode = mode;
>> +		  if (n_elts)
>> +		    *n_elts = 1;
>> +		  return false;
>> +		}
> In comparison to the other nearby warn_psabi logic, this seems
> reasonable.  (ok)
>
>
>>   	      return true;
>>   	    }
>>   	}
>> diff --git a/gcc/testsuite/g++.target/powerpc/pr102024.C b/gcc/testsuite/g++.target/powerpc/pr102024.C
>> new file mode 100644
>> index 00000000000..29c9678acfd
>> --- /dev/null
>> +++ b/gcc/testsuite/g++.target/powerpc/pr102024.C
>> @@ -0,0 +1,23 @@
>> +// PR target/102024
>> +// { dg-do compile { target powerpc_elfv2 } }
>> +// { dg-options "-O2" }
>> +
>> +// Test that a zero-width bit field in an otherwise homogeneous aggregate
>> +// generates a psabi warning and passes arguments in GPRs.
>> +
>> +// { dg-final { scan-assembler-times {\mstd\M} 4 } }
>> +
>> +struct a_thing
>> +{
>> +  double x;
>> +  double y;
>> +  double z;
>> +  int : 0;
>> +  double w;
>> +};
>> +
>> +double
>> +foo (a_thing a) // { dg-message "parameter passing for an argument containing zero-width bit fields but that is otherwise a homogeneous aggregate changed in GCC 12.1" }
>> +{
>> +  return a.x * a.y + a.z - a.w;
>> +}
> lgtm
> thanks
> -Will
>
>
>
  

On Wed, Sep 22, 2021 at 09:43:23AM -0500, Bill Schmidt wrote:
> > How previously?  is this one that will need all the backports?
> 
> No, the change happened recently on trunk.

It is actually more complex.
Both C and C++ FEs thought they were removing zero bit fields, but
neither did that, then the non-working code from C FE has been removed,
and finally in 4.5 the C++ FE has been "fixed" to remove zero bit fields
"correctly".  And 12 is going to remove the removal, but marks FIELD_DECLs
that were in 4.5-11 removed and now aren't for -Wpsabi purposes.
So, for most backends, C and C++ was ABI compatible in presence of :0
initially, then for several got incompatible in 4.5 and now is time to
decide for each backend what to do according to their psABI, if :0 should be
ignored or not during the function arg/return value passing decisions.

> > > --- a/gcc/config/rs6000/rs6000-call.c
> > > +++ b/gcc/config/rs6000/rs6000-call.c
> > > @@ -6227,7 +6227,7 @@ const struct altivec_builtin_types altivec_overloaded_builtins[] = {
> > >   static int
> > >   rs6000_aggregate_candidate (const_tree type, machine_mode *modep,
> > > -			    int *empty_base_seen)
> > > +			    int *empty_base_seen, int *zero_width_bf_seen)
> > >   {
> > >     machine_mode mode;
> > >     HOST_WIDE_INT size;
> > > @@ -6298,7 +6298,8 @@ rs6000_aggregate_candidate (const_tree type, machine_mode *modep,
> > >   	  return -1;
> > >   	count = rs6000_aggregate_candidate (TREE_TYPE (type), modep,
> > > -					    empty_base_seen);
> > > +					    empty_base_seen,
> > > +					    zero_width_bf_seen);
> > >   	if (count == -1
> > >   	    || !index
> > >   	    || !TYPE_MAX_VALUE (index)
> > > @@ -6336,6 +6337,12 @@ rs6000_aggregate_candidate (const_tree type, machine_mode *modep,
> > >   	    if (TREE_CODE (field) != FIELD_DECL)
> > >   	      continue;
> > > +	    if (DECL_FIELD_CXX_ZERO_WIDTH_BIT_FIELD (field))
> > > +	      {
> > > +		*zero_width_bf_seen = 1;
> > > +		continue;
> > > +	      }

So, from what you wrote, :0 in the ppc* psABIs the intent is that :0 is not
ignored, right?
In that case I don't really understand the above (the continue in
particular).  Because the continue means it is ignored for C++ and not
ignored for C, so basically you return to the 4.5-11 ABI incompatibility
between C and C++.
C++ :0 will have DECL_FIELD_CXX_ZERO_WIDTH_BIT_FIELD set, C :0 will not...

	Jakub
  

On Wed, Sep 22, 2021 at 05:02:15PM +0200, Jakub Jelinek via Gcc-patches wrote:
> > > > @@ -6298,7 +6298,8 @@ rs6000_aggregate_candidate (const_tree type, machine_mode *modep,
> > > >   	  return -1;
> > > >   	count = rs6000_aggregate_candidate (TREE_TYPE (type), modep,
> > > > -					    empty_base_seen);
> > > > +					    empty_base_seen,
> > > > +					    zero_width_bf_seen);
> > > >   	if (count == -1
> > > >   	    || !index
> > > >   	    || !TYPE_MAX_VALUE (index)
> > > > @@ -6336,6 +6337,12 @@ rs6000_aggregate_candidate (const_tree type, machine_mode *modep,
> > > >   	    if (TREE_CODE (field) != FIELD_DECL)
> > > >   	      continue;
> > > > +	    if (DECL_FIELD_CXX_ZERO_WIDTH_BIT_FIELD (field))
> > > > +	      {
> > > > +		*zero_width_bf_seen = 1;
> > > > +		continue;
> > > > +	      }
> 
> So, from what you wrote, :0 in the ppc* psABIs the intent is that :0 is not
> ignored, right?
> In that case I don't really understand the above (the continue in
> particular).  Because the continue means it is ignored for C++ and not
> ignored for C, so basically you return to the 4.5-11 ABI incompatibility
> between C and C++.
> C++ :0 will have DECL_FIELD_CXX_ZERO_WIDTH_BIT_FIELD set, C :0 will not...

To be more precise, I'd expect what most targets want to do for the
actual ABI decisions not to use DECL_FIELD_CXX_ZERO_WIDTH_BIT_FIELD at all.
I.e. do:
  if (TREE_CODE (field) != FIELD_DECL)
    continue;
  if (DECL_BIT_FIELD (field) && integer_zerop (DECL_SIZE (field)))
    {
      // :0
      // in some psABIs, ignore it, i.e. continue;
      // in others psABIs, take them into account, i.e. do nothing.
    }
and use DECL_FIELD_CXX_ZERO_WIDTH_BIT_FIELD only for the -Wpsabi purposes.

The only exception would be for targets that decide to keep GCC 4.5-11
compatibility with the C incompatible with C++.

	Jakub
  

Hi Jakub,

On 9/22/21 11:33 AM, Jakub Jelinek wrote:
> On Wed, Sep 22, 2021 at 05:02:15PM +0200, Jakub Jelinek via Gcc-patches wrote:
>>>>> @@ -6298,7 +6298,8 @@ rs6000_aggregate_candidate (const_tree type, machine_mode *modep,
>>>>>    	  return -1;
>>>>>    	count = rs6000_aggregate_candidate (TREE_TYPE (type), modep,
>>>>> -					    empty_base_seen);
>>>>> +					    empty_base_seen,
>>>>> +					    zero_width_bf_seen);
>>>>>    	if (count == -1
>>>>>    	    || !index
>>>>>    	    || !TYPE_MAX_VALUE (index)
>>>>> @@ -6336,6 +6337,12 @@ rs6000_aggregate_candidate (const_tree type, machine_mode *modep,
>>>>>    	    if (TREE_CODE (field) != FIELD_DECL)
>>>>>    	      continue;
>>>>> +	    if (DECL_FIELD_CXX_ZERO_WIDTH_BIT_FIELD (field))
>>>>> +	      {
>>>>> +		*zero_width_bf_seen = 1;
>>>>> +		continue;
>>>>> +	      }
>> So, from what you wrote, :0 in the ppc* psABIs the intent is that :0 is not
>> ignored, right?
>> In that case I don't really understand the above (the continue in
>> particular).  Because the continue means it is ignored for C++ and not
>> ignored for C, so basically you return to the 4.5-11 ABI incompatibility
>> between C and C++.
>> C++ :0 will have DECL_FIELD_CXX_ZERO_WIDTH_BIT_FIELD set, C :0 will not...
> To be more precise, I'd expect what most targets want to do for the
> actual ABI decisions not to use DECL_FIELD_CXX_ZERO_WIDTH_BIT_FIELD at all.
> I.e. do:
>    if (TREE_CODE (field) != FIELD_DECL)
>      continue;
>    if (DECL_BIT_FIELD (field) && integer_zerop (DECL_SIZE (field)))
>      {
>        // :0
>        // in some psABIs, ignore it, i.e. continue;
>        // in others psABIs, take them into account, i.e. do nothing.
>      }
> and use DECL_FIELD_CXX_ZERO_WIDTH_BIT_FIELD only for the -Wpsabi purposes.
>
> The only exception would be for targets that decide to keep GCC 4.5-11
> compatibility with the C incompatible with C++.

I think you're misunderstanding what I'm trying to do with this patch.  
I am not changing the code generation at all (your patch did that).  All 
I'm doing is detecting when the old code generation and new code 
generation will differ, and emitting a diagnostic in that case.

The way I do that is to allow rs6000_aggregate_candidate to *think* that 
something is a homogeneous aggregate even when zero-width bitfields are 
present (hence the continue), but record the fact that we saw one.  This 
gives the same answer as we gave before your patch.  Then, in 
rs6000_discover_homogeneous_aggregate, once we think we have a 
homogeneous aggregate, we check whether we actually had a zero-width 
bitfield present.  If so, then we diagnose the change in code generation 
and return false, indicating that we didn't actually find a homogeneous 
aggregate.  Other than the diagnostic, this matches the behavior after 
your patch.

I've verified that we didn't change code generation for C code with 
zero-width bitfields as a result of either your patch or mine. Before 
and after, a C struct containing a zero-width bitfield causes us to 
avoid generating code for a homogeneous aggregate, just as we do for C++ 
after your patch.

I hope this helps clear things up, and I apologize for not giving a 
better description of my intent.

Thanks!
Bill
>
> 	Jakub
>
  

Hi!

On Tue, Sep 21, 2021 at 05:35:56PM -0500, Bill Schmidt wrote:
> Previously zero-width bit fields were removed from structs, so that 
> otherwise
> homogeneous aggregates were treated as such and passed in FPRs and VSRs.
> This was incorrect behavior per the ELFv2 ABI.  Now that these fields are no
> longer being removed, we generate the correct parameter passing code.  Alert
> the unwary user in the rare cases where this behavior changes.

> Bootstrapped and tested on powerpc64le-unknown-linux-gnu with no 
> regressions.
> Is this okay for trunk?

This can obviously not change anything for other ABIs, so it doesn't
need testing anywhere else.  Good :-)

> @@ -6227,7 +6227,7 @@ const struct altivec_builtin_types 
> altivec_overloaded_builtins[] = {
>  
>  static int
>  rs6000_aggregate_candidate (const_tree type, machine_mode *modep,
> -			    int *empty_base_seen)
> +			    int *empty_base_seen, int *zero_width_bf_seen)

The new function parameter should be described in the function comment.

> +	    if (DECL_FIELD_CXX_ZERO_WIDTH_BIT_FIELD (field))
> +	      {
> +		*zero_width_bf_seen = 1;
> +		continue;
> +	      }

Please add a comment here, saying what it is for?  I'd do it inside the
braces.

>  	    sub_count = rs6000_aggregate_candidate (TREE_TYPE (field), modep,
> -						    empty_base_seen);
> +						    empty_base_seen,
> +						    zero_width_bf_seen);

So it is important that this function only ever sets *zero_width_bf_seen
to 1, never resets it; please document that with it as well?

> +		      inform (input_location,
> +			      "parameter passing for an argument containing "
> +			      "zero-width bit fields but that is otherwise a 
> "
> +			      "homogeneous aggregate changed in GCC 12.1");

Just "GCC 12" please.  You might want to indicate that older compilers
did the wrong thing here.  And maybe say this is only for ELFv2 somehow?
In some way that doesn't make it more confusing than not saying it :-)

> +double
> +foo (a_thing a) // { dg-message "parameter passing for an argument 
> containing zero-width bit fields but that is otherwise a homogeneous 
> aggregate changed in GCC 12.1" }

I think you used "format=flawed" again?

Okay for trunk with such comment updates.  Thanks!


Segher
  

Hi Segher,

Thanks for the review!  This is what I committed.

2021-09-23  Bill Schmidt  <wschmidt@linux.ibm.com>

gcc/
	PR target/102024
	* config/rs6000/rs6000-call.c (rs6000_aggregate_candidate): Detect
	zero-width bit fields and return indicator.
	(rs6000_discover_homogeneous_aggregate): Diagnose when the
	presence of a zero-width bit field changes parameter passing in
	GCC 12.

gcc/testsuite/
	PR target/102024
	* g++.target/powerpc/pr102024.C: New.
---
 gcc/config/rs6000/rs6000-call.c             | 64 +++++++++++++++++++--
 gcc/testsuite/g++.target/powerpc/pr102024.C | 23 ++++++++
 2 files changed, 81 insertions(+), 6 deletions(-)
 create mode 100644 gcc/testsuite/g++.target/powerpc/pr102024.C

diff --git a/gcc/config/rs6000/rs6000-call.c b/gcc/config/rs6000/rs6000-call.c
index 7d485480225..2eceb2c71c0 100644
--- a/gcc/config/rs6000/rs6000-call.c
+++ b/gcc/config/rs6000/rs6000-call.c
@@ -6223,11 +6223,19 @@ const struct altivec_builtin_types altivec_overloaded_builtins[] = {
    or vector type.  If a non-floating point or vector type is found, or
    if a floating point or vector type that doesn't match a non-VOIDmode
    *MODEP is found, then return -1, otherwise return the count in the
-   sub-tree.  */
+   sub-tree.
+
+   There have been some ABI snafus along the way with C++.  Modify
+   EMPTY_BASE_SEEN to a nonzero value iff a C++ empty base class makes
+   an appearance; separate flag bits indicate whether or not such a
+   field is marked "no unique address".  Modify ZERO_WIDTH_BF_SEEN
+   to 1 iff a C++ zero-length bitfield makes an appearance, but
+   in this case otherwise treat this as still being a homogeneous
+   aggregate.  */
 
 static int
 rs6000_aggregate_candidate (const_tree type, machine_mode *modep,
-			    int *empty_base_seen)
+			    int *empty_base_seen, int *zero_width_bf_seen)
 {
   machine_mode mode;
   HOST_WIDE_INT size;
@@ -6298,7 +6306,8 @@ rs6000_aggregate_candidate (const_tree type, machine_mode *modep,
 	  return -1;
 
 	count = rs6000_aggregate_candidate (TREE_TYPE (type), modep,
-					    empty_base_seen);
+					    empty_base_seen,
+					    zero_width_bf_seen);
 	if (count == -1
 	    || !index
 	    || !TYPE_MAX_VALUE (index)
@@ -6336,6 +6345,26 @@ rs6000_aggregate_candidate (const_tree type, machine_mode *modep,
 	    if (TREE_CODE (field) != FIELD_DECL)
 	      continue;
 
+	    if (DECL_FIELD_CXX_ZERO_WIDTH_BIT_FIELD (field))
+	      {
+		/* GCC 11 and earlier generated incorrect code in a rare
+		   corner case for C++.  When a RECORD_TYPE looks like a
+		   homogeneous aggregate, except that it also contains
+		   one or more zero-width bit fields, these earlier
+		   compilers would incorrectly pass the fields in FPRs
+		   or VSRs.  This occurred because the front end wrongly
+		   removed these bitfields from the RECORD_TYPE.  In
+		   GCC 12 and later, the front end flaw was corrected.
+		   We want to diagnose this case.  To do this, we pretend
+		   that we don't see the zero-width bit fields (hence
+		   the continue statement here), but pass back a flag
+		   indicating what happened.  The caller then diagnoses
+		   the issue and rejects the RECORD_TYPE as a homogeneous
+		   aggregate.  */
+		*zero_width_bf_seen = 1;
+		continue;
+	      }
+
 	    if (DECL_FIELD_ABI_IGNORED (field))
 	      {
 		if (lookup_attribute ("no_unique_address",
@@ -6347,7 +6376,8 @@ rs6000_aggregate_candidate (const_tree type, machine_mode *modep,
 	      }
 
 	    sub_count = rs6000_aggregate_candidate (TREE_TYPE (field), modep,
-						    empty_base_seen);
+						    empty_base_seen,
+						    zero_width_bf_seen);
 	    if (sub_count < 0)
 	      return -1;
 	    count += sub_count;
@@ -6381,7 +6411,8 @@ rs6000_aggregate_candidate (const_tree type, machine_mode *modep,
 	      continue;
 
 	    sub_count = rs6000_aggregate_candidate (TREE_TYPE (field), modep,
-						    empty_base_seen);
+						    empty_base_seen,
+						    zero_width_bf_seen);
 	    if (sub_count < 0)
 	      return -1;
 	    count = count > sub_count ? count : sub_count;
@@ -6423,8 +6454,10 @@ rs6000_discover_homogeneous_aggregate (machine_mode mode, const_tree type,
     {
       machine_mode field_mode = VOIDmode;
       int empty_base_seen = 0;
+      int zero_width_bf_seen = 0;
       int field_count = rs6000_aggregate_candidate (type, &field_mode,
-						    &empty_base_seen);
+						    &empty_base_seen,
+						    &zero_width_bf_seen);
 
       if (field_count > 0)
 	{
@@ -6460,6 +6493,25 @@ rs6000_discover_homogeneous_aggregate (machine_mode mode, const_tree type,
 		      last_reported_type_uid = uid;
 		    }
 		}
+	      if (zero_width_bf_seen && warn_psabi)
+		{
+		  static unsigned last_reported_type_uid;
+		  unsigned uid = TYPE_UID (TYPE_MAIN_VARIANT (type));
+		  if (uid != last_reported_type_uid)
+		    {
+		      inform (input_location,
+			      "ELFv2 parameter passing for an argument "
+			      "containing zero-width bit fields but that is "
+			      "otherwise a homogeneous aggregate was "
+			      "corrected in GCC 12");
+		      last_reported_type_uid = uid;
+		    }
+		  if (elt_mode)
+		    *elt_mode = mode;
+		  if (n_elts)
+		    *n_elts = 1;
+		  return false;
+		}
 	      return true;
 	    }
 	}
diff --git a/gcc/testsuite/g++.target/powerpc/pr102024.C b/gcc/testsuite/g++.target/powerpc/pr102024.C
new file mode 100644
index 00000000000..769585052b5
--- /dev/null
+++ b/gcc/testsuite/g++.target/powerpc/pr102024.C
@@ -0,0 +1,23 @@
+// PR target/102024
+// { dg-do compile { target powerpc_elfv2 } }
+// { dg-options "-O2" }
+
+// Test that a zero-width bit field in an otherwise homogeneous aggregate
+// generates a psabi warning and passes arguments in GPRs.
+
+// { dg-final { scan-assembler-times {\mstd\M} 4 } }
+
+struct a_thing
+{
+  double x;
+  double y;
+  double z;
+  int : 0;
+  double w;
+};
+
+double
+foo (a_thing a) // { dg-message "ELFv2 parameter passing for an argument containing zero-width bit fields but that is otherwise a homogeneous aggregate was corrected in GCC 12" }
+{
+  return a.x * a.y + a.z - a.w;
+}