Message ID | 20220329153211.110702-1-rearnsha@arm.com |
---|---|
State | New |
Headers | show |
Series | [1/2] arm: correctly handle zero-sized bit-fields in AAPCS [PR102024] | expand |
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
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.
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"; gcc_assert (alt == -1); last_reported_type_uid = uid; /* Use TYPE_MAIN_VARIANT to strip any redundant const @@ -6548,12 +6573,16 @@ aapcs_vfp_is_call_or_return_candidate (enum arm_pcs pcs_variant, inform (input_location, "parameter passing for argument of " "type %qT with %<[[no_unique_address]]%> members " "changed %{in GCC 10.1%}", - TYPE_MAIN_VARIANT (type), url); + TYPE_MAIN_VARIANT (type), url10); else if (warn_psabi_flags & WARN_PSABI_EMPTY_CXX17_BASE) 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_MAIN_VARIANT (type), url); + TYPE_MAIN_VARIANT (type), url10); + else if (warn_psabi_flags & WARN_PSABI_ZERO_WIDTH_BITFIELD) + inform (input_location, "parameter passing for argument of " + "type %qT changed %{in GCC 12.1%}", + TYPE_MAIN_VARIANT (type), url12); } *count = ag_count; } diff --git a/gcc/testsuite/gcc.target/arm/aapcs/vfp26.c b/gcc/testsuite/gcc.target/arm/aapcs/vfp26.c new file mode 100644 index 00000000000..9b1e8aa39d6 --- /dev/null +++ b/gcc/testsuite/gcc.target/arm/aapcs/vfp26.c @@ -0,0 +1,31 @@ +/* Test AAPCS layout (VFP variant) */ + +/* { dg-do run { target arm_eabi } } */ +/* { dg-require-effective-target arm_hard_vfp_ok } */ +/* { dg-require-effective-target arm32 } */ +/* { dg-options "-O -mfpu=vfp -mfloat-abi=hard" } */ + +#ifndef IN_FRAMEWORK +#define VFP +#define TESTFILE "vfp26.c" + +/* Anonymous bitfields do not add members; if they do not change the layout + then the end result may still be an HFA. */ +struct z +{ + float a; + int :0; + float b; +}; + +struct z a = { 5.0f, 6.0f }; +struct z b = { 9.0f, 10.0f }; + +#define MYFUNCTYPE struct z + +#include "abitest.h" +#else + ARG(int, 7, R0) + ARG(struct z, a, S0) + LAST_ARG(struct z, b, S2) +#endif