| Message ID | 20210831080558.GT920497@tucnak |
|---|---|
| State | New |
| Headers | show |
| Series | x86_64: Issue -Wpsabi warning about C++ zero width bitfield ABI changes [PR102024] | expand |
Hi! On Tue, Aug 31, 2021 at 10:05:58AM +0200, Jakub Jelinek via Gcc-patches wrote: > This is an incremental patch to > https://gcc.gnu.org/pipermail/gcc-patches/2021-August/578447.html > for x86_64 ABI. > 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). 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? > The following patch assumes the current GCC C behavior of not ignoring them > when not at bitpositions divisible by 64 is right (though I'm really not > sure about that) and implements a -Wpsabi warning for that case. > The psABI IMHO should be clarified in any case. > The other option is to start ignoring them always (and issue -Wpsabi warning > if !DECL_FIELD_ABI_IGNORED on zero-width bitfield and it would change the > outcome). > > I think libffi doesn't need changing, as I think it doesn't support > bitfields. > > Bootstrapped/regtested on x86_64-linux and i686-linux. > > 2021-08-31 Jakub Jelinek <jakub@redhat.com> > > PR target/102024 > * config/i386/i386.c (classify_argument): Add cxx_bitfields argument, > when seeing DECL_FIELD_ABI_IGNORED bitfields either set it to 1 or > if equal to 2 ignore 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 Mon, Nov 29, 2021 at 3:24 AM Jakub Jelinek <jakub@redhat.com> wrote: > > Hi! > > On Tue, Aug 31, 2021 at 10:05:58AM +0200, Jakub Jelinek via Gcc-patches wrote: > > This is an incremental patch to > > https://gcc.gnu.org/pipermail/gcc-patches/2021-August/578447.html > > for x86_64 ABI. > > 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). > > 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 > > The following patch assumes the current GCC C behavior of not ignoring them > > when not at bitpositions divisible by 64 is right (though I'm really not > > sure about that) and implements a -Wpsabi warning for that case. > > The psABI IMHO should be clarified in any case. > > The other option is to start ignoring them always (and issue -Wpsabi warning > > if !DECL_FIELD_ABI_IGNORED on zero-width bitfield and it would change the > > outcome). > > > > I think libffi doesn't need changing, as I think it doesn't support > > bitfields. > > > > Bootstrapped/regtested on x86_64-linux and i686-linux. > > > > 2021-08-31 Jakub Jelinek <jakub@redhat.com> > > > > PR target/102024 > > * config/i386/i386.c (classify_argument): Add cxx_bitfields argument, > > when seeing DECL_FIELD_ABI_IGNORED bitfields either set it to 1 or > > if equal to 2 ignore 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 >
--- gcc/config/i386/i386.c.jj 2021-08-30 08:36:11.070518705 +0200 +++ gcc/config/i386/i386.c 2021-08-30 15:06:07.339940410 +0200 @@ -2062,7 +2062,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 &cxx_bitfields) { HOST_WIDE_INT bytes = mode == BLKmode ? int_size_in_bytes (type) : (int) GET_MODE_SIZE (mode); @@ -2120,6 +2121,12 @@ classify_argument (machine_mode mode, co misaligned integers. */ if (DECL_BIT_FIELD (field)) { + if (DECL_FIELD_ABI_IGNORED (field)) + { + if (cxx_bitfields == 2) + continue; + cxx_bitfields = 1; + } for (i = (int_bit_position (field) + (bit_offset % 64)) / 8 / 8; i < ((int_bit_position (field) + (bit_offset % 64)) @@ -2157,7 +2164,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, + cxx_bitfields); if (!num) return 0; pos = (int_bit_position (field) @@ -2175,7 +2183,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, + cxx_bitfields); if (!num) return 0; @@ -2206,7 +2215,7 @@ classify_argument (machine_mode mode, co num = classify_argument (TYPE_MODE (TREE_TYPE (field)), TREE_TYPE (field), subclasses, - bit_offset); + bit_offset, cxx_bitfields); if (!num) return 0; for (i = 0; i < num && i < words; i++) @@ -2226,7 +2235,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) @@ -2252,8 +2261,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)) { @@ -2462,6 +2471,43 @@ classify_argument (machine_mode mode, co } } +/* Wrapper around classify_argument with the extra cxx_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 cxx_bitfields = 0; + static bool warned = false; + int n = classify_argument (mode, type, classes, bit_offset, cxx_bitfields); + if (!cxx_bitfields || warned || !warn_psabi) + return n; + enum x86_64_reg_class alt_classes[MAX_CLASSES]; + cxx_bitfields = 2; + if (classify_argument (mode, type, alt_classes, bit_offset, + cxx_bitfields) != n) + cxx_bitfields = 3; + else + for (int i = 0; i < n; i++) + if (classes[i] != alt_classes[i]) + { + cxx_bitfields = 3; + break; + } + if (cxx_bitfields == 3) + { + warned = true; + const char *url + = CHANGES_ROOT_URL "gcc-12/changes.html#cxx_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-08-30 15:11:09.223718641 +0200 +++ gcc/testsuite/gcc.target/i386/pr102024.c 2021-08-30 15:11:45.426211495 +0200 @@ -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" } */ +} --- gcc/testsuite/g++.target/i386/pr102024.C.jj 2021-08-30 15:12:42.710409020 +0200 +++ gcc/testsuite/g++.target/i386/pr102024.C 2021-08-30 15:13:23.930831579 +0200 @@ -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 } } } +}
Hi! This is an incremental patch to https://gcc.gnu.org/pipermail/gcc-patches/2021-August/578447.html for x86_64 ABI. 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 following patch assumes the current GCC C behavior of not ignoring them when not at bitpositions divisible by 64 is right (though I'm really not sure about that) and implements a -Wpsabi warning for that case. The psABI IMHO should be clarified in any case. The other option is to start ignoring them always (and issue -Wpsabi warning if !DECL_FIELD_ABI_IGNORED on zero-width bitfield and it would change the outcome). I think libffi doesn't need changing, as I think it doesn't support bitfields. Bootstrapped/regtested on x86_64-linux and i686-linux. 2021-08-31 Jakub Jelinek <jakub@redhat.com> PR target/102024 * config/i386/i386.c (classify_argument): Add cxx_bitfields argument, when seeing DECL_FIELD_ABI_IGNORED bitfields either set it to 1 or if equal to 2 ignore 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