[PATCH v4] Fix ICE when mixing VLAs and statement expressions [PR91038]
Jason Merrill
jason@redhat.com
Tue Nov 16 19:53:11 GMT 2021
On 11/16/21 08:48, Uecker, Martin wrote:
> Am Montag, den 08.11.2021, 19:13 +0100 schrieb Martin Uecker:
>> Am Montag, den 08.11.2021, 12:13 -0500 schrieb Jason Merrill:
>>> On 11/7/21 01:40, Uecker, Martin wrote:
>>>> Am Mittwoch, den 03.11.2021, 10:18 -0400 schrieb Jason Merrill:
>>
>> ...
>>
>>>> Thank you! I made these changes and ran
>>>> bootstrap and tests again.
>>>
>>> Hmm, it doesn't look like you made the change to use the save_expr
>>> function instead of build1?
>>
>> Oh, sorry. I wanted to change it and then forgot.
>> Now also with this change (changelog as before).
>
>
> Ok, with is this change?
OK.
> Best,
> Martin
>
>
>
>>>> Ok for trunk?
>>>>
>>>>
>>>> Any idea how to fix returning structs with
>>>> VLA member from statement expressions?
>>>
>>> Testcase?
>>
>> void foo(void)
>> {
>> ({ int N = 3; struct { char x[N]; } x; x; });
>> }
>>
>> The difference to the tests in this patch (which
>> also forgot to include in the last version) is that
>> the object of variable size is returned from the
>> statement expression and not a pointer to it.
>> This can not happen with arrays because they decay
>> to pointers.
>>
>>
>> Martin
>>
>>
>>>> Otherwise, I will add an error message to
>>>> the FE in another patch.
>>>>
>>>> Martin
>>>>
>>
>> diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c
>> index 436df45df68..95083f95442 100644
>> --- a/gcc/c-family/c-common.c
>> +++ b/gcc/c-family/c-common.c
>> @@ -3306,7 +3306,19 @@ pointer_int_sum (location_t loc, enum tree_code resultcode,
>> TREE_TYPE (result_type)))
>> size_exp = integer_one_node;
>> else
>> - size_exp = size_in_bytes_loc (loc, TREE_TYPE (result_type));
>> + {
>> + size_exp = size_in_bytes_loc (loc, TREE_TYPE (result_type));
>> + /* Wrap the pointer expression in a SAVE_EXPR to make sure it
>> + is evaluated first when the size expression may depend
>> + on it for VM types. */
>> + if (TREE_SIDE_EFFECTS (size_exp)
>> + && TREE_SIDE_EFFECTS (ptrop)
>> + && variably_modified_type_p (TREE_TYPE (ptrop), NULL))
>> + {
>> + ptrop = save_expr (ptrop);
>> + size_exp = build2 (COMPOUND_EXPR, TREE_TYPE (intop), ptrop, size_exp);
>> + }
>> + }
>>
>> /* We are manipulating pointer values, so we don't need to warn
>> about relying on undefined signed overflow. We disable the
>> diff --git a/gcc/gimplify.c b/gcc/gimplify.c
>> index c2ab96e7e18..84f7dc3c248 100644
>> --- a/gcc/gimplify.c
>> +++ b/gcc/gimplify.c
>> @@ -2964,7 +2964,9 @@ gimplify_var_or_parm_decl (tree *expr_p)
>> declaration, for which we've already issued an error. It would
>> be really nice if the front end wouldn't leak these at all.
>> Currently the only known culprit is C++ destructors, as seen
>> - in g++.old-deja/g++.jason/binding.C. */
>> + in g++.old-deja/g++.jason/binding.C.
>> + Another possible culpit are size expressions for variably modified
>> + types which are lost in the FE or not gimplified correctly. */
>> if (VAR_P (decl)
>> && !DECL_SEEN_IN_BIND_EXPR_P (decl)
>> && !TREE_STATIC (decl) && !DECL_EXTERNAL (decl)
>> @@ -3109,16 +3111,22 @@ gimplify_compound_lval (tree *expr_p, gimple_seq *pre_p, gimple_seq
>> *post_p,
>> expression until we deal with any variable bounds, sizes, or
>> positions in order to deal with PLACEHOLDER_EXPRs.
>>
>> - So we do this in three steps. First we deal with the annotations
>> - for any variables in the components, then we gimplify the base,
>> - then we gimplify any indices, from left to right. */
>> + The base expression may contain a statement expression that
>> + has declarations used in size expressions, so has to be
>> + gimplified before gimplifying the size expressions.
>> +
>> + So we do this in three steps. First we deal with variable
>> + bounds, sizes, and positions, then we gimplify the base,
>> + then we deal with the annotations for any variables in the
>> + components and any indices, from left to right. */
>> +
>> for (i = expr_stack.length () - 1; i >= 0; i--)
>> {
>> tree t = expr_stack[i];
>>
>> if (TREE_CODE (t) == ARRAY_REF || TREE_CODE (t) == ARRAY_RANGE_REF)
>> {
>> - /* Gimplify the low bound and element type size and put them into
>> + /* Deal with the low bound and element type size and put them into
>> the ARRAY_REF. If these values are set, they have already been
>> gimplified. */
>> if (TREE_OPERAND (t, 2) == NULL_TREE)
>> @@ -3127,18 +3135,8 @@ gimplify_compound_lval (tree *expr_p, gimple_seq *pre_p, gimple_seq
>> *post_p,
>> if (!is_gimple_min_invariant (low))
>> {
>> TREE_OPERAND (t, 2) = low;
>> - tret = gimplify_expr (&TREE_OPERAND (t, 2), pre_p,
>> - post_p, is_gimple_reg,
>> - fb_rvalue);
>> - ret = MIN (ret, tret);
>> }
>> }
>> - else
>> - {
>> - tret = gimplify_expr (&TREE_OPERAND (t, 2), pre_p, post_p,
>> - is_gimple_reg, fb_rvalue);
>> - ret = MIN (ret, tret);
>> - }
>>
>> if (TREE_OPERAND (t, 3) == NULL_TREE)
>> {
>> @@ -3155,18 +3153,8 @@ gimplify_compound_lval (tree *expr_p, gimple_seq *pre_p, gimple_seq
>> *post_p,
>> elmt_size, factor);
>>
>> TREE_OPERAND (t, 3) = elmt_size;
>> - tret = gimplify_expr (&TREE_OPERAND (t, 3), pre_p,
>> - post_p, is_gimple_reg,
>> - fb_rvalue);
>> - ret = MIN (ret, tret);
>> }
>> }
>> - else
>> - {
>> - tret = gimplify_expr (&TREE_OPERAND (t, 3), pre_p, post_p,
>> - is_gimple_reg, fb_rvalue);
>> - ret = MIN (ret, tret);
>> - }
>> }
>> else if (TREE_CODE (t) == COMPONENT_REF)
>> {
>> @@ -3186,18 +3174,8 @@ gimplify_compound_lval (tree *expr_p, gimple_seq *pre_p, gimple_seq
>> *post_p,
>> offset, factor);
>>
>> TREE_OPERAND (t, 2) = offset;
>> - tret = gimplify_expr (&TREE_OPERAND (t, 2), pre_p,
>> - post_p, is_gimple_reg,
>> - fb_rvalue);
>> - ret = MIN (ret, tret);
>> }
>> }
>> - else
>> - {
>> - tret = gimplify_expr (&TREE_OPERAND (t, 2), pre_p, post_p,
>> - is_gimple_reg, fb_rvalue);
>> - ret = MIN (ret, tret);
>> - }
>> }
>> }
>>
>> @@ -3208,21 +3186,34 @@ gimplify_compound_lval (tree *expr_p, gimple_seq *pre_p, gimple_seq
>> *post_p,
>> fallback | fb_lvalue);
>> ret = MIN (ret, tret);
>>
>> - /* And finally, the indices and operands of ARRAY_REF. During this
>> - loop we also remove any useless conversions. */
>> + /* Step 3: gimplify size expressions and the indices and operands of
>> + ARRAY_REF. During this loop we also remove any useless conversions. */
>> +
>> for (; expr_stack.length () > 0; )
>> {
>> tree t = expr_stack.pop ();
>>
>> if (TREE_CODE (t) == ARRAY_REF || TREE_CODE (t) == ARRAY_RANGE_REF)
>> {
>> + /* Gimplify the low bound and element type size. */
>> + tret = gimplify_expr (&TREE_OPERAND (t, 2), pre_p, post_p,
>> + is_gimple_reg, fb_rvalue);
>> + ret = MIN (ret, tret);
>> +
>> + tret = gimplify_expr (&TREE_OPERAND (t, 3), pre_p, post_p,
>> + is_gimple_reg, fb_rvalue);
>> + ret = MIN (ret, tret);
>> +
>> /* Gimplify the dimension. */
>> - if (!is_gimple_min_invariant (TREE_OPERAND (t, 1)))
>> - {
>> - tret = gimplify_expr (&TREE_OPERAND (t, 1), pre_p, post_p,
>> - is_gimple_val, fb_rvalue);
>> - ret = MIN (ret, tret);
>> - }
>> + tret = gimplify_expr (&TREE_OPERAND (t, 1), pre_p, post_p,
>> + is_gimple_val, fb_rvalue);
>> + ret = MIN (ret, tret);
>> + }
>> + else if (TREE_CODE (t) == COMPONENT_REF)
>> + {
>> + tret = gimplify_expr (&TREE_OPERAND (t, 2), pre_p, post_p,
>> + is_gimple_reg, fb_rvalue);
>> + ret = MIN (ret, tret);
>> }
>>
>> STRIP_USELESS_TYPE_CONVERSION (TREE_OPERAND (t, 0));
>> @@ -6927,6 +6918,8 @@ gimplify_target_expr (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p)
>> {
>> if (!TYPE_SIZES_GIMPLIFIED (TREE_TYPE (temp)))
>> gimplify_type_sizes (TREE_TYPE (temp), pre_p);
>> + /* FIXME: this is correct only when the size of the type does
>> + not depend on expressions evaluated in init. */
>> gimplify_vla_decl (temp, pre_p);
>> }
>> else
>> diff --git a/gcc/testsuite/gcc.dg/vla-stexp-3.c b/gcc/testsuite/gcc.dg/vla-stexp-3.c
>> new file mode 100644
>> index 00000000000..e663de1cd72
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.dg/vla-stexp-3.c
>> @@ -0,0 +1,11 @@
>> +/* PR91038 */
>> +/* { dg-do compile } */
>> +/* { dg-options "" } */
>> +
>> +
>> +void bar(void)
>> +{
>> + ({ int N = 2; int (*x)[9][N] = 0; x; })[1];
>> + ({ int N = 2; int (*x)[9][N] = 0; x; })[0]; // should not ice
>> +}
>> +
>> diff --git a/gcc/testsuite/gcc.dg/vla-stexp-4.c b/gcc/testsuite/gcc.dg/vla-stexp-4.c
>> new file mode 100644
>> index 00000000000..612b5a802fc
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.dg/vla-stexp-4.c
>> @@ -0,0 +1,94 @@
>> +/* PR29970, PR91038 */
>> +/* { dg-do run } */
>> +/* { dg-options "-O0 -Wunused-variable" } */
>> +
>> +int foo3b(void) // should not return 0
>> +{
>> + int n = 0;
>> + return sizeof *({ n = 10; int x[n]; &x; });
>> +}
>> +
>> +int foo4(void) // should not ICE
>> +{
>> + return (*({
>> + int n = 20;
>> + char (*x)[n][n] = __builtin_malloc(n * n);
>> + (*x)[12][1] = 1;
>> + x;
>> + }))[12][1];
>> +}
>> +
>> +int foo5(void) // should return 1, returns 0
>> +{
>> + int n = 0;
>> + return (*({
>> + n = 20;
>> + char (*x)[n][n] = __builtin_malloc(n * n);
>> + (*x)[12][1] = 1;
>> + (*x)[0][1] = 0;
>> + x;
>> + }))[12][1];
>> +}
>> +
>> +int foo5c(void) // should return 400
>> +{
>> + int n = 0;
>> + return sizeof(*({
>> + n = 20;
>> + char (*x)[n][n] = __builtin_malloc(n * n);
>> + (*x)[12][1] = 1;
>> + (*x)[0][1] = 0;
>> + x;
>> + }));
>> +}
>> +
>> +int foo5b(void) // should return 1, returns 0
>> +{
>> + int n = 0; /* { dg-warning "unused variable" } */
>> + return (*({
>> + int n = 20;
>> + char (*x)[n][n] = __builtin_malloc(n * n);
>> + (*x)[12][1] = 1;
>> + (*x)[0][1] = 0;
>> + x;
>> + }))[12][1];
>> +}
>> +
>> +int foo5a(void) // should return 1, returns 0
>> +{
>> + return (*({
>> + int n = 20;
>> + char (*x)[n][n] = __builtin_malloc(n * n);
>> + (*x)[12][1] = 1;
>> + (*x)[0][1] = 0;
>> + x;
>> + }))[12][1];
>> +}
>> +
>> +
>> +
>> +
>> +int main()
>> +{
>> + if (sizeof(int[10]) != foo3b())
>> + __builtin_abort();
>> +
>> + if (1 != foo4())
>> + __builtin_abort();
>> +
>> + if (400 != foo5c())
>> + __builtin_abort();
>> +
>> + if (1 != foo5a())
>> + __builtin_abort();
>> +
>> + if (1 != foo5b()) // -O0
>> + __builtin_abort();
>> +
>> + if (1 != foo5())
>> + __builtin_abort();
>> +
>> + return 0;
>> +}
>> +
>> +
>> diff --git a/gcc/testsuite/gcc.dg/vla-stexp-5.c b/gcc/testsuite/gcc.dg/vla-stexp-5.c
>> new file mode 100644
>> index 00000000000..d6a7f2b34b8
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.dg/vla-stexp-5.c
>> @@ -0,0 +1,30 @@
>> +/* PR29970 */
>> +/* { dg-do run } */
>> +/* { dg-options "-Wunused-variable" } */
>> +
>> +
>> +
>> +
>> +int foo2a(void) // should not ICE
>> +{
>> + return ({ int n = 20; struct { int x[n];} x; x.x[12] = 1; sizeof(x); });
>> +}
>> +
>> +
>> +int foo2b(void) // should not ICE
>> +{
>> + return sizeof *({ int n = 20; struct { int x[n];} x; x.x[12] = 1; &x; });
>> +}
>> +
>> +int main()
>> +{
>> + if (sizeof(struct { int x[20]; }) != foo2a())
>> + __builtin_abort();
>> +
>> + if (sizeof(struct { int x[20]; }) != foo2b())
>> + __builtin_abort();
>> +
>> + return 0;
>> +}
>> +
>> +
>> diff --git a/gcc/testsuite/gcc.dg/vla-stexp-6.c b/gcc/testsuite/gcc.dg/vla-stexp-6.c
>> new file mode 100644
>> index 00000000000..3d96d38898b
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.dg/vla-stexp-6.c
>> @@ -0,0 +1,94 @@
>> +/* PR29970, PR91038 */
>> +/* { dg-do run } */
>> +/* { dg-options "-O2 -Wunused-variable" } */
>> +
>> +int foo3b(void) // should not return 0
>> +{
>> + int n = 0;
>> + return sizeof *({ n = 10; int x[n]; &x; });
>> +}
>> +
>> +int foo4(void) // should not ICE
>> +{
>> + return (*({
>> + int n = 20;
>> + char (*x)[n][n] = __builtin_malloc(n * n);
>> + (*x)[12][1] = 1;
>> + x;
>> + }))[12][1];
>> +}
>> +
>> +int foo5(void) // should return 1, returns 0
>> +{
>> + int n = 0;
>> + return (*({
>> + n = 20;
>> + char (*x)[n][n] = __builtin_malloc(n * n);
>> + (*x)[12][1] = 1;
>> + (*x)[0][1] = 0;
>> + x;
>> + }))[12][1];
>> +}
>> +
>> +int foo5c(void) // should return 400
>> +{
>> + int n = 0;
>> + return sizeof(*({
>> + n = 20;
>> + char (*x)[n][n] = __builtin_malloc(n * n);
>> + (*x)[12][1] = 1;
>> + (*x)[0][1] = 0;
>> + x;
>> + }));
>> +}
>> +
>> +int foo5b(void) // should return 1, returns 0
>> +{
>> + int n = 0; /* { dg-warning "unused variable" } */
>> + return (*({
>> + int n = 20;
>> + char (*x)[n][n] = __builtin_malloc(n * n);
>> + (*x)[12][1] = 1;
>> + (*x)[0][1] = 0;
>> + x;
>> + }))[12][1];
>> +}
>> +
>> +int foo5a(void) // should return 1, returns 0
>> +{
>> + return (*({
>> + int n = 20;
>> + char (*x)[n][n] = __builtin_malloc(n * n);
>> + (*x)[12][1] = 1;
>> + (*x)[0][1] = 0;
>> + x;
>> + }))[12][1];
>> +}
>> +
>> +
>> +
>> +
>> +int main()
>> +{
>> + if (sizeof(int[10]) != foo3b())
>> + __builtin_abort();
>> +
>> + if (1 != foo4())
>> + __builtin_abort();
>> +
>> + if (400 != foo5c())
>> + __builtin_abort();
>> +
>> + if (1 != foo5a())
>> + __builtin_abort();
>> +
>> + if (1 != foo5b()) // -O0
>> + __builtin_abort();
>> +
>> + if (1 != foo5())
>> + __builtin_abort();
>> +
>> + return 0;
>> +}
>> +
>> +
>> diff --git a/gcc/testsuite/gcc.dg/vla-stexp-7.c b/gcc/testsuite/gcc.dg/vla-stexp-7.c
>> new file mode 100644
>> index 00000000000..3091b9184c2
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.dg/vla-stexp-7.c
>> @@ -0,0 +1,44 @@
>> +/* PR91038 */
>> +/* { dg-do run } */
>> +/* { dg-options "-O2 -Wunused-variable" } */
>> +
>> +
>> +#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
>> +
>> +struct lbm {
>> +
>> + int D;
>> + const int* DQ;
>> +
>> +} D2Q9 = { 2,
>> + (const int*)&(const int[9][2]){
>> + { 0, 0 },
>> + { 1, 0 }, { 0, 1 }, { -1, 0 }, { 0, -1 },
>> + { 1, 1 }, { -1, 1 }, { -1, -1 }, { 1, -1 },
>> + }
>> +};
>> +
>> +void zouhe_left(void)
>> +{
>> + __auto_type xx = (*({ int N = 2; struct lbm __x = D2Q9; ((const int(*)[9][N])__x.DQ); }));
>> +
>> + if (1 != xx[1][0])
>> + __builtin_abort();
>> +
>> + if (2 != ARRAY_SIZE(xx[1]))
>> + __builtin_abort();
>> +
>> + if (1 != (*({ int N = 2; struct lbm __x = D2Q9; ((const int(*)[9][N])__x.DQ); }))[1][0])
>> + __builtin_abort();
>> +
>> + if (2 != ARRAY_SIZE(*({ int N = 2; struct lbm __x = D2Q9; ((const int(*)[9][N])__x.DQ);
>> })[1]))
>> + __builtin_abort();
>> +}
>> +
>> +int main()
>> +{
>> + zouhe_left();
>> + return 0;
>> +}
>> +
>> +
>> diff --git a/gcc/testsuite/gcc.dg/vla-stexp-8.c b/gcc/testsuite/gcc.dg/vla-stexp-8.c
>> new file mode 100644
>> index 00000000000..5b475eb6cf2
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.dg/vla-stexp-8.c
>> @@ -0,0 +1,47 @@
>> +/* PR29970, PR91038 */
>> +/* { dg-do compile } */
>> +/* { dg-options "-O2 -Wunused-variable" } */
>> +
>> +
>> +int foo0(void)
>> +{
>> + int c = *(*(*({ int n = 10; int (*x)[n][n] = __builtin_malloc(sizeof *x); x; }) + 5) + 5);
>> + return c;
>> +}
>> +
>> +int foo1(void)
>> +{
>> + int c = *(5 + *(5 + *({ int n = 10; int (*x)[n][n] = __builtin_malloc(sizeof *x); x; })));
>> + return c;
>> +}
>> +
>> +int bar2(void)
>> +{
>> + int c = (*({ int n = 10; struct { int y[n]; int z; }* x = __builtin_malloc(sizeof *x); x;
>> })).z;
>> + return c;
>> +}
>> +
>> +int bar3(void)
>> +{
>> + int n = 2; /* { dg-warning "unused variable" } */
>> + int c = (*({ int n = 3; /* { dg-warning "unused variable" } */
>> + ({ int n = 10; int (*x)[n][n] = __builtin_malloc(sizeof *x); x; }); }))[5][5];
>> + return c;
>> +}
>> +
>> +int bar3b(void)
>> +{
>> + int n = 2; /* { dg-warning "unused variable" } */
>> + int c = (*({ int n = 3; /* { dg-warning "unused variable" } */
>> + ({ int n = 10; int (*x)[n][n] = __builtin_malloc(sizeof *x); x; }); }))[0][0];
>> + return c;
>> +}
>> +
>> +int bar4(void)
>> +{
>> + int n = 2; /* { dg-warning "unused variable" } */
>> + int c = *(5 + *( 5 + *({ int n = 3; /* { dg-warning "unused variable" } */
>> + ({ int n = 10; int (*x)[n][n] = __builtin_malloc(sizeof *x); x; }); })));
>> + return c;
>> +}
>> +
>> diff --git a/gcc/testsuite/gcc.dg/vla-stexp-9.c b/gcc/testsuite/gcc.dg/vla-stexp-9.c
>> new file mode 100644
>> index 00000000000..3593a790785
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.dg/vla-stexp-9.c
>> @@ -0,0 +1,53 @@
>> +/* PR91038 */
>> +/* { dg-do run } */
>> +/* { dg-options "-O2 -Wunused-variable" } */
>> +
>> +
>> +
>> +void foo(void)
>> +{
>> + if (2 * sizeof(int) != sizeof((*({ int N = 2; int (*x)[9][N] = 0; x; })[1])))
>> + __builtin_abort();
>> +}
>> +
>> +void bar(void)
>> +{
>> + if (2 * sizeof(int) != sizeof((*({ int N = 2; int (*x)[9][N] = 0; x; })[0])))
>> + __builtin_abort();
>> +}
>> +
>> +void bar0(void)
>> +{
>> + if (2 * 9 * sizeof(int) != sizeof((*({ int N = 2; int (*x)[9][N] = 0; x; }))))
>> + __builtin_abort();
>> +}
>> +
>> +void bar11(void)
>> +{
>> + sizeof(*((*({ int N = 2; int (*x)[9][N] = 0; x; }) + 0)));
>> +}
>> +
>> +void bar12(void)
>> +{
>> + if (2 * sizeof(int) != sizeof(*((*({ int N = 2; int (*x)[9][N] = 0; x; }) ))))
>> + __builtin_abort();
>> +}
>> +
>> +void bar1(void)
>> +{
>> + if (2 * sizeof(int) != sizeof(*((*({ int N = 2; int (*x)[9][N] = 0; x; }) + 0))))
>> + __builtin_abort();
>> +}
>> +
>> +
>> +
>> +
>> +int main()
>> +{
>> + foo();
>> + bar0();
>> + bar12();
>> + bar1();
>> + bar();
>> +}
>> +
>>
More information about the Gcc-patches
mailing list