[PATCH] Use complete_array_type on flexible array member initializers

[Bernd Edlinger]

On 08/26/18 07:36, Jeff Law wrote:
> On 08/24/2018 07:13 AM, Bernd Edlinger wrote:
>> Hi!
>>
>>
>> This patch prevents init values of STRING_CST and braced
>> array initializers to reach the middle-end with incomplete
>> type.
>>
>> This will allow further simplifications in the middle-end,
>> and address existing issues with STRING_CST in a correct
>> way.
>>
>>
>>
>> Bootstrapped and reg-tested on x86_64-pc-linux-gnu.
>> Is it OK for trunk?
>>
>>
>> Thanks
>> Bernd.
>>
>>
>> patch-flexarray.diff
>>
>>
>> gcc:
>> 2018-08-24  Bernd Edlinger  <bernd.edlinger@hotmail.de>
>>
>> 	* varasm.c (output_constructor_regular_field): Check TYPE_SIZE_UNIT of
>> 	the init value.
>>
>> c-family:
>> 2018-08-24  Bernd Edlinger  <bernd.edlinger@hotmail.de>
>>
>> 	* c-common.c (complete_flexible_array_elts): New helper function.
>> 	* c-common.h (complete_flexible_array_elts): Declare.
>>
>> c:
>> 2018-08-24  Bernd Edlinger  <bernd.edlinger@hotmail.de>
>>
>> 	* c-decl.c (finish_decl): Call complete_flexible_array_elts.
>>
>> cp:
>> 2018-08-24  Bernd Edlinger  <bernd.edlinger@hotmail.de>
>>
>> 	* decl.c (check_initializer): Call complete_flexible_array_elts.
>>
>>
>> diff -Npur gcc/c/c-decl.c gcc/c/c-decl.c
>> --- gcc/c/c-decl.c	2018-08-21 08:17:41.000000000 +0200
>> +++ gcc/c/c-decl.c	2018-08-24 12:06:21.374892294 +0200
>> @@ -5035,6 +5035,8 @@ finish_decl (tree decl, location_t init_
>>         if (init && TREE_CODE (init) == CONSTRUCTOR)
>>   	add_flexible_array_elts_to_size (decl, init);
>>   
>> +      complete_flexible_array_elts (DECL_INITIAL (decl));
>> +
>>         if (DECL_SIZE (decl) == NULL_TREE && TREE_TYPE (decl) != error_mark_node
>>   	  && COMPLETE_TYPE_P (TREE_TYPE (decl)))
>>   	layout_decl (decl, 0);
>> diff -Npur gcc/c-family/c-common.c gcc/c-family/c-common.c
>> --- gcc/c-family/c-common.c	2018-08-17 05:02:11.000000000 +0200
>> +++ gcc/c-family/c-common.c	2018-08-24 12:45:56.559011703 +0200
>> @@ -6427,6 +6427,28 @@ complete_array_type (tree *ptype, tree i
>>     return failure;
>>   }
>>   
>> +/* INIT is an constructor of a structure with a flexible array member.
>> +   Complete the flexible array member with a domain based on it's value.  */
>> +void
>> +complete_flexible_array_elts (tree init)
>> +{
>> +  tree elt, type;
>> +
>> +  if (init == NULL_TREE || TREE_CODE (init) != CONSTRUCTOR)
>> +    return;
>> +
>> +  if (vec_safe_is_empty (CONSTRUCTOR_ELTS (init)))
>> +    return;
>> +
>> +  elt = CONSTRUCTOR_ELTS (init)->last ().value;
>> +  type = TREE_TYPE (elt);
>> +  if (TREE_CODE (type) == ARRAY_TYPE
>> +      && TYPE_SIZE (type) == NULL_TREE)
>> +    complete_array_type (&TREE_TYPE (elt), elt, false);
>> +  else
>> +    complete_flexible_array_elts (elt);
>> +}
> Shouldn't this be handled in c-decl.c by the call to
> add_flexible_array_elts_to_size?    Why the recursion when the
> CONSTRUCTOR_ELT isn't an array type?
> 

There are tests in the test suite that use something like that:
struct {
   union {
     struct {
        int a;
        char b[];
     };
     struct {
        char x[32];
     };
   };
} u = { { { 1, "test" } } };

So it fails to go through add_flexible_array_elts_to_size.

I am not sure what happens if the string is larger than 32 byte.
The test suite does not do that.
Well I just tried, while writing those lines:

struct {
  union {
   struct {
    int a;
    char b[];
   };
   struct {
    char x[4];
   };
  };
} u = { { { 1, "test" } } };

=>

	.file	"t.c"
	.text
	.globl	u
	.data
	.align 4
	.type	u, @object
	.size	u, 4
u:
	.long	1
	.string	"test"
	.ident	"GCC: (GNU) 9.0.0 20180825 (experimental)"
	.section	.note.GNU-stack,"",@progbits

So the .size is too small but that is probably only a fauxpas.


> 
>> diff -Npur gcc/cp/decl.c gcc/cp/decl.c
>> --- gcc/cp/decl.c	2018-08-22 22:35:38.000000000 +0200
>> +++ gcc/cp/decl.c	2018-08-24 12:06:21.377892252 +0200
>> @@ -6528,6 +6528,8 @@ check_initializer (tree decl, tree init,
>>   
>>   	  init_code = store_init_value (decl, init, cleanups, flags);
>>   
>> +	  complete_flexible_array_elts (DECL_INITIAL (decl));
>> +
>>   	  if (pedantic && TREE_CODE (type) == ARRAY_TYPE
>>   	      && DECL_INITIAL (decl)
>>   	      && TREE_CODE (DECL_INITIAL (decl)) == STRING_CST
> Should the C++ front-end be going through cp_complete_array_type instead?
> 

No I don't think so, because at that time BRACE_ENCLOSED_INITIALIZER_P
property does no longer work, as I explained in the previous mail.

And cp_complete_array_type does use that property:

int
cp_complete_array_type (tree *ptype, tree initial_value, bool do_default)
{
   int failure;
   tree type, elt_type;

   /* Don't get confused by a CONSTRUCTOR for some other type.  */
   if (initial_value && TREE_CODE (initial_value) == CONSTRUCTOR
       && !BRACE_ENCLOSED_INITIALIZER_P (initial_value)
       && TREE_CODE (TREE_TYPE (initial_value)) != ARRAY_TYPE)
     return 1;

But I need to do that completion for STRING_CST and CONSTRUCTORS
initializing a flexible array, of a structure of a union
within a structure.

I tried to come up with a test case where the cp_complete_array_type
might make a difference (looking into string constant with extra braces),
but it worked:


$ cat test.cc
struct {
   int a;
   char b[];
} xx = { 1,  { "test" } };

$ cat test.cc
struct {
  union {
   struct {
    int a;
    char b[];
   };
   struct {
     char c[32];
   };
  };
} xx = { 1,  "test" };
$ gcc test.cc
test.cc:11:21: error: initialization of flexible array member in a nested context
11 | } xx = { 1,  "test" };
    |                     ^

So since this recursive thing appears to be disallowed in C++ it would allow to use
cp_complete_array_type without the recursion.

But for consistency I would stay with complete_flexible_array_elts, even for C++.

However if you like it better, I am ready to change that hunk to use cp_complete_array_type.


Thanks
Bernd.