Re: [PATCH] c++, abi: Fix up class layout with bitfields [PR109039]

[Jason Merrill <jason@redhat.com>]

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
>