From: Jason Merrill <jason@redhat.com>
To: Bernd Edlinger <bernd.edlinger@hotmail.de>
Cc: Jeff Law <law@redhat.com>,
"gcc-patches@gcc.gnu.org" <gcc-patches@gcc.gnu.org>,
Richard Biener <rguenther@suse.de>,
Joseph Myers <joseph@codesourcery.com>,
Nathan Sidwell <nathan@acm.org>
Subject: Re: [PATCH] Use complete_array_type on flexible array member initializers
Date: Wed, 29 Aug 2018 21:11:00 -0000 [thread overview]
Message-ID: <CADzB+2mpjzUGrJkB58uE0-a4YLvgbBT544HT9+aptf9azRUWGQ@mail.gmail.com> (raw)
In-Reply-To: <AM5PR0701MB265773485617B9A2AA2324EFE4340@AM5PR0701MB2657.eurprd07.prod.outlook.com>
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
next prev parent reply other threads:[~2018-08-29 21:11 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-08-24 13:13 Bernd Edlinger
2018-08-26 5:36 ` Jeff Law
2018-08-26 8:52 ` Bernd Edlinger
2018-08-29 21:11 ` Jason Merrill [this message]
2018-09-03 3:44 ` Jeff Law
2018-09-03 12:35 ` Bernd Edlinger
2018-09-04 14:31 ` Jeff Law
2018-09-06 11:05 ` Bernd Edlinger
2018-09-06 15:44 ` Jeff Law
2018-09-06 17:12 ` Bernd Edlinger
2018-09-06 22:01 ` Jeff Law
2018-09-06 22:16 ` Jeff Law
2018-09-06 22:26 ` Jeff Law
2018-09-07 6:51 ` Bernd Edlinger
2018-09-07 13:36 ` Bernd Edlinger
2018-09-07 17:44 ` Bernd Edlinger
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CADzB+2mpjzUGrJkB58uE0-a4YLvgbBT544HT9+aptf9azRUWGQ@mail.gmail.com \
--to=jason@redhat.com \
--cc=bernd.edlinger@hotmail.de \
--cc=gcc-patches@gcc.gnu.org \
--cc=joseph@codesourcery.com \
--cc=law@redhat.com \
--cc=nathan@acm.org \
--cc=rguenther@suse.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).