c++, abi: Fix up class layout with bitfields [PR109039]
Commit Message
Hi!
The following testcase FAILs, because starting with r12-6028
the S class has only 2 bytes, not enough to hold one 7-bit bitfield, one 8-bit
bitfield and one 8-bit char field.
The reason is that when end_of_class attempts to compute dsize, it simply
adds byte_position of the field and DECL_SIZE_UNIT (and uses maximum from
those offsets).
The problematic bit-field in question has bit_position 7, byte_position 0,
DECL_SIZE 8 and DECL_SIZE_UNIT 1. So, byte_position + DECL_SIZE_UNIT is
1, even when the bitfield only has a single bit in the first byte and 7
further bits in the second byte, so per the Itanium ABI it should be 2:
"In either case, update dsize(C) to include the last byte
containing (part of) the bit-field, and update sizeof(C) to
max(sizeof(C),dsize(C))."
The following patch fixes it by computing bitsize of the end and using
CEIL_DIV_EXPR division to round it to next byte boundary and convert
from bits to bytes.
While this is an ABI change, classes with such incorrect layout couldn't
have worked properly, so I doubt anybody is actually running it often
in the wild. Thus I think adding some ABI warning for it is unnecessary.
Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk
(and after a while for GCC 12)?
2023-03-09 Jakub Jelinek <jakub@redhat.com>
PR c++/109039
* class.cc (end_of_class): For bit-fields, instead of computing
offset as sum of byte_position (field) and DECL_SIZE_UNIT (field),
compute it as sum of bit_position (field) and DECL_SIZE (field)
divided by BITS_PER_UNIT rounded up.
* g++.dg/abi/no_unique_address7.C: New test.
Jakub
Comments
On 3/9/23 14:40, Jakub Jelinek wrote:
> Hi!
>
> The following testcase FAILs, because starting with r12-6028
> the S class has only 2 bytes, not enough to hold one 7-bit bitfield, one 8-bit
> bitfield and one 8-bit char field.
>
> The reason is that when end_of_class attempts to compute dsize, it simply
> adds byte_position of the field and DECL_SIZE_UNIT (and uses maximum from
> those offsets).
> The problematic bit-field in question has bit_position 7, byte_position 0,
> DECL_SIZE 8 and DECL_SIZE_UNIT 1. So, byte_position + DECL_SIZE_UNIT is
> 1, even when the bitfield only has a single bit in the first byte and 7
> further bits in the second byte, so per the Itanium ABI it should be 2:
> "In either case, update dsize(C) to include the last byte
> containing (part of) the bit-field, and update sizeof(C) to
> max(sizeof(C),dsize(C))."
>
> The following patch fixes it by computing bitsize of the end and using
> CEIL_DIV_EXPR division to round it to next byte boundary and convert
> from bits to bytes.
>
> While this is an ABI change, classes with such incorrect layout couldn't
> have worked properly, so I doubt anybody is actually running it often
> in the wild. Thus I think adding some ABI warning for it is unnecessary.
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk
> (and after a while for GCC 12)?
OK.
> 2023-03-09 Jakub Jelinek <jakub@redhat.com>
>
> PR c++/109039
> * class.cc (end_of_class): For bit-fields, instead of computing
> offset as sum of byte_position (field) and DECL_SIZE_UNIT (field),
> compute it as sum of bit_position (field) and DECL_SIZE (field)
> divided by BITS_PER_UNIT rounded up.
>
> * g++.dg/abi/no_unique_address7.C: New test.
>
> --- gcc/cp/class.cc.jj 2023-02-04 06:22:17.053407477 +0100
> +++ gcc/cp/class.cc 2023-03-09 18:02:43.967815721 +0100
> @@ -6476,7 +6476,15 @@ end_of_class (tree t, eoc_mode mode)
> size of the type (usually 1) for computing nvsize. */
> size = TYPE_SIZE_UNIT (TREE_TYPE (field));
>
> - offset = size_binop (PLUS_EXPR, byte_position (field), size);
> + if (DECL_BIT_FIELD_TYPE (field))
> + {
> + offset = size_binop (PLUS_EXPR, bit_position (field),
> + DECL_SIZE (field));
> + offset = size_binop (CEIL_DIV_EXPR, offset, bitsize_unit_node);
> + offset = fold_convert (sizetype, offset);
> + }
> + else
> + offset = size_binop (PLUS_EXPR, byte_position (field), size);
> if (tree_int_cst_lt (result, offset))
> result = offset;
> }
> --- gcc/testsuite/g++.dg/abi/no_unique_address7.C.jj 2023-03-09 18:09:08.397205087 +0100
> +++ gcc/testsuite/g++.dg/abi/no_unique_address7.C 2023-03-09 18:08:56.439379395 +0100
> @@ -0,0 +1,33 @@
> +// PR c++/109039
> +// { dg-do run { target c++11 } }
> +
> +struct X {
> + signed short x0 : 7;
> + signed short x1 : 8;
> + X () : x0 (1), x1 (2) {}
> + int get () { return x0 + x1; }
> +};
> +
> +struct S {
> + [[no_unique_address]] X x;
> + signed char c;
> + S () : c (0) {}
> +};
> +
> +S s;
> +
> +int
> +main ()
> +{
> + if (s.x.x0 != 1 || s.x.x1 != 2 || s.c != 0)
> + __builtin_abort ();
> + s.x.x0 = -1;
> + s.x.x1 = -1;
> + if (s.x.x0 != -1 || s.x.x1 != -1 || s.c != 0)
> + __builtin_abort ();
> + s.c = -1;
> + s.x.x0 = 0;
> + s.x.x1 = 0;
> + if (s.x.x0 != 0 || s.x.x1 != 0 || s.c != -1)
> + __builtin_abort ();
> +}
>
> Jakub
>
@@ -6476,7 +6476,15 @@ end_of_class (tree t, eoc_mode mode)
size of the type (usually 1) for computing nvsize. */
size = TYPE_SIZE_UNIT (TREE_TYPE (field));
- offset = size_binop (PLUS_EXPR, byte_position (field), size);
+ if (DECL_BIT_FIELD_TYPE (field))
+ {
+ offset = size_binop (PLUS_EXPR, bit_position (field),
+ DECL_SIZE (field));
+ offset = size_binop (CEIL_DIV_EXPR, offset, bitsize_unit_node);
+ offset = fold_convert (sizetype, offset);
+ }
+ else
+ offset = size_binop (PLUS_EXPR, byte_position (field), size);
if (tree_int_cst_lt (result, offset))
result = offset;
}
@@ -0,0 +1,33 @@
+// PR c++/109039
+// { dg-do run { target c++11 } }
+
+struct X {
+ signed short x0 : 7;
+ signed short x1 : 8;
+ X () : x0 (1), x1 (2) {}
+ int get () { return x0 + x1; }
+};
+
+struct S {
+ [[no_unique_address]] X x;
+ signed char c;
+ S () : c (0) {}
+};
+
+S s;
+
+int
+main ()
+{
+ if (s.x.x0 != 1 || s.x.x1 != 2 || s.c != 0)
+ __builtin_abort ();
+ s.x.x0 = -1;
+ s.x.x1 = -1;
+ if (s.x.x0 != -1 || s.x.x1 != -1 || s.c != 0)
+ __builtin_abort ();
+ s.c = -1;
+ s.x.x0 = 0;
+ s.x.x1 = 0;
+ if (s.x.x0 != 0 || s.x.x1 != 0 || s.c != -1)
+ __builtin_abort ();
+}