From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 92393 invoked by alias); 3 Sep 2018 12:35:13 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 92376 invoked by uid 89); 3 Sep 2018 12:35:13 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-11.2 required=5.0 tests=AWL,BAYES_00,FREEMAIL_FROM,GIT_PATCH_2,GIT_PATCH_3,KAM_MANYTO,RCVD_IN_DNSWL_NONE,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.2 spammy= X-HELO: EUR03-DB5-obe.outbound.protection.outlook.com Received: from mail-oln040092071092.outbound.protection.outlook.com (HELO EUR03-DB5-obe.outbound.protection.outlook.com) (40.92.71.92) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 03 Sep 2018 12:35:11 +0000 Received: from DB5EUR03FT048.eop-EUR03.prod.protection.outlook.com (10.152.20.54) by DB5EUR03HT189.eop-EUR03.prod.protection.outlook.com (10.152.20.218) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256_P256) id 15.20.1122.11; Mon, 3 Sep 2018 12:35:08 +0000 Received: from VI1PR0701MB2862.eurprd07.prod.outlook.com (10.152.20.60) by DB5EUR03FT048.mail.protection.outlook.com (10.152.21.28) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384_P256) id 15.20.1122.11 via Frontend Transport; Mon, 3 Sep 2018 12:35:07 +0000 Received: from VI1PR0701MB2862.eurprd07.prod.outlook.com ([fe80::854a:328:c580:5fb9]) by VI1PR0701MB2862.eurprd07.prod.outlook.com ([fe80::854a:328:c580:5fb9%3]) with mapi id 15.20.1122.009; Mon, 3 Sep 2018 12:35:07 +0000 From: Bernd Edlinger To: Jeff Law , "gcc-patches@gcc.gnu.org" , Richard Biener , Joseph Myers , Nathan Sidwell , Jason Merrill Subject: Re: [PATCH] Use complete_array_type on flexible array member initializers Date: Mon, 03 Sep 2018 12:35:00 -0000 Message-ID: References: ,<3c52e19a-c979-4beb-838b-06f11b1b356f@redhat.com> In-Reply-To: <3c52e19a-c979-4beb-838b-06f11b1b356f@redhat.com> received-spf: None (protection.outlook.com: hotmail.de does not designate permitted sender hosts) authentication-results: spf=none (sender IP is ) smtp.mailfrom=bernd.edlinger@hotmail.de; Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 X-SW-Source: 2018-09/txt/msg00105.txt.bz2 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 >>> >>>> * varasm.c (output_constructor_regular_field): Check TYPE_SIZE_UNI= T of >>>> the init value. >>>> >>> c-family: >>>> 2018-08-24 Bernd Edlinger >>>> >>>> * c-common.c (complete_flexible_array_elts): New helper function. >>>> * c-common.h (complete_flexible_array_elts): Declare. >>>> >>>> c: >>>> 2018-08-24 Bernd Edlinger >>>> >>>> * c-decl.c (finish_decl): Call complete_flexible_array_elts. >>>> >>>> cp: >>>> 2018-08-24 Bernd Edlinger >>>> >>>> * 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) =3D=3D CONSTRUCTOR) >>>> add_flexible_array_elts_to_size (decl, init); >>>> >>>> + complete_flexible_array_elts (DECL_INITIAL (decl)); >>>> + >>>> if (DECL_SIZE (decl) =3D=3D NULL_TREE && TREE_TYPE (decl) !=3D= 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 val= ue. */ >>>> +void >>>> +complete_flexible_array_elts (tree init) >>>> +{ >>>> + tree elt, type; >>>> + >>>> + if (init =3D=3D NULL_TREE || TREE_CODE (init) !=3D CONSTRUCTOR) >>>> + return; >>>> + >>>> + if (vec_safe_is_empty (CONSTRUCTOR_ELTS (init))) >>>> + return; >>>> + >>>> + elt =3D CONSTRUCTOR_ELTS (init)->last ().value; >>>> + type =3D TREE_TYPE (elt); >>>> + if (TREE_CODE (type) =3D=3D ARRAY_TYPE >>>> + && TYPE_SIZE (type) =3D=3D 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 =3D { { { 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=3D0x7ffff7ff6ab0, > init=3D0x7fffe9f3edc8) 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) > type size > unit-size > align:32 warn_if_not_align:0 symtab:0 alias-set -1 > canonical-type 0x7fffe9f35498 > fields 0x7fffe9f35540> > BLK j.c:10:4 size unit-size > > align:32 warn_if_not_align:0 offset_align 128 > offset > bit-offset context > > > chain > > public static BLK j.c:11:3 size > unit-size > align:32 warn_if_not_align:0 initial > > >=20 >=20 > So I'm a bit confused. It seems to go through > add_flexible_array_elts_to_size in my limited testing. >=20 >=20 > 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. >=20 > But it may also be the case that the recursive needs to handle the > CONSTRUCTORS embedded within other CONSTRUCTORS mean we want two routines. >=20 > Basically I want to see a rationale why we want/need two routines here. >=20 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. >=20 >> 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 =3D { { { 1, "test" } } }; >> >> =3D> >> >> .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 =3D store_init_value (decl, init, cleanups, flags); >>>> >>>> + complete_flexible_array_elts (DECL_INITIAL (decl)); >>>> + >>>> if (pedantic && TREE_CODE (type) =3D=3D ARRAY_TYPE >>>> && DECL_INITIAL (decl) >>>> && TREE_CODE (DECL_INITIAL (decl)) =3D=3D STRING_CST >>> Should the C++ front-end be going through cp_complete_array_type instea= d? >>> >> >> 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=3D=3DNULL at various places. Bernd.