On Wed, Dec 15, 2021 at 3:50 PM Jakub Jelinek <ja...@redhat.com> wrote:
>
> On Mon, Nov 29, 2021 at 05:25:30AM -0700, H.J. Lu wrote:
> > > I'd like to ping this patch, but perhaps first it would be nice to discuss
> > > it in the x86-64 psABI group.
> > > The current psABI doesn't seem to mention zero sized bitfields at all
> > > explicitly, so perhaps theoretically they should be treated as INTEGER
> > > class,
> > > but if they are at positions multiple of 64 bits, then it is unclear into
> > > which eightbyte they should be considered, whether the previous one if any
> > > or the next one if any. I guess similar problem is for zero sized
> > > structures, but those should according to algorithm have NO_CLASS and so
> > > it
> > > doesn't really make a difference. And, no compiler I'm aware of treats
> > > the zero sized bitfields at 64 bit boundaries as INTEGER class, LLVM/ICC
> > > are
> > > ignoring such bitfields everywhere, GCC ignores them at those boundaries
> > > (and used to ignore them in C++ everywhere). I guess my preferred
> > > solution
> > > would be to say explicitly that zero sized bitfields are NO_CLASS.
> > > I'm not a member of the google x86-64 psABI group, can somebody please
> > > raise
> > > it there?
> >
> > https://groups.google.com/g/x86-64-abi/c/OYeWs14WHQ4
>
> Thanks.
> I see nobody commented on Micha's post there.
>
> Here is a patch that implements it in GCC, i.e. C++ doesn't change ABI (at
> least
> not from the past few releases) and C does for GCC:
>
> 2021-12-15 Jakub Jelinek <ja...@redhat.com>
>
> PR target/102024
> * config/i386/i386.c (classify_argument): Add zero_width_bitfields
> argument, when seeing DECL_FIELD_CXX_ZERO_WIDTH_BIT_FIELD bitfields,
> always ignore them, when seeing other zero sized bitfields, either
> set zero_width_bitfields to 1 and ignore it or if equal to 2 process
> it. Pass it to recursive calls. Add wrapper
> with old arguments and diagnose ABI differences for C structures
> with zero width bitfields. Formatting fixes.
>
> * gcc.target/i386/pr102024.c: New test.
> * g++.target/i386/pr102024.C: New test.
Please get a signoff on the ABI change (perhaps HJ can help here),
I'll approve the implementation after that.
Uros.
>
> --- gcc/config/i386/i386.c.jj 2021-12-10 17:00:06.024369219 +0100
> +++ gcc/config/i386/i386.c 2021-12-15 15:04:49.245148023 +0100
> @@ -2065,7 +2065,8 @@ merge_classes (enum x86_64_reg_class cla
>
> static int
> classify_argument (machine_mode mode, const_tree type,
> - enum x86_64_reg_class classes[MAX_CLASSES], int bit_offset)
> + enum x86_64_reg_class classes[MAX_CLASSES], int bit_offset,
> + int &zero_width_bitfields)
> {
> HOST_WIDE_INT bytes
> = mode == BLKmode ? int_size_in_bytes (type) : (int) GET_MODE_SIZE
> (mode);
> @@ -2123,6 +2124,16 @@ classify_argument (machine_mode mode, co
> misaligned integers. */
> if (DECL_BIT_FIELD (field))
> {
> + if (integer_zerop (DECL_SIZE (field)))
> + {
> + if (DECL_FIELD_CXX_ZERO_WIDTH_BIT_FIELD (field))
> + continue;
> + if (zero_width_bitfields != 2)
> + {
> + zero_width_bitfields = 1;
> + continue;
> + }
> + }
> for (i = (int_bit_position (field)
> + (bit_offset % 64)) / 8 / 8;
> i < ((int_bit_position (field) + (bit_offset % 64))
> @@ -2160,7 +2171,8 @@ classify_argument (machine_mode mode, co
> num = classify_argument (TYPE_MODE (type), type,
> subclasses,
> (int_bit_position (field)
> - + bit_offset) % 512);
> + + bit_offset) % 512,
> + zero_width_bitfields);
> if (!num)
> return 0;
> pos = (int_bit_position (field)
> @@ -2178,7 +2190,8 @@ classify_argument (machine_mode mode, co
> {
> int num;
> num = classify_argument (TYPE_MODE (TREE_TYPE (type)),
> - TREE_TYPE (type), subclasses,
> bit_offset);
> + TREE_TYPE (type), subclasses, bit_offset,
> + zero_width_bitfields);
> if (!num)
> return 0;
>
> @@ -2211,7 +2224,7 @@ classify_argument (machine_mode mode, co
>
> num = classify_argument (TYPE_MODE (TREE_TYPE (field)),
> TREE_TYPE (field), subclasses,
> - bit_offset);
> + bit_offset, zero_width_bitfields);
> if (!num)
> return 0;
> for (i = 0; i < num && i < words; i++)
> @@ -2231,7 +2244,7 @@ classify_argument (machine_mode mode, co
> X86_64_SSEUP_CLASS, everything should be passed in
> memory. */
> if (classes[0] != X86_64_SSE_CLASS)
> - return 0;
> + return 0;
>
> for (i = 1; i < words; i++)
> if (classes[i] != X86_64_SSEUP_CLASS)
> @@ -2257,8 +2270,8 @@ classify_argument (machine_mode mode, co
> classes[i] = X86_64_SSE_CLASS;
> }
>
> - /* If X86_64_X87UP_CLASS isn't preceded by X86_64_X87_CLASS,
> - everything should be passed in memory. */
> + /* If X86_64_X87UP_CLASS isn't preceded by X86_64_X87_CLASS,
> + everything should be passed in memory. */
> if (classes[i] == X86_64_X87UP_CLASS
> && (classes[i - 1] != X86_64_X87_CLASS))
> {
> @@ -2487,6 +2500,44 @@ classify_argument (machine_mode mode, co
> }
> }
>
> +/* Wrapper around classify_argument with the extra zero_width_bitfields
> + argument, to diagnose GCC 12.1 ABI differences for C. */
> +
> +static int
> +classify_argument (machine_mode mode, const_tree type,
> + enum x86_64_reg_class classes[MAX_CLASSES], int bit_offset)
> +{
> + int zero_width_bitfields = 0;
> + static bool warned = false;
> + int n = classify_argument (mode, type, classes, bit_offset,
> + zero_width_bitfields);
> + if (!zero_width_bitfields || warned || !warn_psabi)
> + return n;
> + enum x86_64_reg_class alt_classes[MAX_CLASSES];
> + zero_width_bitfields = 2;
> + if (classify_argument (mode, type, alt_classes, bit_offset,
> + zero_width_bitfields) != n)
> + zero_width_bitfields = 3;
> + else
> + for (int i = 0; i < n; i++)
> + if (classes[i] != alt_classes[i])
> + {
> + zero_width_bitfields = 3;
> + break;
> + }
> + if (zero_width_bitfields == 3)
> + {
> + warned = true;
> + const char *url
> + = CHANGES_ROOT_URL "gcc-12/changes.html#zero_width_bitfields";
> +
> + inform (input_location,
> + "the ABI of passing C structures with zero-width bit-fields"
> + " has changed in GCC %{12.1%}", url);
> + }
> + return n;
> +}
> +
> /* Examine the argument and return set number of register required in each
> class. Return true iff parameter should be passed in memory. */
>
> --- gcc/testsuite/gcc.target/i386/pr102024.c.jj 2021-12-15 14:52:55.970248045
> +0100
> +++ gcc/testsuite/gcc.target/i386/pr102024.c 2021-12-15 15:15:43.629881418
> +0100
> @@ -0,0 +1,12 @@
> +/* PR target/102024 */
> +/* { dg-do compile } */
> +
> +struct S { float a; int : 0; float b; };
> +void foo (struct S x);
> +
> +void
> +bar (void)
> +{
> + struct S s = { 0.0f, 0.0f };
> + foo (s); /* { dg-message "the ABI of passing C structures with
> zero-width bit-fields has changed in GCC 12.1" "" { target { ! ia32 } } } */
> +}
> --- gcc/testsuite/g++.target/i386/pr102024.C.jj 2021-12-15 14:52:55.970248045
> +0100
> +++ gcc/testsuite/g++.target/i386/pr102024.C 2021-12-15 15:16:02.094619940
> +0100
> @@ -0,0 +1,12 @@
> +// PR target/102024
> +// { dg-do compile }
> +
> +struct S { float a; int : 0; float b; };
> +void foo (struct S x);
> +
> +void
> +bar (void)
> +{
> + struct S s = { 0.0f, 0.0f };
> + foo (s); // { dg-bogus "the ABI of passing C structures with
> zero-width bit-fields has changed in GCC 12.1" }
> +}
>
>
> Jakub
>
