[PATCH v4] Fix ICE when mixing VLAs and statement expressions [PR91038]
Jason Merrill
jason@redhat.com
Wed Nov 3 14:18:34 GMT 2021
On 10/31/21 05:22, Uecker, Martin wrote:
>
> Hi Jason,
>
> here is the fourth version of the patch.
>
> I followed your suggestion and now make this
> transformation sooner in pointer_int_sum.
> I also added a check to only do this
> transformation when the pointer is not a
> VAR_DECL, which avoids it in the most
> common cases where it is not necessary.
>
> Looking for BIND_EXPR seems complicated
> and I could not convince myself that it is
> worth it. I also see the risk that this
> makes potential failure cases even more
> subtle. What do you think?
That makes sense. Though see some minor comments below.
> Bootstrapped and regression tested
> on x86-64 for all languages.
>
>
>
> Martin
>
>
>
>
> Fix ICE when mixing VLAs and statement expressions [PR91038]
>
> When returning VM-types from statement expressions, this can
> lead to an ICE when declarations from the statement expression
> are referred to later. Most of these issues can be addressed by
> gimplifying the base expression earlier in gimplify_compound_lval.
> Another issue is fixed by wrapping the pointer expression in
> pointer_int_sum. This fixes PR91038 and some of the test cases
> from PR29970 (structs with VLA members need further work).
>
>
> 2021-10-30 Martin Uecker <muecker@gwdg.de>
>
> gcc/
> PR c/91038
> PR c/29970
> * c-family/c-common.c (pointer_int_sum): Make sure
> pointer expressions are evaluated first when the size
> expression depends on for variably-modified types.
> * gimplify.c (gimplify_var_or_parm_decl): Update comment.
> (gimplify_compound_lval): Gimplify base expression first.
> (gimplify_target_expr): Add comment.
>
> gcc/testsuite/
> PR c/91038
> PR c/29970
> * gcc.dg/vla-stexp-3.c: New test.
> * gcc.dg/vla-stexp-4.c: New test.
> * gcc.dg/vla-stexp-5.c: New test.
> * gcc.dg/vla-stexp-6.c: New test.
> * gcc.dg/vla-stexp-7.c: New test.
> * gcc.dg/vla-stexp-8.c: New test.
> * gcc.dg/vla-stexp-9.c: New test.
> diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c
> index 32c7e3e8972..a10b374dbed 100644
> --- a/gcc/c-family/c-common.c
> +++ b/gcc/c-family/c-common.c
> @@ -3301,7 +3301,20 @@ 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.
> + */
We usually don't put * at the beginning of new comment lines, and we put
the closing */ at the end of the last line of text.
> + if (TREE_SIDE_EFFECTS (size_exp)
> + && variably_modified_type_p (TREE_TYPE (ptrop), NULL)
> + && (VAR_DECL != TREE_CODE (ptrop)))
How about checking TREE_SIDE_EFFECTS (ptrop) instead of TREE_CODE to
avoid this in more cases?
> + {
> + ptrop = build1_loc (loc, SAVE_EXPR, TREE_TYPE (ptrop), ptrop);
Why not use save_expr()?
> + 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 8bb54fd7481..7b6874a3142 100644
> --- a/gcc/gimplify.c
> +++ b/gcc/gimplify.c
> @@ -2958,7 +2958,10 @@ 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.
> + */
Again, */ on the last line of text.
> if (VAR_P (decl)
> && !DECL_SEEN_IN_BIND_EXPR_P (decl)
> && !TREE_STATIC (decl) && !DECL_EXTERNAL (decl)
> @@ -3103,16 +3106,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)
> @@ -3121,18 +3130,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)
> {
> @@ -3149,18 +3148,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)
> {
> @@ -3180,18 +3169,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);
> - }
> }
> }
>
> @@ -3202,21 +3181,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));
> @@ -6914,6 +6906,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