| Message ID | AM5PR0701MB265787FC87F141E9DBC95940E4360@AM5PR0701MB2657.eurprd07.prod.outlook.com |
|---|---|
| State | New |
| Headers | show |
| Series | Use complete_array_type on flexible array member initializers | expand |
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
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.
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
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
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.
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
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. 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));
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
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.
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
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
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
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.
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.
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.
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));