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.edlin...@hotmail.de>
>>>
>>>> * varasm.c (output_constructor_regular_field): Check TYPE_SIZE_UNIT of
>>>> the init value.
>>>>
>>> c-family:
>>>> 2018-08-24 Bernd Edlinger <bernd.edlin...@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.edlin...@hotmail.de>
>>>>
>>>> * c-decl.c (finish_decl): Call complete_flexible_array_elts.
>>>>
>>>> cp:
>>>> 2018-08-24 Bernd Edlinger <bernd.edlin...@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.
