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

[Jeff Law <law at redhat dot com>]

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