[PATCH] Fix ICE when mixing VLAs and statement expressions [PR91038]

Richard Biener richard.guenther@gmail.com
Mon Aug 2 10:16:13 GMT 2021


On Sun, Aug 1, 2021 at 7:37 PM Uecker, Martin
<Martin.Uecker@med.uni-goettingen.de> wrote:
>
>
>
> Here is an attempt to fix some old and annoying bugs related
> to VLAs and statement expressions. In particulary, this seems
> to fix the issues with variably-modified types which are
> returned from statement expressions (which works on clang),
> but there are still bugs remaining related to structs
> with VLA members (which seems to be a FE bug).
>
> Of course, I might be doing something stupid...

How's evaluation order of (f())[g()] defined (with f returning a pointer)?
Isn't that just f() + g()*sizeof(int) and thus undefined?

If it's undefined then I think the incoming GENERIC is ill-defined.

> The patch survives bootstrapping and regresstion testing
> on x86_64.
>
>
> --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. Some of these issues can be addressed by
> gimplifying the base expression earlier in gimplify_compound_lval.
> This fixes PR91038 and some of the test cases from PR29970
> (structs with VLA members need further work).
>
>
>     2021-08-01  Martin Uecker  <muecker@gwdg.de>
>
>     gcc/
>         PR c/91038
>         PR c/29970
>         * gimplify.c (gimplify_var_or_parm_decl): Update comment.
>         (gimplify_compound_lval): Gimplify base expression first.
>
>     gcc/testsuite/
>         PR c/91038
>         PR c/29970
>         * gcc.dg/vla-stexp-01.c: New test.
>         * gcc.dg/vla-stexp-02.c: New test.
>         * gcc.dg/vla-stexp-03.c: New test.
>         * gcc.dg/vla-stexp-04.c: New test.
>
>
> diff --git a/gcc/gimplify.c b/gcc/gimplify.c
> index 21ff32ee4aa..885d5f73585 100644
> --- a/gcc/gimplify.c
> +++ b/gcc/gimplify.c
> @@ -2839,7 +2839,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 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)
> @@ -2984,9 +2987,23 @@ 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.  */
> +     So we do this in three steps.  First we gimplify the base,
> +     then we deal with the annotations for any variables in the
> +     components, 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 first. */
> +
> +  /* Step 1 is to gimplify the base expression.  Make sure lvalue is set
> +     so as to match the min_lval predicate.  Failure to do so may result
> +     in the creation of large aggregate temporaries.  */
> +  tret = gimplify_expr (p, pre_p, post_p, is_gimple_min_lval,
> +                       fallback | fb_lvalue);
> +
> +  ret = MIN (ret, tret);
> +
> +
>    for (i = expr_stack.length () - 1; i >= 0; i--)
>      {
>        tree t = expr_stack[i];
> @@ -3076,12 +3093,6 @@ gimplify_compound_lval (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p,
>         }
>      }
>
> -  /* Step 2 is to gimplify the base expression.  Make sure lvalue is set
> -     so as to match the min_lval predicate.  Failure to do so may result
> -     in the creation of large aggregate temporaries.  */
> -  tret = gimplify_expr (p, pre_p, post_p, is_gimple_min_lval,
> -                       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.  */
> diff --git a/gcc/testsuite/gcc.dg/vla-stexpr-01.c b/gcc/testsuite/gcc.dg/vla-stexpr-01.c
> new file mode 100644
> index 00000000000..27e1817eb63
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/vla-stexpr-01.c
> @@ -0,0 +1,94 @@
> +/* PR29970, PR91038 */
> +/* { dg-do run } */
> +/* { dg-options "-O0" } */
> +
> +int foo3b(void)   // should not return 0
> +{
> +        int n = 0;
> +        return sizeof *({ n = 10; int x[n]; x; &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;
> +        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-stexpr-02.c b/gcc/testsuite/gcc.dg/vla-stexpr-02.c
> new file mode 100644
> index 00000000000..99b4f571e52
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/vla-stexpr-02.c
> @@ -0,0 +1,31 @@
> +/* PR29970 */
> +/* { dg-do run } */
> +/* { dg-options "" } */
> +
> +
> +
> +
> +int foo2a(void)   // should not ICE
> +{
> +        return ({ int n = 20; struct { int x[n];} x; x.x[12] = 1; sizeof(x); });
> +}
> +
> +#if 0
> +int foo2b(void)   // should not ICE
> +{
> +        return sizeof *({ int n = 20; struct { int x[n];} x; x.x[12] = 1; &x; });
> +}
> +#endif
> +
> +int main()
> +{
> +       if (sizeof(struct { int x[20]; }) != foo2a())
> +               __builtin_abort();
> +#if 0
> +       if (sizeof(struct { int x[20]; }) != foo2b())
> +               __builtin_abort();
> +#endif
> +       return 0;
> +}
> +
> +
> diff --git a/gcc/testsuite/gcc.dg/vla-stexpr-03.c b/gcc/testsuite/gcc.dg/vla-stexpr-03.c
> new file mode 100644
> index 00000000000..61f8986eaa3
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/vla-stexpr-03.c
> @@ -0,0 +1,94 @@
> +/* PR29970, PR91038 */
> +/* { dg-do run } */
> +/* { dg-options "-O2" } */
> +
> +int foo3b(void)   // should not return 0
> +{
> +        int n = 0;
> +        return sizeof *({ n = 10; int x[n]; x; &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;
> +        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-stexpr-04.c b/gcc/testsuite/gcc.dg/vla-stexpr-04.c
> new file mode 100644
> index 00000000000..ffc8b12fa9b
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/vla-stexpr-04.c
> @@ -0,0 +1,38 @@
> +/* PR91038 */
> +/* { dg-do run } */
> +/* { dg-options "" } */
> +
> +
> +#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();
> +}
> +
> +int main()
> +{
> +       zouhe_left();
> +       return 0;
> +}
> +
> +


More information about the Gcc-patches mailing list