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

Here is the third version of the patch. This also
fixes the index zero case.  Thus, this should be
a complete fix for 91038 and should fix all cases
also supported by clang.  Still not working is
returning a struct of variable size from a
statement expression (29970) when the size depends
on computations inside the statement expression.

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 not reording some size-related expressions
during folding. 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.
	(gimplify_target_expr): Do not gimplify size expression.
	* fold-const.c (fold_binary_loc): Do not reorder SAVE_EXPR
	in pointer arithmetic for variably modified types.
    
    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.

On 9/5/21 15:14, Uecker, Martin wrote:
> 
> Here is the third version of the patch. This also
> fixes the index zero case.  Thus, this should be
> a complete fix for 91038 and should fix all cases
> also supported by clang.  Still not working is
> returning a struct of variable size from a
> statement expression (29970) when the size depends
> on computations inside the statement expression.
> 
> 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 not reording some size-related expressions
> during folding. 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.
> 	(gimplify_target_expr): Do not gimplify size expression.
> 	* fold-const.c (fold_binary_loc): Do not reorder SAVE_EXPR
> 	in pointer arithmetic for variably modified types.
>      
>      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/fold-const.c b/gcc/fold-const.c
> index ff23f12f33c..1e6f50692b5 100644
> --- a/gcc/fold-const.c
> +++ b/gcc/fold-const.c
> @@ -10854,7 +10854,15 @@ fold_binary_loc (location_t loc, enum tree_code code, tree type,
>   	  return build2_loc (loc, COMPOUND_EXPR, type, TREE_OPERAND (arg0, 0),
>   			     tem);
>   	}
> -      if (TREE_CODE (arg1) == COMPOUND_EXPR)
> +      /* This interleaves execution of the two sub-expressions
> +	 which is allowed in C.  For pointer arithmetic when the
> +	 the pointer has a variably modified type, the right expression
> +	 might have a SAVE_EXPR which depends on the left expr, so
> +	 do not fold in this case.  */
> +      if (TREE_CODE (arg1) == COMPOUND_EXPR
> +	  && !(code == POINTER_PLUS_EXPR
> +	       && TREE_CODE (TREE_OPERAND (arg1, 0)) == SAVE_EXPR)
> +	       && variably_modified_type_p (type, NULL_TREE))

This seems pretty fragile.  If the problem is that the SAVE_EXPR depends 
on a statement-expr on the LHS, can't that happen with expressions other 
than POINTER_PLUS_EXPR?

Maybe we should include the statement-expr in the SAVE_EXPR?

>   	{
>   	  tem = fold_build2_loc (loc, code, type, op0,
>   			     fold_convert_loc (loc, TREE_TYPE (op1),
> diff --git a/gcc/gimplify.c b/gcc/gimplify.c
> index 99d1c7fcce4..8ee205f593c 100644
> --- a/gcc/gimplify.c
> +++ b/gcc/gimplify.c
> @@ -2840,7 +2840,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.
> +  */
>     if (VAR_P (decl)
>         && !DECL_SEEN_IN_BIND_EXPR_P (decl)
>         && !TREE_STATIC (decl) && !DECL_EXTERNAL (decl)
> @@ -2985,16 +2988,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)
> @@ -3003,18 +3012,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)
>   	    {
> @@ -3031,18 +3030,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)
>   	{
> @@ -3062,18 +3051,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);
> -	    }
>   	}
>       }
>   
> @@ -3084,21 +3063,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));
> @@ -6766,8 +6758,8 @@ gimplify_target_expr (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p)
>   	 to the temps list.  Handle also variable length TARGET_EXPRs.  */
>         if (!poly_int_tree_p (DECL_SIZE (temp)))
>   	{
> -	  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();
> +}
> +
>

Am Mittwoch, den 22.09.2021, 17:18 -0400 schrieb Jason Merrill:
> On 9/5/21 15:14, Uecker, Martin wrote:
> > Here is the third version of the patch. This also
> > fixes the index zero case.  Thus, this should be
> > a complete fix for 91038 and should fix all cases
> > also supported by clang.  Still not working is
> > returning a struct of variable size from a
> > statement expression (29970) when the size depends
> > on computations inside the statement expression.
> > 
> > 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 not reording some size-related expressions
> > during folding. 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.
> > 	(gimplify_target_expr): Do not gimplify size expression.
> > 	* fold-const.c (fold_binary_loc): Do not reorder SAVE_EXPR
> > 	in pointer arithmetic for variably modified types.
> >      
> >      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/fold-const.c b/gcc/fold-const.c
> > index ff23f12f33c..1e6f50692b5 100644
> > --- a/gcc/fold-const.c
> > +++ b/gcc/fold-const.c
> > @@ -10854,7 +10854,15 @@ fold_binary_loc (location_t loc, enum tree_code code, tree type,
> >   	  return build2_loc (loc, COMPOUND_EXPR, type, TREE_OPERAND (arg0, 0),
> >   			     tem);
> >   	}
> > -      if (TREE_CODE (arg1) == COMPOUND_EXPR)
> > +      /* This interleaves execution of the two sub-expressions
> > +	 which is allowed in C.  For pointer arithmetic when the
> > +	 the pointer has a variably modified type, the right expression
> > +	 might have a SAVE_EXPR which depends on the left expr, so
> > +	 do not fold in this case.  */
> > +      if (TREE_CODE (arg1) == COMPOUND_EXPR
> > +	  && !(code == POINTER_PLUS_EXPR
> > +	       && TREE_CODE (TREE_OPERAND (arg1, 0)) == SAVE_EXPR)
> > +	       && variably_modified_type_p (type, NULL_TREE))
> 
> This seems pretty fragile.  If the problem is that the SAVE_EXPR depends 
> on a statement-expr on the LHS, can't that happen with expressions other 
> than POINTER_PLUS_EXPR?

I intentionally limited the change to this specific
case to avoid accidentally breaking or pessimizing
anything else. I did not notice any other cases that
need fixing. It is of course possible that
I have missed some...

> 
> Maybe we should include the statement-expr in the SAVE_EXPR?

I am not really sure how to implement this.

Maybe we could apply this patch first (because
I have to work around this bug in a couple of
projects which is a bit annoying)? I am happy
to implement an alternative later if there is
a better way (which I can understand).

Martin



> >   	{
> >   	  tem = fold_build2_loc (loc, code, type, op0,
> >   			     fold_convert_loc (loc, TREE_TYPE (op1),
> > diff --git a/gcc/gimplify.c b/gcc/gimplify.c
> > index 99d1c7fcce4..8ee205f593c 100644
> > --- a/gcc/gimplify.c
> > +++ b/gcc/gimplify.c
> > @@ -2840,7 +2840,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.
> > +  */
> >     if (VAR_P (decl)
> >         && !DECL_SEEN_IN_BIND_EXPR_P (decl)
> >         && !TREE_STATIC (decl) && !DECL_EXTERNAL (decl)
> > @@ -2985,16 +2988,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)
> > @@ -3003,18 +3012,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)
> >   	    {
> > @@ -3031,18 +3030,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)
> >   	{
> > @@ -3062,18 +3051,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);
> > -	    }
> >   	}
> >       }
> >   
> > @@ -3084,21 +3063,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));
> > @@ -6766,8 +6758,8 @@ gimplify_target_expr (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p)
> >   	 to the temps list.  Handle also variable length TARGET_EXPRs.  */
> >         if (!poly_int_tree_p (DECL_SIZE (temp)))
> >   	{
> > -	  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();
> > +}
> > +
> >

On 9/23/21 15:49, Uecker, Martin wrote:
> Am Mittwoch, den 22.09.2021, 17:18 -0400 schrieb Jason Merrill:
>> On 9/5/21 15:14, Uecker, Martin wrote:
>>> Here is the third version of the patch. This also
>>> fixes the index zero case.  Thus, this should be
>>> a complete fix for 91038 and should fix all cases
>>> also supported by clang.  Still not working is
>>> returning a struct of variable size from a
>>> statement expression (29970) when the size depends
>>> on computations inside the statement expression.
>>>
>>> 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 not reording some size-related expressions
>>> during folding. 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.
>>> 	(gimplify_target_expr): Do not gimplify size expression.
>>> 	* fold-const.c (fold_binary_loc): Do not reorder SAVE_EXPR
>>> 	in pointer arithmetic for variably modified types.
>>>       
>>>       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/fold-const.c b/gcc/fold-const.c
>>> index ff23f12f33c..1e6f50692b5 100644
>>> --- a/gcc/fold-const.c
>>> +++ b/gcc/fold-const.c
>>> @@ -10854,7 +10854,15 @@ fold_binary_loc (location_t loc, enum tree_code code, tree type,
>>>    	  return build2_loc (loc, COMPOUND_EXPR, type, TREE_OPERAND (arg0, 0),
>>>    			     tem);
>>>    	}
>>> -      if (TREE_CODE (arg1) == COMPOUND_EXPR)
>>> +      /* This interleaves execution of the two sub-expressions
>>> +	 which is allowed in C.  For pointer arithmetic when the
>>> +	 the pointer has a variably modified type, the right expression
>>> +	 might have a SAVE_EXPR which depends on the left expr, so
>>> +	 do not fold in this case.  */
>>> +      if (TREE_CODE (arg1) == COMPOUND_EXPR
>>> +	  && !(code == POINTER_PLUS_EXPR
>>> +	       && TREE_CODE (TREE_OPERAND (arg1, 0)) == SAVE_EXPR)
>>> +	       && variably_modified_type_p (type, NULL_TREE))
>>
>> This seems pretty fragile.  If the problem is that the SAVE_EXPR depends
>> on a statement-expr on the LHS, can't that happen with expressions other
>> than POINTER_PLUS_EXPR?
> 
> I intentionally limited the change to this specific
> case to avoid accidentally breaking or pessimizing
> anything else. I did not notice any other cases that
> need fixing. It is of course possible that
> I have missed some...
> 
>>
>> Maybe we should include the statement-expr in the SAVE_EXPR?
> 
> I am not really sure how to implement this.

Nor am I, I'm not at all familiar with the VLA handling code.  But if 
the generated trees rely on evaluation happening with a stricter order 
than is actually guaranteed, that seems like a bug in the generation of 
the expression trees, not the folding code.

Could someone that knows more about VLA weigh in?

> Maybe we could apply this patch first (because
> I have to work around this bug in a couple of
> projects which is a bit annoying)? I am happy
> to implement an alternative later if there is
> a better way (which I can understand).

The gimplify_compound_lval change is OK now.

What's the rationale for the gimplify_target_expr change?

>>>    	{
>>>    	  tem = fold_build2_loc (loc, code, type, op0,
>>>    			     fold_convert_loc (loc, TREE_TYPE (op1),
>>> diff --git a/gcc/gimplify.c b/gcc/gimplify.c
>>> index 99d1c7fcce4..8ee205f593c 100644
>>> --- a/gcc/gimplify.c
>>> +++ b/gcc/gimplify.c
>>> @@ -2840,7 +2840,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.
>>> +  */
>>>      if (VAR_P (decl)
>>>          && !DECL_SEEN_IN_BIND_EXPR_P (decl)
>>>          && !TREE_STATIC (decl) && !DECL_EXTERNAL (decl)
>>> @@ -2985,16 +2988,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)
>>> @@ -3003,18 +3012,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)
>>>    	    {
>>> @@ -3031,18 +3030,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)
>>>    	{
>>> @@ -3062,18 +3051,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);
>>> -	    }
>>>    	}
>>>        }
>>>    
>>> @@ -3084,21 +3063,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));
>>> @@ -6766,8 +6758,8 @@ gimplify_target_expr (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p)
>>>    	 to the temps list.  Handle also variable length TARGET_EXPRs.  */
>>>          if (!poly_int_tree_p (DECL_SIZE (temp)))
>>>    	{
>>> -	  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();
>>> +}
>>> +
>>>

Am Donnerstag, den 23.09.2021, 17:37 -0400 schrieb Jason Merrill:
> On 9/23/21 15:49, Uecker, Martin wrote:
> > Am Mittwoch, den 22.09.2021, 17:18 -0400 schrieb Jason Merrill:
> > > On 9/5/21 15:14, Uecker, Martin wrote:
> > > > Here is the third version of the patch. This also
> > > > fixes the index zero case.  Thus, this should be
> > > > a complete fix for 91038 and should fix all cases
> > > > also supported by clang.  Still not working is
> > > > returning a struct of variable size from a
> > > > statement expression (29970) when the size depends
> > > > on computations inside the statement expression.
> > > > 
> > > > 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 not reording some size-related expressions
> > > > during folding. 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.
> > > > 	(gimplify_target_expr): Do not gimplify size expression.
> > > > 	* fold-const.c (fold_binary_loc): Do not reorder SAVE_EXPR
> > > > 	in pointer arithmetic for variably modified types.
> > > >       
> > > >       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/fold-const.c b/gcc/fold-const.c
> > > > index ff23f12f33c..1e6f50692b5 100644
> > > > --- a/gcc/fold-const.c
> > > > +++ b/gcc/fold-const.c
> > > > @@ -10854,7 +10854,15 @@ fold_binary_loc (location_t loc, enum tree_code code, tree type,
> > > >    	  return build2_loc (loc, COMPOUND_EXPR, type, TREE_OPERAND (arg0, 0),
> > > >    			     tem);
> > > >    	}
> > > > -      if (TREE_CODE (arg1) == COMPOUND_EXPR)
> > > > +      /* This interleaves execution of the two sub-expressions
> > > > +	 which is allowed in C.  For pointer arithmetic when the
> > > > +	 the pointer has a variably modified type, the right expression
> > > > +	 might have a SAVE_EXPR which depends on the left expr, so
> > > > +	 do not fold in this case.  */
> > > > +      if (TREE_CODE (arg1) == COMPOUND_EXPR
> > > > +	  && !(code == POINTER_PLUS_EXPR
> > > > +	       && TREE_CODE (TREE_OPERAND (arg1, 0)) == SAVE_EXPR)
> > > > +	       && variably_modified_type_p (type, NULL_TREE))
> > > 
> > > This seems pretty fragile.  If the problem is that the SAVE_EXPR depends
> > > on a statement-expr on the LHS, can't that happen with expressions other
> > > than POINTER_PLUS_EXPR?
> > 
> > I intentionally limited the change to this specific
> > case to avoid accidentally breaking or pessimizing
> > anything else. I did not notice any other cases that
> > need fixing. It is of course possible that
> > I have missed some...
> > 
> > > Maybe we should include the statement-expr in the SAVE_EXPR?
> > 
> > I am not really sure how to implement this.
> 
> Nor am I, I'm not at all familiar with the VLA handling code.  But if 
> the generated trees rely on evaluation happening with a stricter order 
> than is actually guaranteed, that seems like a bug in the generation of 
> the expression trees, not the folding code.

Is there a definition about what is guaranteed?

It seems to be based on the somewhat scary
C semantics where in

(a, b) op (c, d)

evaluation order could be

a, c, b, d.

If I remember correctly

({ int N; int x[N]; &x; })[0]

becomes

TARGET_EXPR <D, { int N; int x[N]; D = &x }> + (SAVE_EXPR <N> * 0)

which (somehow involving the rule above) becomes

SAVE_EXPR <N>; TARGET_EXPR <D, { int N; int x[N]; D = &x })

which then fails to gimplify.

It is not entirely clear to me what code the FE
should generate to avoid this.

> Could someone that knows more about VLA weigh in?

> > Maybe we could apply this patch first (because
> > I have to work around this bug in a couple of
> > projects which is a bit annoying)? I am happy
> > to implement an alternative later if there is
> > a better way (which I can understand).
> 
> The gimplify_compound_lval change is OK now.
> 
> What's the rationale for the gimplify_target_expr change?

It seems another case where size expressions are
gimplified earlier than expressions they might
depend
on.

But I can't seem to find a case anymore where this
change makes a difference.


Martin

> > > >    	{
> > > >    	  tem = fold_build2_loc (loc, code, type, op0,
> > > >    			     fold_convert_loc (loc, TREE_TYPE (op1),
> > > > diff --git a/gcc/gimplify.c b/gcc/gimplify.c
> > > > index 99d1c7fcce4..8ee205f593c 100644
> > > > --- a/gcc/gimplify.c
> > > > +++ b/gcc/gimplify.c
> > > > @@ -2840,7 +2840,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.
> > > > +  */
> > > >      if (VAR_P (decl)
> > > >          && !DECL_SEEN_IN_BIND_EXPR_P (decl)
> > > >          && !TREE_STATIC (decl) && !DECL_EXTERNAL (decl)
> > > > @@ -2985,16 +2988,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)
> > > > @@ -3003,18 +3012,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)
> > > >    	    {
> > > > @@ -3031,18 +3030,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)
> > > >    	{
> > > > @@ -3062,18 +3051,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);
> > > > -	    }
> > > >    	}
> > > >        }
> > > >    
> > > > @@ -3084,21 +3063,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));
> > > > @@ -6766,8 +6758,8 @@ gimplify_target_expr (tree *expr_p, gimple_seq *pre_p, gimple_seq
> > > > *post_p)
> > > >    	 to the temps list.  Handle also variable length TARGET_EXPRs.  */
> > > >          if (!poly_int_tree_p (DECL_SIZE (temp)))
> > > >    	{
> > > > -	  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();
> > > > +}
> > > > +
> > > >

On 10/2/21 15:06, Uecker, Martin wrote:
> Am Donnerstag, den 23.09.2021, 17:37 -0400 schrieb Jason Merrill:
>> On 9/23/21 15:49, Uecker, Martin wrote:
>>> Am Mittwoch, den 22.09.2021, 17:18 -0400 schrieb Jason Merrill:
>>>> On 9/5/21 15:14, Uecker, Martin wrote:
>>>>> Here is the third version of the patch. This also
>>>>> fixes the index zero case.  Thus, this should be
>>>>> a complete fix for 91038 and should fix all cases
>>>>> also supported by clang.  Still not working is
>>>>> returning a struct of variable size from a
>>>>> statement expression (29970) when the size depends
>>>>> on computations inside the statement expression.
>>>>>
>>>>> 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 not reording some size-related expressions
>>>>> during folding. 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.
>>>>> 	(gimplify_target_expr): Do not gimplify size expression.
>>>>> 	* fold-const.c (fold_binary_loc): Do not reorder SAVE_EXPR
>>>>> 	in pointer arithmetic for variably modified types.
>>>>>        
>>>>>        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/fold-const.c b/gcc/fold-const.c
>>>>> index ff23f12f33c..1e6f50692b5 100644
>>>>> --- a/gcc/fold-const.c
>>>>> +++ b/gcc/fold-const.c
>>>>> @@ -10854,7 +10854,15 @@ fold_binary_loc (location_t loc, enum tree_code code, tree type,
>>>>>     	  return build2_loc (loc, COMPOUND_EXPR, type, TREE_OPERAND (arg0, 0),
>>>>>     			     tem);
>>>>>     	}
>>>>> -      if (TREE_CODE (arg1) == COMPOUND_EXPR)
>>>>> +      /* This interleaves execution of the two sub-expressions
>>>>> +	 which is allowed in C.  For pointer arithmetic when the
>>>>> +	 the pointer has a variably modified type, the right expression
>>>>> +	 might have a SAVE_EXPR which depends on the left expr, so
>>>>> +	 do not fold in this case.  */
>>>>> +      if (TREE_CODE (arg1) == COMPOUND_EXPR
>>>>> +	  && !(code == POINTER_PLUS_EXPR
>>>>> +	       && TREE_CODE (TREE_OPERAND (arg1, 0)) == SAVE_EXPR)
>>>>> +	       && variably_modified_type_p (type, NULL_TREE))
>>>>
>>>> This seems pretty fragile.  If the problem is that the SAVE_EXPR depends
>>>> on a statement-expr on the LHS, can't that happen with expressions other
>>>> than POINTER_PLUS_EXPR?
>>>
>>> I intentionally limited the change to this specific
>>> case to avoid accidentally breaking or pessimizing
>>> anything else. I did not notice any other cases that
>>> need fixing. It is of course possible that
>>> I have missed some...
>>>
>>>> Maybe we should include the statement-expr in the SAVE_EXPR?
>>>
>>> I am not really sure how to implement this.
>>
>> Nor am I, I'm not at all familiar with the VLA handling code.  But if
>> the generated trees rely on evaluation happening with a stricter order
>> than is actually guaranteed, that seems like a bug in the generation of
>> the expression trees, not the folding code.
> 
> Is there a definition about what is guaranteed?
> 
> It seems to be based on the somewhat scary
> C semantics where in
> 
> (a, b) op (c, d)
> 
> evaluation order could be
> 
> a, c, b, d.

Exactly.  Or c, d, a, b, for that matter.

> If I remember correctly
> 
> ({ int N; int x[N]; &x; })[0]
> 
> becomes
> 
> TARGET_EXPR <D, { int N; int x[N]; D = &x }> + (SAVE_EXPR <N> * 0)
> 
> which (somehow involving the rule above) becomes
> 
> SAVE_EXPR <N>; TARGET_EXPR <D, { int N; int x[N]; D = &x })
> 
> which then fails to gimplify.
> 
> It is not entirely clear to me what code the FE
> should generate to avoid this.

It looks like the problem comes when pointer_int_sum multiplies size_exp 
by constant 0, which gets folded into a COMPOUND_EXPR, which is 
problematic here.

The simplest fix is probably for pointer_int_sum to avoid doing the 
multiplication with a 0 index.

More generally, the problem is that pointer_int_sum assumes that the 
element size can be evaluated without previously evaluating the pointer 
operand.  Given

p + sizeof(*p)*off

we could turn that into

SAVE_EXPR<p> + (SAVE_EXPR<p>, sizeof(*p)*off)

so that p is evaluated before sizeof(*p) regardless of transformations. 
  It probably makes sense to condition that on if sizeof(*p) is 
TREE_SIDE_EFFECTS.

>> Could someone that knows more about VLA weigh in?
> 
>>> Maybe we could apply this patch first (because
>>> I have to work around this bug in a couple of
>>> projects which is a bit annoying)? I am happy
>>> to implement an alternative later if there is
>>> a better way (which I can understand).
>>
>> The gimplify_compound_lval change is OK now.
>>
>> What's the rationale for the gimplify_target_expr change?
> 
> It seems another case where size expressions are
> gimplified earlier than expressions they might
> depend
> on.
> 
> But I can't seem to find a case anymore where this
> change makes a difference.
> 
> 
> Martin
> 
>>>>>     	{
>>>>>     	  tem = fold_build2_loc (loc, code, type, op0,
>>>>>     			     fold_convert_loc (loc, TREE_TYPE (op1),
>>>>> diff --git a/gcc/gimplify.c b/gcc/gimplify.c
>>>>> index 99d1c7fcce4..8ee205f593c 100644
>>>>> --- a/gcc/gimplify.c
>>>>> +++ b/gcc/gimplify.c
>>>>> @@ -2840,7 +2840,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.
>>>>> +  */
>>>>>       if (VAR_P (decl)
>>>>>           && !DECL_SEEN_IN_BIND_EXPR_P (decl)
>>>>>           && !TREE_STATIC (decl) && !DECL_EXTERNAL (decl)
>>>>> @@ -2985,16 +2988,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)
>>>>> @@ -3003,18 +3012,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)
>>>>>     	    {
>>>>> @@ -3031,18 +3030,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)
>>>>>     	{
>>>>> @@ -3062,18 +3051,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);
>>>>> -	    }
>>>>>     	}
>>>>>         }
>>>>>     
>>>>> @@ -3084,21 +3063,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));
>>>>> @@ -6766,8 +6758,8 @@ gimplify_target_expr (tree *expr_p, gimple_seq *pre_p, gimple_seq
>>>>> *post_p)
>>>>>     	 to the temps list.  Handle also variable length TARGET_EXPRs.  */
>>>>>           if (!poly_int_tree_p (DECL_SIZE (temp)))
>>>>>     	{
>>>>> -	  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();
>>>>> +}
>>>>> +
>>>>>