Re: [PATCH] c++: Fix handling of decls with flexible array members initialized with side-effects [PR88578]

[Jason Merrill <jason@redhat.com>]

On 9/15/21 14:59, Jakub Jelinek wrote:
> On Tue, Sep 14, 2021 at 10:50:32AM -0400, Jason Merrill wrote:
>>> Note, if the flexible array member is initialized only with non-constant
>>> initializers, we have a worse bug that this patch doesn't solve, the
>>> splitting of initializers into constant and dynamic initialization removes
>>> the initializer and we don't have just wrong DECL_*SIZE, but nothing is
>>> emitted when emitting those vars into assembly either and so the dynamic
>>> initialization clobbers other vars that may overlap the variable.
>>> I think we need keep an empty CONSTRUCTOR elt in DECL_INITIAL for the
>>> flexible array member in that case.
>>
>> Makes sense.
> 
> So, the following patch fixes that.
> 
> The typeck2.c change makes sure we keep those CONSTRUCTORs around (although
> they should be empty because all their elts had side-effects/was
> non-constant if it was removed earlier), and the varasm.c change is to avoid
> ICEs on those as well as ICEs on other flex array members that had some
> initializers without side-effects, but not on the last array element.
> 
> The code was already asserting that the (index of the last elt in the
> CONSTRUCTOR + 1) times elt size is equal to TYPE_SIZE_UNIT of the local->val
> type, which is true for C flex arrays or for C++ if they don't have any
> side-effects or the last elt doesn't have side-effects, this patch changes
> that to assertion that the TYPE_SIZE_UNIT is greater than equal to the
> offset of the end of last element in the CONSTRUCTOR and uses TYPE_SIZE_UNIT
> (int_size_in_bytes) in the code later on.
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
> 
> 2021-09-15  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR c++/88578
> 	PR c++/102295
> gcc/
> 	* varasm.c (output_constructor_regular_field): Instead of assertion
> 	that array_size_for_constructor result is equal to size of
> 	TREE_TYPE (local->val) in bytes, assert that the type size is greater
> 	or equal to array_size_for_constructor result and use type size as
> 	fieldsize.
> gcc/cp/
> 	* typeck2.c (split_nonconstant_init_1): Don't throw away empty
> 	initializers of flexible array members if they have non-zero type
> 	size.
> gcc/testsuite/
> 	* g++.dg/ext/flexary39.C: New test.
> 	* g++.dg/ext/flexary40.C: New test.
> 
> --- gcc/varasm.c.jj	2021-05-10 12:22:30.437451816 +0200
> +++ gcc/varasm.c	2021-09-15 13:19:02.841554574 +0200
> @@ -5531,14 +5531,15 @@ output_constructor_regular_field (oc_loc
>   	  && (!TYPE_DOMAIN (TREE_TYPE (local->field))
>   	      || !TYPE_MAX_VALUE (TYPE_DOMAIN (TREE_TYPE (local->field)))))
>   	{
> -	  fieldsize = array_size_for_constructor (local->val);
> +	  unsigned HOST_WIDE_INT fldsize
> +	    = array_size_for_constructor (local->val);
> +	  fieldsize = int_size_in_bytes (TREE_TYPE (local->val));
> +	  gcc_checking_assert (fieldsize >= fldsize);

This assert needs a comment about when they can be unequal.  OK with 
that change.

>   	  /* Given a non-empty initialization, this field had better
>   	     be last.  Given a flexible array member, the next field
>   	     on the chain is a TYPE_DECL of the enclosing struct.  */
>   	  const_tree next = DECL_CHAIN (local->field);
>   	  gcc_assert (!fieldsize || !next || TREE_CODE (next) != FIELD_DECL);
> -	  tree size = TYPE_SIZE_UNIT (TREE_TYPE (local->val));
> -	  gcc_checking_assert (compare_tree_int (size, fieldsize) == 0);
>   	}
>         else
>   	fieldsize = tree_to_uhwi (DECL_SIZE_UNIT (local->field));
> --- gcc/cp/typeck2.c.jj	2021-09-01 21:30:30.520306823 +0200
> +++ gcc/cp/typeck2.c	2021-09-15 14:15:59.049388381 +0200
> @@ -524,7 +524,20 @@ split_nonconstant_init_1 (tree dest, tre
>   		sub = build3 (COMPONENT_REF, inner_type, dest, field_index,
>   			      NULL_TREE);
>   
> -	      if (!split_nonconstant_init_1 (sub, value, true))
> +	      if (!split_nonconstant_init_1 (sub, value, true)
> +		      /* For flexible array member with initializer we
> +			 can't remove the initializer, because only the
> +			 initializer determines how many elements the
> +			 flexible array member has.  */
> +		  || (!array_type_p
> +		      && TREE_CODE (inner_type) == ARRAY_TYPE
> +		      && TYPE_DOMAIN (inner_type) == NULL
> +		      && TREE_CODE (TREE_TYPE (value)) == ARRAY_TYPE
> +		      && COMPLETE_TYPE_P (TREE_TYPE (value))
> +		      && !integer_zerop (TYPE_SIZE (TREE_TYPE (value)))
> +		      && idx == CONSTRUCTOR_NELTS (init) - 1
> +		      && TYPE_HAS_TRIVIAL_DESTRUCTOR
> +				(strip_array_types (inner_type))))
>   		complete_p = false;
>   	      else
>   		{
> --- gcc/testsuite/g++.dg/ext/flexary39.C.jj	2021-09-15 14:01:33.811320756 +0200
> +++ gcc/testsuite/g++.dg/ext/flexary39.C	2021-09-15 14:04:05.962221748 +0200
> @@ -0,0 +1,65 @@
> +// PR c++/88578
> +// { dg-do run }
> +// { dg-options -Wno-pedantic }
> +
> +#define STR(s) #s
> +#define ASSERT(exp) \
> +  ((exp) ? (void)0 : (void)(__builtin_printf ("%s:%i: assertion %s failed\n", \
> +                     __FILE__, __LINE__, STR(exp)), \
> +                      __builtin_abort ()))
> +
> +typedef int int32_t __attribute__((mode (__SI__)));
> +
> +struct Ax { int32_t n, a[]; };
> +struct AAx { int32_t i; Ax ax; };
> +
> +int32_t i = 12345678;
> +
> +void
> +test ()
> +{
> +  {
> +    // OK.  Does not assign any elements to flexible array.
> +    Ax s = { 0 };
> +    ASSERT (s.n == 0);
> +  }
> +  {
> +    // OK only for statically allocated objects, otherwise error.
> +    static Ax s = { 0, { } };
> +    ASSERT (s.n == 0);
> +  }
> +  {
> +    static Ax s = { 1, { 2 } };
> +    ASSERT (s.n == 1 && s.a [0] == 2);
> +  }
> +  {
> +    static Ax s = { 2, { 3, 4 } };
> +    ASSERT (s.n = 2 && s.a [0] == 3 && s.a [1] == 4);
> +  }
> +  {
> +    static Ax s = { 123, i };
> +    ASSERT (s.n == 123 && s.a [0] == i);
> +  }
> +  {
> +    static Ax s = { 456, { i } };
> +    ASSERT (s.n == 456 && s.a [0] == i);
> +  }
> +  {
> +    int32_t j = i + 1, k = j + 1;
> +    static Ax s = { 3, { i, j, k } };
> +    ASSERT (s.n == 3 && s.a [0] == i && s.a [1] == j && s.a [2] == k);
> +  }
> +
> +  {
> +    // OK.  Does not assign any elements to flexible array.
> +    AAx s = { 1, { 2 } };
> +    ASSERT (s.i == 1 && s.ax.n == 2);
> +  }
> +}
> +
> +int
> +main ()
> +{
> +  test ();
> +  test ();
> +}
> --- gcc/testsuite/g++.dg/ext/flexary40.C.jj	2021-09-15 14:03:52.328409836 +0200
> +++ gcc/testsuite/g++.dg/ext/flexary40.C	2021-09-15 13:58:40.888706320 +0200
> @@ -0,0 +1,50 @@
> +// PR c++/102295
> +// { dg-do run }
> +// { dg-options "" }
> +
> +struct A { int a; int b[]; };
> +struct B { B (); int k; };
> +struct C { int l; B m[]; };
> +
> +int x[4];
> +A c = { 42, { ++x[0], ++x[1], ++x[2], ++x[3] } };
> +A d = { 43, { 0, ++x[0], ++x[1], ++x[2], ++x[3] } };
> +A e = { 44, { ++x[0], ++x[1], ++x[2], 17 } };
> +A f = { 45 };
> +C n = { 50, { B (), B () } };
> +C o = { 51, {} };
> +
> +int
> +main ()
> +{
> +  static A g = { 46, { ++x[0], ++x[1], ++x[2], ++x[3] } };
> +  static A h = { 47, { 0, ++x[0], ++x[1], ++x[2], ++x[3] } };
> +  static A i = { 48, { ++x[0], ++x[1], ++x[2], 18 } };
> +  static A j = { 49 };
> +  if (c.a != 42 || c.b[0] != 1 || c.b[1] != 1 || c.b[2] != 1 || c.b[3] != 1)
> +    __builtin_abort ();
> +  if (d.a != 43 || d.b[0] != 0 || d.b[1] != 2 || d.b[2] != 2 || d.b[3] != 2 || d.b[4] != 2)
> +    __builtin_abort ();
> +  if (e.a != 44 || e.b[0] != 3 || e.b[1] != 3 || e.b[2] != 3 || e.b[3] != 17)
> +    __builtin_abort ();
> +  if (f.a != 45)
> +    __builtin_abort ();
> +  if (g.a != 46 || g.b[0] != 4 || g.b[1] != 4 || g.b[2] != 4 || g.b[3] != 3)
> +    __builtin_abort ();
> +  if (h.a != 47 || h.b[0] != 0 || h.b[1] != 5 || h.b[2] != 5 || h.b[3] != 5 || h.b[4] != 4)
> +    __builtin_abort ();
> +  if (i.a != 48 || i.b[0] != 6 || i.b[1] != 6 || i.b[2] != 6 || i.b[3] != 18)
> +    __builtin_abort ();
> +  if (j.a != 49)
> +    __builtin_abort ();
> +  if (n.l != 50 || n.m[0].k != 42 || n.m[1].k != 42)
> +    __builtin_abort ();
> +  if (o.l != 51)
> +    __builtin_abort ();
> +  if (x[0] != 6 || x[1] != 6 || x[2] != 6 || x[3] != 4)
> +    __builtin_abort ();
> +}
> +
> +B::B () : k (42)
> +{
> +}
> 
> 
> 	Jakub
>