[PATCH] Use complete_array_type on flexible array member initializers

[PATCH] Use complete_array_type on flexible array member initializers

[Bernd Edlinger]

[-- Attachment #1: Type: text/plain, Size: 360 bytes --]

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.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: patch-flexarray.diff --]
[-- Type: text/x-patch; name="patch-flexarray.diff", Size: 3751 bytes --]

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);
+}
+
 /* Like c_mark_addressable but don't check register qualifier.  */
 void 
 c_common_mark_addressable_vec (tree t)
diff -Npur gcc/c-family/c-common.h gcc/c-family/c-common.h
--- gcc/c-family/c-common.h	2018-08-17 05:02:11.000000000 +0200
+++ gcc/c-family/c-common.h	2018-08-24 12:06:21.375892280 +0200
@@ -1038,6 +1038,7 @@ extern tree fold_offsetof (tree, tree =
 			   tree_code ctx = ERROR_MARK);
 
 extern int complete_array_type (tree *, tree, bool);
+extern void complete_flexible_array_elts (tree);
 
 extern tree builtin_type_for_size (int, bool);
 
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
diff -Npur gcc/varasm.c gcc/varasm.c
--- gcc/varasm.c	2018-08-16 17:28:11.000000000 +0200
+++ gcc/varasm.c	2018-08-24 12:06:21.378892238 +0200
@@ -5161,6 +5161,8 @@ output_constructor_regular_field (oc_loc
 	     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));

Re: [PATCH] Use complete_array_type on flexible array member initializers

[Jeff Law]

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?


> 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?


Jeff

Re: [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.

Re: [PATCH] Use complete_array_type on flexible array member initializers

[Jason Merrill]

On Sun, Aug 26, 2018 at 4:52 AM, Bernd Edlinger
<bernd.edlinger@hotmail.de> wrote:
> 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.

That check only returns early on CONSTRUCTORs for a non-array type,
which I assume you don't care about.

> 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" };
>     |                     ^

The extra braces that code handles are like

struct A
{
  int i;
  char a[];
};

struct A a = { 1, { "test" } };

...but by the time you're calling complete_flexible_array_elts those
extra braces have already been removed by reshape_init.

The flag copying at the end of cp_complete_array_type is likely to be
important, though, so let's use it.

Jason

Re: [PATCH] Use complete_array_type on flexible array member initializers

[Jeff Law]

On 08/26/2018 02:52 AM, Bernd Edlinger wrote:
> 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.
Huh?  We get into add_flexible_array_elts_to_size just fine.  Using your
testcase above:

Breakpoint 3, add_flexible_array_elts_to_size (decl=0x7ffff7ff6ab0,
init=0x7fffe9f3edc8) at /home/gcc/GIT-3/gcc/gcc/c/c-decl.c:4617
4617      if (vec_safe_is_empty (CONSTRUCTOR_ELTS (init)))
(gdb) p debug_tree (decl)
 <var_decl 0x7ffff7ff6ab0 u
    type <record_type 0x7fffe9f35498 type_0 BLK
        size <integer_cst 0x7fffe9e2c000 constant 256>
        unit-size <integer_cst 0x7fffe9e2c0f0 constant 32>
        align:32 warn_if_not_align:0 symtab:0 alias-set -1
canonical-type 0x7fffe9f35498
        fields <field_decl 0x7fffe9e357b8 D.1914 type <union_type
0x7fffe9f35540>
            BLK j.c:10:4 size <integer_cst 0x7fffe9e2c000 256> unit-size
<integer_cst 0x7fffe9e2c0f0 32>
            align:32 warn_if_not_align:0 offset_align 128
            offset <integer_cst 0x7fffe9e0dcc0 constant 0>
            bit-offset <integer_cst 0x7fffe9e0dd08 constant 0> context
<record_type 0x7fffe9f35498>>
        chain <type_decl 0x7fffe9e35260 D.1905>>
    public static BLK j.c:11:3 size <integer_cst 0x7fffe9e2c000 256>
unit-size <integer_cst 0x7fffe9e2c0f0 32>
    align:32 warn_if_not_align:0 initial <constructor 0x7fffe9f3edc8>>



So I'm a bit confused.  It seems to go through
add_flexible_array_elts_to_size in my limited testing.


Given how closely complete_flexible_array_elts is to
add_flexible_array_elts_to_size I can't help but continue to wonder if
we're really papering over a problem in add_flexible_array_elts_to_size.

But it may also be the case that the recursive needs to handle the
CONSTRUCTORS embedded within other CONSTRUCTORS mean we want two routines.

Basically I want to see a rationale why we want/need two routines here.


> 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.
Looks wrong to me as well.   But I don't think that's the core issue
here.  Let's tackle that separately.


> 
> 
>>
>>> 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:
[ ... ]
Jason's the expert here.    I'll defer to his expertise.  It just seemed
a bit odd to me that we have a routine to "complete" an array that does
any special C++ handling (cp_complete_array_type) and we're not using it.

Jeff

Re: [PATCH] Use complete_array_type on flexible array member initializers

[Bernd Edlinger]

On 09/03/2018 05:43, Jeff Law wrote:
> On 08/26/2018 02:52 AM, Bernd Edlinger wrote:
>> 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.
> Huh?  We get into add_flexible_array_elts_to_size just fine.  Using your
> testcase above:

Yes, I was not clear enough here.
It is called, but it does only handle the case where the flexible array
is the last element of a CONSTRUCTOR, but not the case here,
where the flexible array is the last element of a CONSTRUCTOR within
another CONSTRUCTOR.

> Breakpoint 3, add_flexible_array_elts_to_size (decl=0x7ffff7ff6ab0,
> init=0x7fffe9f3edc8) at /home/gcc/GIT-3/gcc/gcc/c/c-decl.c:4617
> 4617      if (vec_safe_is_empty (CONSTRUCTOR_ELTS (init)))
> (gdb) p debug_tree (decl)
>  <var_decl 0x7ffff7ff6ab0 u
>     type <record_type 0x7fffe9f35498 type_0 BLK
>         size <integer_cst 0x7fffe9e2c000 constant 256>
>         unit-size <integer_cst 0x7fffe9e2c0f0 constant 32>
>         align:32 warn_if_not_align:0 symtab:0 alias-set -1
> canonical-type 0x7fffe9f35498
>         fields <field_decl 0x7fffe9e357b8 D.1914 type <union_type
> 0x7fffe9f35540>
>             BLK j.c:10:4 size <integer_cst 0x7fffe9e2c000 256> unit-size
> <integer_cst 0x7fffe9e2c0f0 32>
>             align:32 warn_if_not_align:0 offset_align 128
>             offset <integer_cst 0x7fffe9e0dcc0 constant 0>
>             bit-offset <integer_cst 0x7fffe9e0dd08 constant 0> context
> <record_type 0x7fffe9f35498>>
>         chain <type_decl 0x7fffe9e35260 D.1905>>
>     public static BLK j.c:11:3 size <integer_cst 0x7fffe9e2c000 256>
> unit-size <integer_cst 0x7fffe9e2c0f0 32>
>     align:32 warn_if_not_align:0 initial <constructor 0x7fffe9f3edc8>>
>
> 
> 
> So I'm a bit confused.  It seems to go through
> add_flexible_array_elts_to_size in my limited testing.
> 
> 
> Given how closely complete_flexible_array_elts is to
> add_flexible_array_elts_to_size I can't help but continue to wonder if
> we're really papering over a problem in add_flexible_array_elts_to_size.
> 
> But it may also be the case that the recursive needs to handle the
> CONSTRUCTORS embedded within other CONSTRUCTORS mean we want two routines.
> 
> Basically I want to see a rationale why we want/need two routines here.
> 

My major concern here is this:
By making the array type complete we destroy the information, that
the array type was incomplete, which may be used somewhere else
in the front-end, therefore I want to do that as late as possible.

> 
>> 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.
> Looks wrong to me as well.   But I don't think that's the core issue
> here.  Let's tackle that separately.
>
>
>>
>>
>>
>>>> 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:
> [ ... ]
> Jason's the expert here.    I'll defer to his expertise.  It just seemed
> a bit odd to me that we have a routine to "complete" an array that does
> any special C++ handling (cp_complete_array_type) and we're not using it.
>

Agreed, I have posted an update which uses cp_complete_array_type, since
Jason raised the same point.

But since C++ does not need the recursion here, things are a lot more easy.
The patch still completes the array type _after_ the FE is completely done with the
parsing, since I want to avoid to destroy the information too early, since the FE is looking
at the TYPE_DOMAIN==NULL at various places.


Bernd.

Re: [PATCH] Use complete_array_type on flexible array member initializers

[Jeff Law]

On 09/03/2018 06:35 AM, Bernd Edlinger wrote:
[ Big snip, dropping lots of context ]

>>>>
>>>
>>> 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:
>> [ ... ]
>> Jason's the expert here.    I'll defer to his expertise.  It just seemed
>> a bit odd to me that we have a routine to "complete" an array that does
>> any special C++ handling (cp_complete_array_type) and we're not using it.
>>
> 
> Agreed, I have posted an update which uses cp_complete_array_type, since
> Jason raised the same point.
> 
> But since C++ does not need the recursion here, things are a lot more easy.
> The patch still completes the array type _after_ the FE is completely done with the
> parsing, since I want to avoid to destroy the information too early, since the FE is looking
> at the TYPE_DOMAIN==NULL at various places.
FYI, I don't see a follow-up for this patch?  Did I miss it?  Or is it
under a different subject line?

jeff

Re: [PATCH] Use complete_array_type on flexible array member initializers

[Bernd Edlinger]

[-- Attachment #1: Type: text/plain, Size: 1447 bytes --]

On 09/04/18 16:30, Jeff Law wrote:
> On 09/03/2018 06:35 AM, Bernd Edlinger wrote:
> [ Big snip, dropping lots of context ]
> 
>>>>>
>>>>
>>>> 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:
>>> [ ... ]
>>> Jason's the expert here.    I'll defer to his expertise.  It just seemed
>>> a bit odd to me that we have a routine to "complete" an array that does
>>> any special C++ handling (cp_complete_array_type) and we're not using it.
>>>
>>
>> Agreed, I have posted an update which uses cp_complete_array_type, since
>> Jason raised the same point.
>>
>> But since C++ does not need the recursion here, things are a lot more easy.
>> The patch still completes the array type _after_ the FE is completely done with the
>> parsing, since I want to avoid to destroy the information too early, since the FE is looking
>> at the TYPE_DOMAIN==NULL at various places.
> FYI, I don't see a follow-up for this patch?  Did I miss it?  Or is it
> under a different subject line?
> 
> jeff
> 

Yes, thanks for reminding me.
I had forgotten to post the updated patch.

So here is my latest version of the flexible array patch.

Bootstrapped and reg-tested on x86_64-pc-linux-gnu (together with the other STRING_CST-v2 patches).
Is it OK for trunk?


Thanks
Bernd.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: patch-flexarray.diff --]
[-- Type: text/x-patch; name="patch-flexarray.diff", Size: 4114 bytes --]

gcc:
2018-08-30  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	* varasm.c (output_constructor_regular_field): Check TYPE_SIZE_UNIT of
	the init value.

c-family:
2018-08-30  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-30  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	* c-decl.c (finish_decl): Call complete_flexible_array_elts.

cp:
2018-08-30  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);
+}
+
 /* Like c_mark_addressable but don't check register qualifier.  */
 void 
 c_common_mark_addressable_vec (tree t)
diff -Npur gcc/c-family/c-common.h gcc/c-family/c-common.h
--- gcc/c-family/c-common.h	2018-08-17 05:02:11.000000000 +0200
+++ gcc/c-family/c-common.h	2018-08-24 12:06:21.375892280 +0200
@@ -1038,6 +1038,7 @@ extern tree fold_offsetof (tree, tree =
 			   tree_code ctx = ERROR_MARK);
 
 extern int complete_array_type (tree *, tree, bool);
+extern void complete_flexible_array_elts (tree);
 
 extern tree builtin_type_for_size (int, bool);
 
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-30 12:06:21.377892252 +0200
@@ -6530,6 +6530,16 @@ check_initializer (tree decl, tree init,
 
 	  init_code = store_init_value (decl, init, cleanups, flags);
 
+	  if (DECL_INITIAL (decl)
+	      && TREE_CODE (DECL_INITIAL (decl)) == CONSTRUCTOR
+	      && !vec_safe_is_empty (CONSTRUCTOR_ELTS (DECL_INITIAL (decl))))
+	    {
+	      tree elt = CONSTRUCTOR_ELTS (DECL_INITIAL (decl))->last ().value;
+	      if (TREE_CODE (TREE_TYPE (elt)) == ARRAY_TYPE
+		  && TYPE_SIZE (TREE_TYPE (elt)) == NULL_TREE)
+		cp_complete_array_type (&TREE_TYPE (elt), elt, false);
+	    }
+
 	  if (pedantic && TREE_CODE (type) == ARRAY_TYPE
 	      && DECL_INITIAL (decl)
 	      && TREE_CODE (DECL_INITIAL (decl)) == STRING_CST
diff -Npur gcc/varasm.c gcc/varasm.c
--- gcc/varasm.c	2018-08-16 17:28:11.000000000 +0200
+++ gcc/varasm.c	2018-08-24 12:06:21.378892238 +0200
@@ -5161,6 +5161,8 @@ output_constructor_regular_field (oc_loc
 	     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));

Re: [PATCH] Use complete_array_type on flexible array member initializers

[Jeff Law]

On 09/06/2018 05:05 AM, Bernd Edlinger wrote:
> On 09/04/18 16:30, Jeff Law wrote:
>> On 09/03/2018 06:35 AM, Bernd Edlinger wrote:
>> [ Big snip, dropping lots of context ]
>>
>>>>> 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:
>>>> [ ... ]
>>>> Jason's the expert here.    I'll defer to his expertise.  It just seemed
>>>> a bit odd to me that we have a routine to "complete" an array that does
>>>> any special C++ handling (cp_complete_array_type) and we're not using it.
>>>>
>>> Agreed, I have posted an update which uses cp_complete_array_type, since
>>> Jason raised the same point.
>>>
>>> But since C++ does not need the recursion here, things are a lot more easy.
>>> The patch still completes the array type _after_ the FE is completely done with the
>>> parsing, since I want to avoid to destroy the information too early, since the FE is looking
>>> at the TYPE_DOMAIN==NULL at various places.
>> FYI, I don't see a follow-up for this patch?  Did I miss it?  Or is it
>> under a different subject line?
>>
>> jeff
>>
> Yes, thanks for reminding me.
> I had forgotten to post the updated patch.
> 
> So here is my latest version of the flexible array patch.
> 
> Bootstrapped and reg-tested on x86_64-pc-linux-gnu (together with the other STRING_CST-v2 patches).
> Is it OK for trunk?
> 
> 
> Thanks
> Bernd.
> 
> 
> patch-flexarray.diff
> 
> 
> gcc:
> 2018-08-30  Bernd Edlinger  <bernd.edlinger@hotmail.de>
> 
> 	* varasm.c (output_constructor_regular_field): Check TYPE_SIZE_UNIT of
> 	the init value.
> 
> c-family:
> 2018-08-30  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-30  Bernd Edlinger  <bernd.edlinger@hotmail.de>
> 
> 	* c-decl.c (finish_decl): Call complete_flexible_array_elts.
> 
> cp:
> 2018-08-30  Bernd Edlinger  <bernd.edlinger@hotmail.de>
> 
> 	* decl.c (check_initializer): Call complete_flexible_array_elts.
Thanks.  I fixed up the ChangeLog entry for the cp/ change and installed
this patch.

As you've probably noted, I'm taking care of the installs on this stuff.
 I'm doing additional testing and as a result I've got a tree with your
proposed patch "push ready".  It'd be silly to not go ahead with the
push :-)



Jeff

Re: [PATCH] Use complete_array_type on flexible array member initializers

[Bernd Edlinger]

On 09/06/18 17:43, Jeff Law wrote:
> On 09/06/2018 05:05 AM, Bernd Edlinger wrote:
>> On 09/04/18 16:30, Jeff Law wrote:
>>> On 09/03/2018 06:35 AM, Bernd Edlinger wrote:
>>> [ Big snip, dropping lots of context ]
>>>
>>>>>> 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:
>>>>> [ ... ]
>>>>> Jason's the expert here.    I'll defer to his expertise.  It just seemed
>>>>> a bit odd to me that we have a routine to "complete" an array that does
>>>>> any special C++ handling (cp_complete_array_type) and we're not using it.
>>>>>
>>>> Agreed, I have posted an update which uses cp_complete_array_type, since
>>>> Jason raised the same point.
>>>>
>>>> But since C++ does not need the recursion here, things are a lot more easy.
>>>> The patch still completes the array type _after_ the FE is completely done with the
>>>> parsing, since I want to avoid to destroy the information too early, since the FE is looking
>>>> at the TYPE_DOMAIN==NULL at various places.
>>> FYI, I don't see a follow-up for this patch?  Did I miss it?  Or is it
>>> under a different subject line?
>>>
>>> jeff
>>>
>> Yes, thanks for reminding me.
>> I had forgotten to post the updated patch.
>>
>> So here is my latest version of the flexible array patch.
>>
>> Bootstrapped and reg-tested on x86_64-pc-linux-gnu (together with the other STRING_CST-v2 patches).
>> Is it OK for trunk?
>>
>>
>> Thanks
>> Bernd.
>>
>>
>> patch-flexarray.diff
>>
>>
>> gcc:
>> 2018-08-30  Bernd Edlinger  <bernd.edlinger@hotmail.de>
>>
>> 	* varasm.c (output_constructor_regular_field): Check TYPE_SIZE_UNIT of
>> 	the init value.
>>
>> c-family:
>> 2018-08-30  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-30  Bernd Edlinger  <bernd.edlinger@hotmail.de>
>>
>> 	* c-decl.c (finish_decl): Call complete_flexible_array_elts.
>>
>> cp:
>> 2018-08-30  Bernd Edlinger  <bernd.edlinger@hotmail.de>
>>
>> 	* decl.c (check_initializer): Call complete_flexible_array_elts.
> Thanks.  I fixed up the ChangeLog entry for the cp/ change and installed
> this patch.
> 
> As you've probably noted, I'm taking care of the installs on this stuff.
>   I'm doing additional testing and as a result I've got a tree with your
> proposed patch "push ready".  It'd be silly to not go ahead with the
> push :-)
> 

Ah, thanks a lot.

Okay, this is the status of the STRING-CST semantic-v2 patches:

[PATCH] Check the STRING_CSTs in varasm.c
https://gcc.gnu.org/ml/gcc-patches/2018-08/msg01569.html
=> Unfortunately I forgot to change the Title to [PATCHv2] or so.
Should I send a ping for this one?

[PATCHv2] Handle overlength strings in the C FE
https://gcc.gnu.org/ml/gcc-patches/2018-08/msg01566.html
=> Should I send a ping for this one?

[PATCHv2] Handle overlength strings in C++ FE
https://gcc.gnu.org/ml/gcc-patches/2018-08/msg01567.html
=> Apporoved, without the part in vtable-class-hierarchy.c (!)

[PATCHv2] Handle overlength string literals in the fortan FE
https://gcc.gnu.org/ml/gcc-patches/2018-08/msg01568.html
=> Approved.



Additional patches in the same area:

[PATCH] Fix not properly nul-terminated string constants in JIT
https://gcc.gnu.org/ml/gcc-patches/2018-08/msg00370.html

=> This triggered an assertion in the V1-patch only, but is no
pre-condition in the V2-semantic patch.  Anyway would be good
to get it in, since strings longer than 200 bytes will not be
zero-terminated.
  
I have a nice follow-up patch that allows non-zero terminated
strings (mostly found in Ada and GO) to go into the string merge
section.

I did not post it yet to avoid confusion, but if you are
interested to see it, I'll go ahead and post it.

Regarding the nonstr parameter in string_constant,
I still do have the impression that it would be more natural
for string_constant _not_ to care about zero-termination, but instead
move that check to c_strlen and c_getstr or similar high level
functions, where for instance also a location value would be
available.

I could imagine something like an optional out-parameter with
detailed error info

struct nonstr
{
   enum { no_error, char_type_mismatch, missing_zero_termination } err;
   tree str;
   location_t loc;
   nonstr(): err(no_error), str(NULL_TREE), loc(UNKNOWN_LOCATION) {}
};

This information would be available in c_strlen here:

   if (eltsize != tree_to_uhwi (TYPE_SIZE_UNIT (TREE_TYPE (TREE_TYPE (src)))))
     return NULL_TREE;

and here:

   /* Don't know what to return if there was no zero termination.
      Ideally this would turn into a gcc_checking_assert over time.  */
   if (len > maxelts - eltoff)
     return NULL_TREE;



Thanks
Bernd.

Re: [PATCH] Use complete_array_type on flexible array member initializers

[Jeff Law]

On 09/06/2018 11:12 AM, Bernd Edlinger wrote:

>>
> 
> Ah, thanks a lot.
> 
> Okay, this is the status of the STRING-CST semantic-v2 patches:
> 
> [PATCH] Check the STRING_CSTs in varasm.c
> https://gcc.gnu.org/ml/gcc-patches/2018-08/msg01569.html
> => Unfortunately I forgot to change the Title to [PATCHv2] or so.
> Should I send a ping for this one?
> 
> [PATCHv2] Handle overlength strings in the C FE
> https://gcc.gnu.org/ml/gcc-patches/2018-08/msg01566.html
> => Should I send a ping for this one?
No need to ping.  I've got it here.  What's odd is that it's regressing
87053 .

Jeff

Re: [PATCH] Use complete_array_type on flexible array member initializers

[Jeff Law]

On 09/06/2018 04:01 PM, Jeff Law wrote:
> On 09/06/2018 11:12 AM, Bernd Edlinger wrote:
> 
>>>
>>
>> Ah, thanks a lot.
>>
>> Okay, this is the status of the STRING-CST semantic-v2 patches:
>>
>> [PATCH] Check the STRING_CSTs in varasm.c
>> https://gcc.gnu.org/ml/gcc-patches/2018-08/msg01569.html
>> => Unfortunately I forgot to change the Title to [PATCHv2] or so.
>> Should I send a ping for this one?
>>
>> [PATCHv2] Handle overlength strings in the C FE
>> https://gcc.gnu.org/ml/gcc-patches/2018-08/msg01566.html
>> => Should I send a ping for this one?
> No need to ping.  I've got it here.  What's odd is that it's regressing
> 87053 .
Which is probably a sign that we've got an incorrect test for NUL
termination somewhere.
jeff

Re: [PATCH] Use complete_array_type on flexible array member initializers

[Jeff Law]

On 09/06/2018 04:16 PM, Jeff Law wrote:
> On 09/06/2018 04:01 PM, Jeff Law wrote:
>> On 09/06/2018 11:12 AM, Bernd Edlinger wrote:
>>
>>>>
>>>
>>> Ah, thanks a lot.
>>>
>>> Okay, this is the status of the STRING-CST semantic-v2 patches:
>>>
>>> [PATCH] Check the STRING_CSTs in varasm.c
>>> https://gcc.gnu.org/ml/gcc-patches/2018-08/msg01569.html
>>> => Unfortunately I forgot to change the Title to [PATCHv2] or so.
>>> Should I send a ping for this one?
>>>
>>> [PATCHv2] Handle overlength strings in the C FE
>>> https://gcc.gnu.org/ml/gcc-patches/2018-08/msg01566.html
>>> => Should I send a ping for this one?
>> No need to ping.  I've got it here.  What's odd is that it's regressing
>> 87053 .
> Which is probably a sign that we've got an incorrect test for NUL
> termination somewhere.
I think I've found the issue.  I've got more testing to do, but looks
like a thinko on my part.

jeff

Re: [PATCH] Use complete_array_type on flexible array member initializers

[Bernd Edlinger]

On 09/07/18 00:26, Jeff Law wrote:
> On 09/06/2018 04:16 PM, Jeff Law wrote:
>> On 09/06/2018 04:01 PM, Jeff Law wrote:
>>> On 09/06/2018 11:12 AM, Bernd Edlinger wrote:
>>>
>>>>>
>>>>
>>>> Ah, thanks a lot.
>>>>
>>>> Okay, this is the status of the STRING-CST semantic-v2 patches:
>>>>
>>>> [PATCH] Check the STRING_CSTs in varasm.c
>>>> https://gcc.gnu.org/ml/gcc-patches/2018-08/msg01569.html
>>>> => Unfortunately I forgot to change the Title to [PATCHv2] or so.
>>>> Should I send a ping for this one?
>>>>
>>>> [PATCHv2] Handle overlength strings in the C FE
>>>> https://gcc.gnu.org/ml/gcc-patches/2018-08/msg01566.html
>>>> => Should I send a ping for this one?
>>> No need to ping.  I've got it here.  What's odd is that it's regressing
>>> 87053 .
>> Which is probably a sign that we've got an incorrect test for NUL
>> termination somewhere.

It may be a sign that we should first fix the low level functions
before the high level stuff.

> I think I've found the issue.  I've got more testing to do, but looks
> like a thinko on my part.
> 

Ah, I forgot, the regression on pr87053 and fortran.dg/pr45636.f90
is fixed by this patch:

[PATCH] Adjust c_getstr/c_strlen to new STRING_CST semantic
https://gcc.gnu.org/ml/gcc-patches/2018-08/msg02013.html

This is a new regression since the patch was initially posted.


Bernd.

Re: [PATCH] Use complete_array_type on flexible array member initializers

[Bernd Edlinger]

On 09/07/18 08:51, Bernd Edlinger wrote:
> On 09/07/18 00:26, Jeff Law wrote:
>> On 09/06/2018 04:16 PM, Jeff Law wrote:
>>> On 09/06/2018 04:01 PM, Jeff Law wrote:
>>>> On 09/06/2018 11:12 AM, Bernd Edlinger wrote:
>>>>
>>>>>>
>>>>>
>>>>> Ah, thanks a lot.
>>>>>
>>>>> Okay, this is the status of the STRING-CST semantic-v2 patches:
>>>>>
>>>>> [PATCH] Check the STRING_CSTs in varasm.c
>>>>> https://gcc.gnu.org/ml/gcc-patches/2018-08/msg01569.html
>>>>> => Unfortunately I forgot to change the Title to [PATCHv2] or so.
>>>>> Should I send a ping for this one?
>>>>>
>>>>> [PATCHv2] Handle overlength strings in the C FE
>>>>> https://gcc.gnu.org/ml/gcc-patches/2018-08/msg01566.html
>>>>> => Should I send a ping for this one?
>>>> No need to ping.  I've got it here.  What's odd is that it's regressing
>>>> 87053 .
>>> Which is probably a sign that we've got an incorrect test for NUL
>>> termination somewhere.
> 
> It may be a sign that we should first fix the low level functions
> before the high level stuff.
> 
>> I think I've found the issue.  I've got more testing to do, but looks
>> like a thinko on my part.
>>
> 
> Ah, I forgot, the regression on pr87053 and fortran.dg/pr45636.f90
> is fixed by this patch:
> 
> [PATCH] Adjust c_getstr/c_strlen to new STRING_CST semantic
> https://gcc.gnu.org/ml/gcc-patches/2018-08/msg02013.html
> 
> This is a new regression since the patch was initially posted.
> 

Well, actually both patches seem to have a circular dependency.

If you want we can break this dependency by adding this to the  c_getstr patch:

--- gcc/fold-const.c	2018-09-07 14:22:50.047964775 +0200
+++ gcc/fold-const.c	2018-09-07 15:06:46.656989904 +0200
@@ -14611,6 +14611,10 @@ c_getstr (tree src, unsigned HOST_WIDE_I
    unsigned HOST_WIDE_INT string_length = TREE_STRING_LENGTH (src);
    unsigned HOST_WIDE_INT string_size = tree_to_uhwi (mem_size);
  
+  /* Ideally this would turn into a gcc_checking_assert over time.  */
+  if (string_length > string_size)
+    return NULL;
+
    const char *string = TREE_STRING_POINTER (src);
  
    if (string_length == 0


This should allow it to work with current semantics as well.


Bernd.

Re: [PATCH] Use complete_array_type on flexible array member initializers

[Bernd Edlinger]

On 09/07/18 15:36, Bernd Edlinger wrote:
> On 09/07/18 08:51, Bernd Edlinger wrote:
>> On 09/07/18 00:26, Jeff Law wrote:
>>> On 09/06/2018 04:16 PM, Jeff Law wrote:
>>>> On 09/06/2018 04:01 PM, Jeff Law wrote:
>>>>> On 09/06/2018 11:12 AM, Bernd Edlinger wrote:
>>>>>
>>>>>>>
>>>>>>
>>>>>> Ah, thanks a lot.
>>>>>>
>>>>>> Okay, this is the status of the STRING-CST semantic-v2 patches:
>>>>>>
>>>>>> [PATCH] Check the STRING_CSTs in varasm.c
>>>>>> https://gcc.gnu.org/ml/gcc-patches/2018-08/msg01569.html
>>>>>> => Unfortunately I forgot to change the Title to [PATCHv2] or so.
>>>>>> Should I send a ping for this one?
>>>>>>
>>>>>> [PATCHv2] Handle overlength strings in the C FE
>>>>>> https://gcc.gnu.org/ml/gcc-patches/2018-08/msg01566.html
>>>>>> => Should I send a ping for this one?
>>>>> No need to ping.  I've got it here.  What's odd is that it's regressing
>>>>> 87053 .
>>>> Which is probably a sign that we've got an incorrect test for NUL
>>>> termination somewhere.
>>
>> It may be a sign that we should first fix the low level functions
>> before the high level stuff.
>>
>>> I think I've found the issue.  I've got more testing to do, but looks
>>> like a thinko on my part.
>>>
>>
>> Ah, I forgot, the regression on pr87053 and fortran.dg/pr45636.f90
>> is fixed by this patch:
>>
>> [PATCH] Adjust c_getstr/c_strlen to new STRING_CST semantic
>> https://gcc.gnu.org/ml/gcc-patches/2018-08/msg02013.html
>>
>> This is a new regression since the patch was initially posted.
>>
> 
> Well, actually both patches seem to have a circular dependency.
> 
> If you want we can break this dependency by adding this to the  c_getstr patch:
> 
> --- gcc/fold-const.c    2018-09-07 14:22:50.047964775 +0200
> +++ gcc/fold-const.c    2018-09-07 15:06:46.656989904 +0200
> @@ -14611,6 +14611,10 @@ c_getstr (tree src, unsigned HOST_WIDE_I
>     unsigned HOST_WIDE_INT string_length = TREE_STRING_LENGTH (src);
>     unsigned HOST_WIDE_INT string_size = tree_to_uhwi (mem_size);
> 
> +  /* Ideally this would turn into a gcc_checking_assert over time.  */
> +  if (string_length > string_size)
> +    return NULL;
> +
>     const char *string = TREE_STRING_POINTER (src);
> 
>     if (string_length == 0
> 
> 
> This should allow it to work with current semantics as well.
> 

Oops, this does not work for strlenopt-49.c...

Please make it:

--- gcc/fold-const.c	2018-09-07 19:39:19.555588785 +0200
+++ gcc/fold-const.c	2018-09-07 19:30:03.372583484 +0200
@@ -14611,6 +14611,10 @@ c_getstr (tree src, unsigned HOST_WIDE_I
    unsigned HOST_WIDE_INT string_length = TREE_STRING_LENGTH (src);
    unsigned HOST_WIDE_INT string_size = tree_to_uhwi (mem_size);
  
+  /* Ideally this would turn into a gcc_checking_assert over time.  */
+  if (string_length > string_size)
+    string_length = string_size;
+
    const char *string = TREE_STRING_POINTER (src);
  
    if (string_length == 0



Bernd.