Re: [PATCH 1/2] arm: correctly handle zero-sized bit-fields in AAPCS [PR102024]

[Richard Earnshaw <Richard.Earnshaw@foss.arm.com>]

On 29/03/2022 17:32, Jakub Jelinek via Gcc-patches wrote:
> On Tue, Mar 29, 2022 at 04:32:10PM +0100, Richard Earnshaw wrote:
>>
>> On arm the AAPCS states that an HFA is determined by the 'shape' of
>> the object after layout has been completed, so anything that adds no
>> members and does not cause the layout to be modified should be ignored
>> for the purposes of determining which registers are used for parameter
>> passing.
>>
>> A zero-sized bit-field falls into this category.  This was not handled
>> correctly for C structs and in G++-11 only handled correctly because
>> such fields were eliminated early by the front end.
>>
>> gcc/ChangeLog:
>>
>> 	PR target/102024
>> 	* config/arm/arm.cc (aapcs_vfp_sub_candidate): Handle zero-sized
>> 	bit-fields.  Detect cases where a warning may be needed.
>> 	(aapcs_vfp_is_call_or_return_candidate): Emit a note if
>> 	a zero-sized bit-field has caused parameter passing to change.
>>
>> gcc/testsuite/ChangeLog:
>>
>> 	PR target/102024
>> 	* gcc.target/arm/aapcs/vfp26.c: New test.
>> ---
>>   gcc/config/arm/arm.cc                      | 35 ++++++++++++++++++++--
>>   gcc/testsuite/gcc.target/arm/aapcs/vfp26.c | 31 +++++++++++++++++++
>>   2 files changed, 63 insertions(+), 3 deletions(-)
>>   create mode 100644 gcc/testsuite/gcc.target/arm/aapcs/vfp26.c
>>
> 
>> diff --git a/gcc/config/arm/arm.cc b/gcc/config/arm/arm.cc
>> index e062361b985..43b775f6918 100644
>> --- a/gcc/config/arm/arm.cc
>> +++ b/gcc/config/arm/arm.cc
>> @@ -6274,6 +6274,7 @@ aapcs_vfp_cum_init (CUMULATIVE_ARGS *pcum  ATTRIBUTE_UNUSED,
>>         a HFA or HVA.  */
>>   const unsigned int WARN_PSABI_EMPTY_CXX17_BASE = 1U << 0;
>>   const unsigned int WARN_PSABI_NO_UNIQUE_ADDRESS = 1U << 1;
>> +const unsigned int WARN_PSABI_ZERO_WIDTH_BITFIELD = 1U << 2;
>>   
>>   /* Walk down the type tree of TYPE counting consecutive base elements.
>>      If *MODEP is VOIDmode, then set it to the first valid floating point
>> @@ -6426,6 +6427,28 @@ aapcs_vfp_sub_candidate (const_tree type, machine_mode *modep,
>>   		    continue;
>>   		  }
>>   	      }
>> +	    /* A zero-width bitfield may affect layout in some
>> +	       circumstances, but adds no members.  The determination
>> +	       of whether or not a type is an HFA is performed after
>> +	       layout is complete, so if the type still looks like an
>> +	       HFA afterwards, it is still classed as one.  This is
>> +	       potentially an ABI break for the hard-float ABI.  */
>> +	    else if (DECL_BIT_FIELD (field)
>> +		     && integer_zerop (DECL_SIZE (field)))
>> +	      {
>> +		/* Prior to GCC-12 these fields were striped early,
>> +		   hiding them from the back-end entirely and
>> +		   resulting in the correct behaviour for argument
>> +		   passing.  Simulate that old behaviour without
>> +		   generating a warning.  */
>> +		if (DECL_FIELD_CXX_ZERO_WIDTH_BIT_FIELD (field))
>> +		  continue;
>> +		if (warn_psabi_flags)
>> +		  {
>> +		    *warn_psabi_flags |= WARN_PSABI_ZERO_WIDTH_BITFIELD;
>> +		    continue;
>> +		  }
>> +	      }
>>   
>>   	    sub_count = aapcs_vfp_sub_candidate (TREE_TYPE (field), modep,
>>   						 warn_psabi_flags);
>> @@ -6538,8 +6561,10 @@ aapcs_vfp_is_call_or_return_candidate (enum arm_pcs pcs_variant,
>>   	      && ((alt = aapcs_vfp_sub_candidate (type, &new_mode, NULL))
>>   		  != ag_count))
>>   	    {
>> -	      const char *url
>> +	      const char *url10
>>   		= CHANGES_ROOT_URL "gcc-10/changes.html#empty_base";
>> +	      const char *url12
>> +		= CHANGES_ROOT_URL "gcc-12/changes.html#empty_base";
> 
> This should be
> 		= CHANGES_ROOT_URL "gcc-12/changes.html#zero_width_bitfields";
> instead.
> 
> Otherwise LGTM.
> 
> 	Jakub
> 

Good catch. Thanks.  Updated and pushed both patches.

R.