Bug 102024 - [12 Regression] zero width bitfields and ABIs
Summary: [12 Regression] zero width bitfields and ABIs
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: target (show other bugs)
Version: 12.0
: P1 normal
Target Milestone: 12.0
Assignee: Not yet assigned to anyone
URL:
Keywords: ABI
Depends on:
Blocks: 104796
  Show dependency treegraph
 
Reported: 2021-08-23 14:36 UTC by Jakub Jelinek
Modified: 2022-04-27 07:23 UTC (History)
22 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2021-08-23 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Jakub Jelinek 2021-08-23 14:36:54 UTC
My r12-2975-g32c3a75390623a0470df52af13f78baddd562981 change removed removal of zero width unnamed bitfields from the C++ FE, the C FE has never been removing those (the removal was certainly premature, as the newly added traits need to see the zero width unnamed bitfields).
This unfortunately can affect ABI on several targets.
From quick grep I think this can't affect
alpha, arc, avr, bfin, bpf, c6x, cr16, cris, csky, epiphany, fr30, frv, ft32, gcn, h8300, lm32, m32c, m32r, m68k, mcore, microblaze, mmix, mn10300, moxie, msp430, nds32, nios2, nvptx, or1k, pa, pdp11, pru, rl78, rx, stormy16, tilegx, tilepro, v850, vax, visium, xtensa
are all safe, they either don't mention TYPE_FIELDS in their backends or use it either in va_list or in structure layout code only (structure layout happens before the zero width bitfields are removed by the FE).
TYPE_MODE should be ok too, it is just passing of arguments by value and returning by value that could be problematic on
aarch64, arm, i386, ia64, iq2000, mips, riscv, rs6000, s390, sh and sparc
I think sh is ok, it ignores integer_zerop (DECL_SIZE (field)).

riscv apparently has old (< GCC 10) and new (>= GCC 10) ABI and the old one didn't ignore them and made C and C++ ABI incompatible, while the new one does ignore them.  So, my patch probably broke the 
"ABI for flattened struct with zero-length bit-fields changed in GCC 10"
warning (why isn't it guarded with -Wpsabi?), perhaps we should mark the C++ zero width bitfields with some attribute or flag so that backends could emit correct -Wpsabi warnings?

x86_64 seems to be affected:
struct S { float a; int : 0; float b; };

__attribute__((noipa)) struct S
foo (struct S x)
{
  return x;
}

void
bar (void)
{
  struct S s = { 0.0f, 0.0f };
  foo (s);
}

With GCC 10 and C we emit:
	xorl	%edi, %edi
	jmp	foo
With C++ we emit:
	pxor	%xmm0, %xmm0
	jmp	_Z3foo1S
with current trunk
	xorl	%edi, %edi
	jmp	_Z3foo1S
Comment 1 Jakub Jelinek 2021-08-23 14:40:26 UTC
clang and clang++ on x86_64 ignore the zero width bitfields, so work similarly to C++ in GCC < 12.
Comment 2 Jakub Jelinek 2021-08-23 15:20:38 UTC
Perhaps we can use DECL_FIELD_ABI_IGNORED flag for the zero width bitfields from C++ FE, right now that flag is only used on aggregate FIELD_DECLs.
Comment 3 Jakub Jelinek 2021-08-23 15:37:37 UTC
PowerPC seems to be (tried powerpc64le) affected as well on the same testcase,
 	addi 2,2,.TOC.-.LCF0@l
 	.localentry	_Z3barv,.-_Z3barv
 	mflr 0
-	li 9,0
-	sldi 9,9,32
-	mtvsrd 1,9
+	li 3,0
 	std 0,16(1)
 	stdu 1,-32(1)
 	.cfi_def_cfa_offset 32
 	.cfi_offset 65, 16
-	xscvspdpn 1,1
-	fmr 2,1
 	bl _Z3foo1S
 	nop
 	addi 1,1,32
seems to be the difference from C++ before my change to C++ after my change and the after my change matches what C did before and does now as well.
So like for x86-64 psABI, the question is what the psABI says and what we should use and either way we need -Wpsabi warning for the case we'll change ABI for.
Comment 4 rsandifo@gcc.gnu.org 2021-08-23 16:17:21 UTC
Thanks for raising this.  Just to summarise what I said on irc:

For aarch64 we'll have to decide between:

(a) keeping everything as it was for GCC 11, including the difference between C and C++
(b) keep the old C++ behaviour and make C match it
(c) keep the old C behaviour and make C++ match it (i.e. what happens on trunk now)

None of these are particularly appealing.  My understanding is that
(2) is what the psABI says, but we also have clang compatibility
to consider.

We'll need to discuss this internally a bit.  Obviously something
needs to be done in time for GCC 12, but due to other pressures,
I can't promise anything in time for end of stage 1.
Comment 5 Segher Boessenkool 2021-08-23 20:42:01 UTC
"structure layout happens before the zero width bitfields are removed by the FE".

So, what can break still, then?


-	li 9,0
-	sldi 9,9,32
-	mtvsrd 1,9
+	li 3,0
 	std 0,16(1)
 	stdu 1,-32(1)
 	.cfi_def_cfa_offset 32
 	.cfi_offset 65, 16
-	xscvspdpn 1,1
-	fmr 2,1

1) The old code was very inefficient, didn't have constants optimised;
2) The old code passed "s" in an FPR, the new one uses a GPR?  Can you also show
what changed in "foo"?
Comment 6 Segher Boessenkool 2021-08-23 20:49:50 UTC
(In reply to Segher Boessenkool from comment #5)
> "structure layout happens before the zero width bitfields are removed by the
> FE".
> 
> So, what can break still, then?

Homogeneous aggregates can break.  Before there wasn't one, but with the :0
removed there is.

> -	li 9,0
> -	sldi 9,9,32
> -	mtvsrd 1,9
> +	li 3,0
>  	std 0,16(1)
>  	stdu 1,-32(1)
>  	.cfi_def_cfa_offset 32
>  	.cfi_offset 65, 16
> -	xscvspdpn 1,1
> -	fmr 2,1

So this shows the backwards change?
Comment 7 Richard Biener 2021-08-24 07:06:29 UTC
So my gut feeling is that zero-width bitfields are more likely to be present in C code and thus not breaking the C ABI might be preferred.  Also any ABI incompatibility between C and C++ is bad so we do want to change either the C or the C++ FEs here.
Comment 8 Michael Matz 2021-08-24 14:13:40 UTC
The only thing the x86-64 psABI says about this case is "'Unnamed bit-fields' types do not affect the alignment of a structure or union." .

(zero-width bit-fields are _always_ unnamed)

But the x86-64 psABI was written with the assumption in mind that zero-width bitfields influence the alignment of the following field (i.e. that they break the current bit bucket for two neighboring bitfield members).  I.e. exactly what GCCs
stor-layout.c code always did for these when not in microsoft compat mode.

In the example in question this align-next-bitfield item doesn't come into play.  But it does show that zero-width bitfields were designed and allowed as extension for a purpose.  So it makes sense for instance on platforms that have a concept of homogenous aggregates that zero-width bit-fields disable those, without otherwise changing memory layout.

So, I think, not removing those members from the FE makes sense, they contain
crucial information.  Unfortunately that means that they need to be dealt with
in code dealing with layout (correct) or argument/return-value passing (seemingly
fishy right now for some platforms?).

Also, all else being equal I think the C language defines the de-facto psABI, so any difference between C and C++ should be resolved towards C.
Comment 9 Segher Boessenkool 2021-08-24 21:36:01 UTC
(In reply to Michael Matz from comment #8)
> So, I think, not removing those members from the FE makes sense, they contain
> crucial information.  Unfortunately that means that they need to be dealt
> with
> in code dealing with layout (correct) or argument/return-value passing
> (seemingly
> fishy right now for some platforms?).
> 
> Also, all else being equal I think the C language defines the de-facto
> psABI, so any difference between C and C++ should be resolved towards C.

Yeah.  In the case of Power homogeneous arguments, removing the bitfield
from  { float a; int:0; float b; }  is fine to do *later*, when the backend
already knows it is an homogeneous argument, when all decisions have been made
and the results recorded.  It would be good if this was documented somewhere,
where that spot is?
Comment 10 Bill Schmidt 2021-08-25 19:23:34 UTC
Sorry to be late jumping in.

Previously zero bitfields were spuriously removed, now they're being left in place and matching C.  That's very good.

As Jakub shows, the biggest problem is with homogeneous aggregates of floating-point and vector types in the ELFv2 ABI used for powerpc64le today.  Before Jakub's change, we would (wrongly) treat the "struct S" as a homogeneous aggregate and pass the arguments in floating-point registers.  With Jakub's change, we (correctly) pass the arguments in GPRs and/or memory.

Otherwise I think our ABIs are unaffected.

We'll need to add a Wpsabi message in the case where we used to recognize a homogeneous aggregate but no longer do.  We ought to be able to add that easily in rs6000_discover_homogeneous_aggregate (rs6000-call.c).
Comment 11 Bill Schmidt 2021-08-25 19:33:52 UTC
Actually, on further review, I guess we have additional concerns.  Unnamed bitfields also have the effect of updating alignment of the subsequent field of a structure.

"The types of unnamed bit fields have no effect on the alignment of a structure or union.  However, the offsets of an individual bit field's member must comply with the alignment rules. An unnamed bit field of zero width causes sufficient padding (possibly none) to be inserted for the next member, or the end of the structure if there are no more nonzero width members, to have an offset from the start of the structure that is a multiple of the size of the declared type of the zero-width member."

That's from ELFv2, but I imagine the same language exists in ELFv1, as I don't believe this was meant to be a change from the previous ABI.

An example from the ABI to test is:

struct {
  char c;
  int : 0;
  char d;
  short : 9;
  char e;
};

I wouuld expect that prior to Jakub's change, we would not see three bytes of pad between c and d, but with his change, we would.

We can probably warn for this as well, but might be a little trickier.  This is more likely to come up than the homogeneous aggregate case, I guess.
Comment 12 Bill Schmidt 2021-08-25 20:08:22 UTC
Never mind my last comment.  Segher pointed out that structure layout is done early enough that this isn't a problem.  I verified this using g++ from GCC 11 and GCC 12 to show that we get correct offsets for the previous example in both cases.
Comment 13 CVS Commits 2021-09-03 07:53:28 UTC
The master branch has been updated by Jakub Jelinek <jakub@gcc.gnu.org>:

https://gcc.gnu.org/g:e902136b310ee17d4b49eb42d9d5e487d5dcf4a1

commit r12-3324-ge902136b310ee17d4b49eb42d9d5e487d5dcf4a1
Author: Jakub Jelinek <jakub@redhat.com>
Date:   Fri Sep 3 09:46:32 2021 +0200

    c++, abi: Set DECL_FIELD_CXX_ZERO_WIDTH_BIT_FIELD on C++ zero width bitfields [PR102024]
    
    The removal of remove_zero_width_bitfields function and its call from
    C++ FE layout_class_type (which I've done in the P0466R5
    layout-compatible helper intrinsics patch, so that the FE can actually
    determine what is and isn't layout-compatible according to the spec)
    unfortunately changed the ABI on various platforms.
    The C FE has been keeping zero-width bitfields in the types, while
    the C++ FE has been removing them after structure layout, so in various
    cases when passing such structures in registers we had different ABI
    between C and C++.
    
    While both the C and C++ FE had some code to remove zero width bitfields
    after structure layout, in both FEs it was buggy and didn't really remove
    any.  In the C FE that code has been removed later on, while in the C++ FE
    for GCC 4.5 in PR42217 it has been actually fixed, so the C++ FE started
    to remove those bitfields.
    
    The following patch doesn't change anything ABI-wise, but allows the
    targets to decide what to do, emit -Wpsabi warnings etc.
    Non-C zero width bitfields will be seen by the backends as normal
    zero width bitfields, C++ zero width bitfields that used to be previously
    removed will have DECL_FIELD_CXX_ZERO_WIDTH_BIT_FIELD flag set.
    I've reused the DECL_FIELD_ABI_IGNORED flag which is only used on non-bitfield
    FIELD_DECLs right now, but the macros now check DECL_BIT_FIELD flag.
    
    Each backend can then decide what it wants, whether it wants to keep
    different ABI between C and C++ as in GCC 11 and older (i.e. incompatible
    with G++ <= 4.4, compatible with G++ 4.5 .. 11), for that it would
    ignore for the aggregate passing/returning decisions all
    DECL_FIELD_CXX_ZERO_WIDTH_BIT_FIELD FIELD_DECLs), whether it wants to never
    ignore zero width bitfields (no changes needed for that case, except perhaps
    -Wpsabi warning should be added and for that DECL_FIELD_CXX_ZERO_WIDTH_BIT_FIELD
    can be tested), or whether it wants to always ignore zero width bitfields
    (I think e.g. riscv in GCC 10+ does that).
    
    All this patch does is set the flag which the backends can then use.
    
    2021-09-03  Jakub Jelinek  <jakub@redhat.com>
    
            PR target/102024
    gcc/
            * tree.h (DECL_FIELD_ABI_IGNORED): Changed into rvalue only macro
            that is false if DECL_BIT_FIELD.
            (SET_DECL_FIELD_ABI_IGNORED, DECL_FIELD_CXX_ZERO_WIDTH_BIT_FIELD,
            SET_DECL_FIELD_CXX_ZERO_WIDTH_BIT_FIELD): Define.
            * tree-streamer-out.c (pack_ts_decl_common_value_fields): For
            DECL_BIT_FIELD stream DECL_FIELD_CXX_ZERO_WIDTH_BIT_FIELD instead
            of DECL_FIELD_ABI_IGNORED.
            * tree-streamer-in.c (unpack_ts_decl_common_value_fields): Use
            SET_DECL_FIELD_ABI_IGNORED instead of writing to
            DECL_FIELD_ABI_IGNORED and for DECL_BIT_FIELD use
            SET_DECL_FIELD_CXX_ZERO_WIDTH_BIT_FIELD instead.
            * lto-streamer-out.c (hash_tree): For DECL_BIT_FIELD hash
            DECL_FIELD_CXX_ZERO_WIDTH_BIT_FIELD instead of DECL_FIELD_ABI_IGNORED.
    gcc/cp/
            * class.c (build_base_field): Use SET_DECL_FIELD_ABI_IGNORED
            instead of writing to DECL_FIELD_ABI_IGNORED.
            (layout_class_type): Likewise.  In the place where zero-width
            bitfields used to be removed, use
            SET_DECL_FIELD_CXX_ZERO_WIDTH_BIT_FIELD on those fields instead.
    gcc/lto/
            * lto-common.c (compare_tree_sccs_1): Also compare
            DECL_FIELD_CXX_ZERO_WIDTH_BIT_FIELD values.
Comment 14 Jakub Jelinek 2021-09-03 08:12:00 UTC
The generic support is now committed, each backend can decide on what to do using those flags.  That decision should be guided by psABI if it is clear on those matters, or otherwise ideally the psABIs should be clarified.
Comment 15 Bill Schmidt 2021-09-21 16:16:56 UTC
Jakub, could you add a note with a section ID in https://gcc.gnu.org/gcc-12/changes.html as was done for the similar case in GCC 10?  This allowed us to specify a URL in the informational diagnostic, like so:

const char *url = CHANGES_ROOT_URL "gcc-10/changes.html#empty_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, url);
Comment 16 CVS Commits 2021-09-23 12:40:42 UTC
The master branch has been updated by William Schmidt <wschmidt@gcc.gnu.org>:

https://gcc.gnu.org/g:16e3d6b8b2b5168ebc773833f0e7ccf2191932c1

commit r12-3843-g16e3d6b8b2b5168ebc773833f0e7ccf2191932c1
Author: Bill Schmidt <wschmidt@linux.ibm.com>
Date:   Thu Sep 23 07:35:42 2021 -0500

    rs6000: Add psabi diagnostic for C++ zero-width bit field ABI change
    
    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.
    
    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.
Comment 17 CVS Commits 2022-01-10 16:47:55 UTC
The master branch has been updated by Jakub Jelinek <jakub@gcc.gnu.org>:

https://gcc.gnu.org/g:3159da6c46568a7c600f78fb3a3b76e2ea4bf4cc

commit r12-6418-g3159da6c46568a7c600f78fb3a3b76e2ea4bf4cc
Author: Jakub Jelinek <jakub@redhat.com>
Date:   Mon Jan 10 17:43:23 2022 +0100

    x86_64: Ignore zero width bitfields in ABI and issue -Wpsabi warning about C zero width bitfield ABI changes [PR102024]
    
    For zero-width bitfields current GCC classify_argument does:
                      if (DECL_BIT_FIELD (field))
                        {
                          for (i = (int_bit_position (field)
                                    + (bit_offset % 64)) / 8 / 8;
                               i < ((int_bit_position (field) + (bit_offset % 64))
                                    + tree_to_shwi (DECL_SIZE (field))
                                    + 63) / 8 / 8; i++)
                            classes[i]
                              = merge_classes (X86_64_INTEGER_CLASS, classes[i]);
                        }
    which I think means that if the zero-width bitfields are at bit-positions
    (in the toplevel aggregate) which are multiples of 64 bits doesn't do
    anything, (int_bit_position (field) + (bit_offset % 64)) / 64 and
    (int_bit_position (field) + (bit_offset % 64) + 63) / 64 should be equal.
    But for zero-width bitfields at other bit positions it will call
    merge_classes once.  Now, the typical case is that the zero width bitfield
    is surrounded by some bitfields and in that case, it doesn't change
    anything, but it can be sandwitched in between floats too as the testcases
    show.
    In C we had this behavior, in C++ previously the FE was removing the
    zero-width bitfields and therefore they were ignored.
    LLVM and ICC seems to ignore those bitfields both in C and C++ (== passing
    struct S in SSE register rather than in GPR).
    
    The x86-64 psABI has been recently clarified by
    https://gitlab.com/x86-psABIs/x86-64-ABI/-/commit/1aa4398d26c250b252a0c4a0f777216c9a6789ec
    that zero width bitfield should be always ignored.
    
    This patch implements that and emits a warning for C for cases where the ABI
    changed from GCC 11.
    
    2022-01-10  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.
Comment 18 Eric Botcazou 2022-03-24 09:39:17 UTC
> TYPE_MODE should be ok too, it is just passing of arguments by value and
> returning by value that could be problematic on
> aarch64, arm, i386, ia64, iq2000, mips, riscv, rs6000, s390, sh and sparc
> I think sh is ok, it ignores integer_zerop (DECL_SIZE (field)).

On SPARC, the 32-bit ABI passes structures by reference so is not concerned while the 64-bit ABI does the same as SH:

  /* Walk the real fields, but skip those with no size or a zero size.
     ??? Fields with variable offset are handled as having zero offset.  */
  for (tree field = TYPE_FIELDS (type); field; field = DECL_CHAIN (field))
    if (TREE_CODE (field) == FIELD_DECL)
      {
	if (!DECL_SIZE (field) || integer_zerop (DECL_SIZE (field)))
	  continue;

so is not affected either.
Comment 19 CVS Commits 2022-03-24 11:25:45 UTC
The master branch has been updated by Jakub Jelinek <jakub@gcc.gnu.org>:

https://gcc.gnu.org/g:568377743e22c1377d0aaa1ac9113da3ff1b6bd4

commit r12-7798-g568377743e22c1377d0aaa1ac9113da3ff1b6bd4
Author: Jakub Jelinek <jakub@redhat.com>
Date:   Thu Mar 24 12:25:15 2022 +0100

    testsuite: Add compat.exp testcase for most common zero width bitfld ABI passing [PR102024]
    
    On Tue, Mar 22, 2022 at 05:51:58PM +0100, Jakub Jelinek via Gcc wrote:
    > I guess it would be nice to include the testcases we are talking about,
    > like { float x; int : 0; float y; } and { float x; int : 0; } and
    > { int : 0; float x; } into compat.exp testsuite so that we see ABI
    > differences in compat testing.
    
    Here is a patch that does that.  It uses the struct-layout-1* framework,
    but isn't generated because we don't want in this case pseudo-random
    structure layouts, but particular ones we know cause or could cause problems
    on some targets.  If other problematic cases are discovered, we can add
    further ones.
    
    Tested on x86_64-linux with:
    make check-gcc check-g++ RUNTESTFLAGS='ALT_CC_UNDER_TEST=gcc ALT_CXX_UNDER_TEST=g++ compat.exp=pr102*'
    and with
    make check-gcc check-g++ RUNTESTFLAGS='compat.exp=pr102*'
    The former as expected has:
    FAIL: gcc.dg/compat/pr102024 c_compat_x_tst.o-c_compat_y_alt.o execute
    FAIL: gcc.dg/compat/pr102024 c_compat_x_alt.o-c_compat_y_tst.o execute
    fails because on x86_64 we've changed the C ABI but kept the C++ ABI here.
    E.g. on rs6000 it should be the g++.dg such tests to fail (all assuming
    the alt gcc/g++ is GCC 4.5 through 11).
    
    2022-03-24  Jakub Jelinek  <jakub@redhat.com>
    
            PR target/102024
            * gcc.dg/compat/pr102024_main.c: New test.
            * gcc.dg/compat/pr102024_test.h: New test.
            * gcc.dg/compat/pr102024_x.c: New test.
            * gcc.dg/compat/pr102024_y.c: New test.
            * g++.dg/compat/pr102024_main.C: New test.
            * g++.dg/compat/pr102024_test.h: New test.
            * g++.dg/compat/pr102024_x.C: New test.
            * g++.dg/compat/pr102024_y.C: New test.
Comment 20 CVS Commits 2022-03-29 16:44:56 UTC
The master branch has been updated by Richard Earnshaw <rearnsha@gcc.gnu.org>:

https://gcc.gnu.org/g:3032df28f2a1cc6514571558b76d9b80373b19c6

commit r12-7895-g3032df28f2a1cc6514571558b76d9b80373b19c6
Author: Richard Earnshaw <rearnsha@arm.com>
Date:   Tue Mar 29 10:24:49 2022 +0100

    arm: correctly handle zero-sized bit-fields in AAPCS [PR102024]
    
    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.
Comment 21 CVS Commits 2022-03-29 16:45:02 UTC
The master branch has been updated by Richard Earnshaw <rearnsha@gcc.gnu.org>:

https://gcc.gnu.org/g:b243ad1afb7f06ef4ab7649600d900b09b9c6b52

commit r12-7896-gb243ad1afb7f06ef4ab7649600d900b09b9c6b52
Author: Richard Earnshaw <rearnsha@arm.com>
Date:   Tue Mar 29 16:07:09 2022 +0100

    aarch64: correctly handle zero-sized bit-fields in AAPCS64 [PR102024]
    
    On aarch64 the AAPCS64 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/aarch64/aarch64.cc (aapcs_vfp_sub_candidate): Handle
            zero-sized bit-fields.  Detect cases where a warning may be needed.
            (aarch64_vfp_is_call_or_return_candidate): Emit a note if a
            zero-sized bit-field has caused parameter passing to change.
    
    gcc/testsuite/ChangeLog:
    
            * gcc.target/aarch64/aapcs64/test_28.c: New test.
Comment 22 Jakub Jelinek 2022-03-30 10:27:30 UTC
So, to sum up:
aarch64, arm, x86-64 and riscv (last one since GCC 10 already) do ignore zero width bit-fields in argument/return value passing decisions and so have a
C ABI incompatibility from earlier GCC versions.  There is -Wpsabi diagnostics on affected argument/return value passing.
powerpc64le and s390x do not ignore them, so have a C++ ABI incompatibility
from earlier GCC versions.  There is -Wpsabi diagnostics on affected argument/return value passing.
alpha, arc, avr, bfin, bpf, c6x, cr16, cris, csky, epiphany, fr30, frv, ft32, gcn, h8300, i386 (32-bit), lm32, m32c, m32r, m68k, mcore, microblaze, mmix, mn10300, moxie, msp430, nds32, nios2, nvptx, or1k, pa, pdp11, powerpc (except for powerpc64le), pru, rl78, rx, sh, sparc, stormy16, tilegx, tilepro, v850, vax, visium and xtensa are not affected.
ia64 and iq2000 are unknown.
mips seems to be affected but nothing has been done about it, which means that right now if it is affected, it is not ignoring the zero width bit-fields without -Wpsabi diagnostics.
Except for the not affected list and maybe unknown list, there was also an ABI 
change between G++ 4.4 and earlier and 4.5 and later.
Comment 23 Jakub Jelinek 2022-03-30 10:39:16 UTC
As for mips,
https://techpubs.jurassic.nl/manuals/0640/developer/Mpro_n32_ABI/sgi_html/ch02.html
says e.g.:
"Regardless of the struct field structure, it is treated as a sequence of 64-bit chunks. If a chunk consists solely of a double float field (but not a double, which is part of a union), it is passed in a floating point register. Any other chunk is passed in an integer register."
but it is ambiguous if in cases like:
struct A { double a; int : 0; double b; };
struct B { double a; int : 0; };
struct C { int : 0; double a; };
those zero width bit-fields are considered part of any chunk and if so, which one (as they are zero size and on the chunk boundaries).
The return value rule seems less ambigous:
"A struct with only one or two floating point fields is returned in $f0 (and $f2 if necessary). This is a generalization of the Fortran COMPLEX case."
my reading of that is that zero width bitfields shouldn't be ignored in that case, because if they are present, the struct doesn't contain only one or two floating point fields, it contains at least the zero width bitfield as well.

CCing MIPS maintainers on this (and also LoongArch, while there is no ABI incompatibility for a new port, it is perhaps something to discuss and decide too).
Comment 24 Hans-Peter Nilsson 2022-03-30 12:40:24 UTC
(In reply to Jakub Jelinek from comment #23)
> CCing MIPS maintainers on this (and also LoongArch, while there is no ABI
> incompatibility for a new port, it is perhaps something to discuss and
> decide too).

Not a MIPS maintainer, and none of my targets are affected.  While it seems likely I was just one the next line, there's a slight chance you meant to add someone else, which would be important and a valid reason for this long sentence and I think I'll stay CC:ed. :)
Comment 25 Jakub Jelinek 2022-03-30 12:43:52 UTC
Sorry, apparently I must have misread the mmix below mips as being mips too.
Comment 26 Jakub Jelinek 2022-03-30 13:05:22 UTC
Reading the mips_function_arg code in detail, it seems it has been ignoring the zero width bitfields because it only checks for 64-bit aligned 64-bit double fields and if they appear, passes that part of struct in FPRs, otherwise in GPRs.
So there is likely no change in that case from older releases (of course it is a question what the psABI wants).
mips_fpr_return_fields seems to be affected, but the psABI wording (at least my reading thereof) seems to be that int:0; should make it return 0 like it now does
(but didn't for C++ in 4.5 to 11).
Thus maybe only -Wpsabi diagnostics for mips_fpr_return_fields is missing.

Let me close this as a P1 for now then.
Comment 27 Xi Ruoyao 2022-03-30 13:42:27 UTC
(In reply to Jakub Jelinek from comment #23)

> struct A { double a; int : 0; double b; };

For MIPS I've done some experiment with this and the result (with N64 ABI) is:

With GCC trunk, G++ trunk, and GCC 11.2: argument passed via FPR $f12 and GPR $5,
returned via GPR $2 and $3

With G++ 11.2: argument passed via FPR $f12 and $f13, returned via FPR $f0 and $f2

So I guess we need -Wpsabi for both mips_function_arg and mips_fpr_return_fields.
Comment 28 Jakub Jelinek 2022-03-30 13:53:27 UTC
(In reply to Xi Ruoyao from comment #27)
> (In reply to Jakub Jelinek from comment #23)
> 
> > struct A { double a; int : 0; double b; };
> 
> For MIPS I've done some experiment with this and the result (with N64 ABI)
> is:
> 
> With GCC trunk, G++ trunk, and GCC 11.2: argument passed via FPR $f12 and
> GPR $5,
> returned via GPR $2 and $3

Ah, indeed.
The
              for (; field; field = DECL_CHAIN (field))
                if (TREE_CODE (field) == FIELD_DECL
                    && int_bit_position (field) >= bitpos)
                  break;
loop will simply stop on the first field with bitpos equal or greater than bitpos, so the zero sized bit-fields (and generally any other zero sized fields
like struct S {} s; in GNU C) will be treated as being part of the next slot.
So in struct B { int : 0; double a, b; }; it will go into GPR and FPR,
while struct C { double a; int : 0; double b; }; into FPR and GPR, and
struct D { double a, b; int : 0; } into FPR & FPR.

> With G++ 11.2: argument passed via FPR $f12 and $f13, returned via FPR $f0
> and $f2
> 
> So I guess we need -Wpsabi for both mips_function_arg and
> mips_fpr_return_fields.

Is there somebody who can clarify the MIPS ABI intent?
Also, what does LLVM do?
Comment 29 Xi Ruoyao 2022-03-30 14:34:38 UTC
> Is there somebody who can clarify the MIPS ABI intent?
> Also, what does LLVM do?

I've CC'ed Yunqiang and Fangrui.  And I'll build clang for MIPS to see...
Comment 30 Xi Ruoyao 2022-03-30 19:02:07 UTC
(In reply to Jakub Jelinek from comment #28)

> Also, what does LLVM do?

clang-14 agree with gcc-12 on the return values, as we expected (the ABI documentation is clear enough).

But clang-14 treats arguments differently:

struct foo
{
  int : 0;
  double a;
  int : 0;
  double b;
  int : 0;
};

extern void func(struct foo);

void
pass_foo(void)
{
  struct foo test;
  test.a = 114;
  test.b = 514;
  func(test);
}

It puts "a" into $f12 and "b" into $f13.  So the behavior of clang-14 and clang++-14 handling arguments with zero-width bit-fields is same as g++-11, and different from g++-12, gcc-11, and gcc-12.

I'm not sure if we should keep our current behavior, or change it to match LLVM.
Comment 31 Segher Boessenkool 2022-03-31 12:36:03 UTC
Well, what do other compilers do?  It's not such a good idea to break ABI
compatibility with the 1990's compilers ;-)
Comment 32 Jakub Jelinek 2022-03-31 12:50:22 UTC
One of the reasons we changed the ABI on various arches one way or another is that we had this ABI incompatibility between C and C++, that just can't be right.
At that time we can and should decide if we go for compatibility with C or C++.
The MIPS n64 wording I could find:
"Regardless of the struct field structure, it is treated as a sequence of 64-bit chunks. If a chunk consists solely of a double float field (but not a double, which is part of a union), it is passed in a floating point register. Any other chunk is passed in an integer register."
is unclear for both the zero width bitfields and empty structure/union C members,
members that have zero size and are on the 64-bit chunk boundaries can belong to either previous or next chunk or can be ignored altogether.

I think best would be to ignore them altogether, especially if other compilers do that too.
Also consider:
struct T {};
struct S { struct T t; double a; struct T u; double b; };
struct U { int a[0]; double b; int c[0]; double d; };
argument passing in C (etc.) - in C++ struct T will have size 1 and therefore will be passed differently between C (which pedantically disallows empty structures or zero length arrays) and C++.
Comment 33 Xi Ruoyao 2022-03-31 15:08:40 UTC
(In reply to Segher Boessenkool from comment #31)
> Well, what do other compilers do?  It's not such a good idea to break ABI
> compatibility with the 1990's compilers ;-)

Does someone have access to a Greenhills compiler here?

(In reply to Jakub Jelinek from comment #32)

> I think best would be to ignore them altogether, especially if other
> compilers do that too.

I agree the behavior of clang (or previous G++) is more "rational".  To make things more "interesting":

> So in struct B { int : 0; double a, b; }; it will go into GPR and FPR

GCC trunk puts "a" into FPR, not GPR!  So the "leading" zero-width bit-fields are ignored (GCC does not think it's a part of any "64-bit chunk"), but other zero-width bit-fields are accounted...  This is just puzzling to me.

I'll make a patch to match the behavior of clang.
Comment 34 Xi Ruoyao 2022-03-31 15:14:16 UTC
(In reply to Xi Ruoyao from comment #33)

> > So in struct B { int : 0; double a, b; }; it will go into GPR and FPR
> 
> GCC trunk puts "a" into FPR, not GPR!  So the "leading" zero-width
> bit-fields are ignored (GCC does not think it's a part of any "64-bit
> chunk"), but other zero-width bit-fields are accounted...  This is just
> puzzling to me.

Remove this: I just forgot to rename ".C" to ".c" when I tested this with GCC 11.  But still I think clang's behavior is better.

> I'll make a patch to match the behavior of clang.
Comment 35 CVS Commits 2022-04-01 09:50:14 UTC
The master branch has been updated by Jakub Jelinek <jakub@gcc.gnu.org>:

https://gcc.gnu.org/g:e0ce885851dfd926c0cfe6f23a2debc87ea2bb9d

commit r12-7951-ge0ce885851dfd926c0cfe6f23a2debc87ea2bb9d
Author: Jakub Jelinek <jakub@redhat.com>
Date:   Fri Apr 1 11:49:40 2022 +0200

    testsuite: Add further zero size elt passing tests [PR102024]
    
    As discussed in PR102024, zero width bitfields might not be the only ones
    causing ABI issues at least on mips, zero size arrays or (in C only) zero
    sized (empty) structures can be problematic too.
    
    The following patch adds some coverage for it too.
    
    Tested on x86_64-linux with
    make check-gcc check-g++ RUNTESTFLAGS='ALT_CC_UNDER_TEST=gcc ALT_CXX_UNDER_TEST=g++ --target_board=unix\{-m32,-m64\} compat.exp=pr102024*'
    make check-gcc check-g++ RUNTESTFLAGS='ALT_CC_UNDER_TEST=clang ALT_CXX_UNDER_TEST=clang++ --target_board=unix\{-m32,-m64\} compat.exp=pr102024*'
    with gcc/g++ 10.3 and clang 11.  Everything but (expectedly)
    FAIL: gcc.dg/compat/pr102024 c_compat_x_tst.o-c_compat_y_alt.o execute
    FAIL: gcc.dg/compat/pr102024 c_compat_x_alt.o-c_compat_y_tst.o execute
    for -m64 ALT_CC_UNDER_TEST=gcc passes.
    
    2022-04-01  Jakub Jelinek  <jakub@redhat.com>
    
            PR target/102024
            * gcc.dg/compat/pr102024_test.h: Add further tests with zero sized
            structures and arrays.
            * g++.dg/compat/pr102024_test.h: Add further tests with zero sized
            arrays.
Comment 36 CVS Commits 2022-04-01 14:39:44 UTC
The master branch has been updated by Xi Ruoyao <xry111@gcc.gnu.org>:

https://gcc.gnu.org/g:0d4b97f1ee5213dffce107bc9f260a22fb23b4b1

commit r12-7961-g0d4b97f1ee5213dffce107bc9f260a22fb23b4b1
Author: Xi Ruoyao <xry111@mengyan1223.wang>
Date:   Wed Mar 30 22:22:45 2022 +0800

    mips: Emit psabi diagnostic for return values affected by C++ zero-width bit-field ABI change [PR 102024]
    
    gcc/
            PR target/102024
            * config/mips/mips.cc (mips_fpr_return_fields): Detect C++
            zero-width bit-fields and set up an indicator.
            (mips_return_in_msb): Adapt for mips_fpr_return_fields change.
            (mips_function_value_1): Diagnose when the presense of a C++
            zero-width bit-field changes function returning in GCC 12.
    
    gcc/testsuite/
            PR target/102024
            * g++.target/mips/mips.exp: New test supporting file.
            * g++.target/mips/pr102024.C: New test.
Comment 37 CVS Commits 2022-04-01 14:39:49 UTC
The master branch has been updated by Xi Ruoyao <xry111@gcc.gnu.org>:

https://gcc.gnu.org/g:413187b0b3c873333253838e4afbf8463b288b59

commit r12-7962-g413187b0b3c873333253838e4afbf8463b288b59
Author: Xi Ruoyao <xry111@mengyan1223.wang>
Date:   Thu Mar 31 23:40:23 2022 +0800

    mips: Ignore zero width fields in arguments and issue -Wpsabi warning about C zero-width field ABI changes [PR102024]
    
    gcc/
            PR target/102024
            * config/mips/mips.cc (mips_function_arg): Ignore zero-width
            fields, and inform if it causes a psABI change.
    
    gcc/testsuite/
            PR target/102024
            * gcc.target/mips/pr102024-1.c: New test.
            * gcc.target/mips/pr102024-2.c: New test.
            * gcc.target/mips/pr102024-3.c: New test.
Comment 38 Segher Boessenkool 2022-04-02 00:47:40 UTC
+ cat test.c

struct foo
{
  int : 0;
  double a;
  int : 0;
  double b;
  int : 0;
};

extern void func(struct foo);

void
pass_foo(void)
{
  struct foo test;
  test.a = 114;
  test.b = 514;
  func(test);
}
+ cc -version
MIPSpro Compilers: Version 7.30
+ cc -64 -ansi -c test.c -o test.o
Cannot find license file (-1,73:2) No such file or directory

 This product (cc) requires a license password. 
 For license installation and trouble shooting 
 information visit the web page: 

         http://www.sgi.com/Support/Licensing/install_docs.html 

 To obtain a Permanent license (proof of purchase
 required) or an Evaluation license please
 visit our license request web page: 

         http://www.sgi.com/Products/license.html 

         or send a blank email message to: 

         license@sgi.com 

 In North America, Silicon Graphics' customers may request 
 Permanent licenses by sending a facsimile to: 

         (650) 390-0537 

         or by calling our technical support hotline 

         1-800-800-4SGI 

 If you are Outside of North America or you are not a Silicon 
 Graphics support customer then contact your local support provider. 

qemu: Unsupported syscall: sgisys(107)
qemu: Unsupported syscall: sgisys(111)
qemu: Unsupported syscall: sgisys(107)
qemu: Unsupported syscall: sgisys(111)

### Assertion failure at line 137 of ../../be/cg/MIPS/exp_loadstore.cxx:
### Compiler Error in file test.c during Code_Expansion phase:
### unexpected mtypes in Pick_Load_Instruction
cc INTERNAL ERROR:  /usr/lib32/cmplrs/be returned non-zero status 1
Comment 39 CVS Commits 2022-04-27 07:23:41 UTC
The master branch has been updated by Andreas Krebbel <krebbel@gcc.gnu.org>:

https://gcc.gnu.org/g:bc79f0d9048375e402497d5f2ef457c9500310e4

commit r12-8277-gbc79f0d9048375e402497d5f2ef457c9500310e4
Author: Andreas Krebbel <krebbel@linux.ibm.com>
Date:   Wed Apr 27 09:20:41 2022 +0200

    PR102024 - IBM Z: Add psabi diagnostics
    
    For IBM Z in particular there is a problem with structs like:
    
    struct A { float a; int :0; };
    
    Our ABI document allows passing a struct in an FPR only if it has
    exactly one member. On the other hand it says that structs of 1,2,4,8
    bytes are passed in a GPR. So this struct is expected to be passed in
    a GPR. Since we don't return structs in registers (regardless of the
    number of members) it is always returned in memory.
    
    Situation is as follows:
    
    All compiler versions tested return it in memory - as expected.
    
    gcc 11, gcc 12, g++ 12, and clang 13 pass it in a GPR - as expected.
    
    g++ 11 as well as clang++ 13 pass in an FPR
    
    For IBM Z we stick to the current GCC 12 behavior, i.e. zero-width
    bitfields are NOT ignored.  A struct as above will be passed in a
    GPR. Rational behind this is that not affecting the C ABI is more
    important here.
    
    A patch for clang is in progress: https://reviews.llvm.org/D122388
    
    In addition to the usual regression test I ran the compat and
    struct-layout-1 testsuites comparing the compiler before and after the
    patch.
    
    gcc/ChangeLog:
            PR target/102024
            * config/s390/s390-protos.h (s390_function_arg_vector): Remove
            prototype.
            * config/s390/s390.cc (s390_single_field_struct_p): New function.
            (s390_function_arg_vector): Invoke s390_single_field_struct_p.
            (s390_function_arg_float): Likewise.
    
    gcc/testsuite/ChangeLog:
            PR target/102024
            * g++.target/s390/pr102024-1.C: New test.
            * g++.target/s390/pr102024-2.C: New test.
            * g++.target/s390/pr102024-3.C: New test.
            * g++.target/s390/pr102024-4.C: New test.
            * g++.target/s390/pr102024-5.C: New test.
            * g++.target/s390/pr102024-6.C: New test.