x86_64: Ignore zero width bitfields in ABI and issue -Wpsabi warning about C zero width bitfield ABI changes [PR102024]

  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  <jakub@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.



	Jakub
  

On Wed, Dec 15, 2021 at 3:50 PM Jakub Jelinek <jakub@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  <jakub@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
>
  

Hello,

On Mon, 20 Dec 2021, Uros Bizjak wrote:

> > 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  <jakub@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.

Christmas came in the way, but I just merged the proposed change 
(zero-with bit-fields -> NO_CLASS) into the psABI.


Ciao,
Michael.

> 
> 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
> >
>
  

On Mon, Jan 10, 2022 at 3:23 PM Michael Matz <matz@suse.de> wrote:
>
> Hello,
>
> On Mon, 20 Dec 2021, Uros Bizjak wrote:
>
> > > 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  <jakub@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.
>
> Christmas came in the way, but I just merged the proposed change
> (zero-with bit-fields -> NO_CLASS) into the psABI.

Thanks - LGTM for the patch.

Thanks,
Uros.