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

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

[Uecker, Martin]

Hi Jason,

here is the fourth version of the patch.

I followed your suggestion and now make this
transformation sooner in pointer_int_sum.
I also added a check to only do this
transformation when the pointer is not a
VAR_DECL, which avoids it in the most
common cases where it is not necessary.

Looking for BIND_EXPR seems complicated
and I could not convince myself that it is
worth it.  I also see the risk that this
makes potential failure cases even more
subtle. What do you think?

Bootstrapped and regression tested
on x86-64 for all languages.



Martin




Fix ICE when mixing VLAs and statement expressions [PR91038]

When returning VM-types from statement expressions, this can
lead to an ICE when declarations from the statement expression
are referred to later. Most of these issues can be addressed by
gimplifying the base expression earlier in gimplify_compound_lval.
Another issue is fixed by wrapping the pointer expression in
pointer_int_sum. This fixes PR91038 and some of the test cases
from PR29970 (structs with VLA members need further work).

    
    2021-10-30  Martin Uecker  <muecker@gwdg.de>
    
    gcc/
        PR c/91038
        PR c/29970
	* c-family/c-common.c (pointer_int_sum): Make sure
	pointer expressions are evaluated first when the size
	expression depends on for variably-modified types.
        * gimplify.c (gimplify_var_or_parm_decl): Update comment.
        (gimplify_compound_lval): Gimplify base expression first.
        (gimplify_target_expr): Add comment.
    
    gcc/testsuite/
        PR c/91038
        PR c/29970
        * gcc.dg/vla-stexp-3.c: New test.
        * gcc.dg/vla-stexp-4.c: New test.
        * gcc.dg/vla-stexp-5.c: New test.
        * gcc.dg/vla-stexp-6.c: New test.
        * gcc.dg/vla-stexp-7.c: New test.
        * gcc.dg/vla-stexp-8.c: New test.
        * gcc.dg/vla-stexp-9.c: New test.
diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c
index 32c7e3e8972..a10b374dbed 100644
--- a/gcc/c-family/c-common.c
+++ b/gcc/c-family/c-common.c
@@ -3301,7 +3301,20 @@ pointer_int_sum (location_t loc, enum tree_code resultcode,
 				 TREE_TYPE (result_type)))
     size_exp = integer_one_node;
   else
-    size_exp = size_in_bytes_loc (loc, TREE_TYPE (result_type));
+    {
+      size_exp = size_in_bytes_loc (loc, TREE_TYPE (result_type));
+      /* Wrap the pointer expression in a SAVE_EXPR to make sure it
+       * is evaluated first when the size expression may depend
+       * on it for VM types.
+       */
+      if (TREE_SIDE_EFFECTS (size_exp)
+	  && variably_modified_type_p (TREE_TYPE (ptrop), NULL)
+	  && (VAR_DECL != TREE_CODE (ptrop)))
+	{
+	  ptrop = build1_loc (loc, SAVE_EXPR, TREE_TYPE (ptrop), ptrop);
+	  size_exp = build2 (COMPOUND_EXPR, TREE_TYPE (intop), ptrop, size_exp);
+	}
+    }
 
   /* We are manipulating pointer values, so we don't need to warn
      about relying on undefined signed overflow.  We disable the
diff --git a/gcc/gimplify.c b/gcc/gimplify.c
index 8bb54fd7481..7b6874a3142 100644
--- a/gcc/gimplify.c
+++ b/gcc/gimplify.c
@@ -2958,7 +2958,10 @@ gimplify_var_or_parm_decl (tree *expr_p)
      declaration, for which we've already issued an error.  It would
      be really nice if the front end wouldn't leak these at all.
      Currently the only known culprit is C++ destructors, as seen
-     in g++.old-deja/g++.jason/binding.C.  */
+     in g++.old-deja/g++.jason/binding.C.
+     Another possible culpit are size expressions for variably modified
+     types which are lost in the FE or not gimplified correctly.
+  */
   if (VAR_P (decl)
       && !DECL_SEEN_IN_BIND_EXPR_P (decl)
       && !TREE_STATIC (decl) && !DECL_EXTERNAL (decl)
@@ -3103,16 +3106,22 @@ gimplify_compound_lval (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p,
      expression until we deal with any variable bounds, sizes, or
      positions in order to deal with PLACEHOLDER_EXPRs.
 
-     So we do this in three steps.  First we deal with the annotations
-     for any variables in the components, then we gimplify the base,
-     then we gimplify any indices, from left to right.  */
+     The base expression may contain a statement expression that
+     has declarations used in size expressions, so has to be
+     gimplified before gimplifying the size expressions.
+
+     So we do this in three steps.  First we deal with variable
+     bounds, sizes, and positions, then we gimplify the base,
+     then we deal with the annotations for any variables in the
+     components and any indices, from left to right.  */
+
   for (i = expr_stack.length () - 1; i >= 0; i--)
     {
       tree t = expr_stack[i];
 
       if (TREE_CODE (t) == ARRAY_REF || TREE_CODE (t) == ARRAY_RANGE_REF)
 	{
-	  /* Gimplify the low bound and element type size and put them into
+	  /* Deal with the low bound and element type size and put them into
 	     the ARRAY_REF.  If these values are set, they have already been
 	     gimplified.  */
 	  if (TREE_OPERAND (t, 2) == NULL_TREE)
@@ -3121,18 +3130,8 @@ gimplify_compound_lval (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p,
 	      if (!is_gimple_min_invariant (low))
 		{
 		  TREE_OPERAND (t, 2) = low;
-		  tret = gimplify_expr (&TREE_OPERAND (t, 2), pre_p,
-					post_p, is_gimple_reg,
-					fb_rvalue);
-		  ret = MIN (ret, tret);
 		}
 	    }
-	  else
-	    {
-	      tret = gimplify_expr (&TREE_OPERAND (t, 2), pre_p, post_p,
-				    is_gimple_reg, fb_rvalue);
-	      ret = MIN (ret, tret);
-	    }
 
 	  if (TREE_OPERAND (t, 3) == NULL_TREE)
 	    {
@@ -3149,18 +3148,8 @@ gimplify_compound_lval (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p,
 					      elmt_size, factor);
 
 		  TREE_OPERAND (t, 3) = elmt_size;
-		  tret = gimplify_expr (&TREE_OPERAND (t, 3), pre_p,
-					post_p, is_gimple_reg,
-					fb_rvalue);
-		  ret = MIN (ret, tret);
 		}
 	    }
-	  else
-	    {
-	      tret = gimplify_expr (&TREE_OPERAND (t, 3), pre_p, post_p,
-				    is_gimple_reg, fb_rvalue);
-	      ret = MIN (ret, tret);
-	    }
 	}
       else if (TREE_CODE (t) == COMPONENT_REF)
 	{
@@ -3180,18 +3169,8 @@ gimplify_compound_lval (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p,
 					   offset, factor);
 
 		  TREE_OPERAND (t, 2) = offset;
-		  tret = gimplify_expr (&TREE_OPERAND (t, 2), pre_p,
-					post_p, is_gimple_reg,
-					fb_rvalue);
-		  ret = MIN (ret, tret);
 		}
 	    }
-	  else
-	    {
-	      tret = gimplify_expr (&TREE_OPERAND (t, 2), pre_p, post_p,
-				    is_gimple_reg, fb_rvalue);
-	      ret = MIN (ret, tret);
-	    }
 	}
     }
 
@@ -3202,21 +3181,34 @@ gimplify_compound_lval (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p,
 			fallback | fb_lvalue);
   ret = MIN (ret, tret);
 
-  /* And finally, the indices and operands of ARRAY_REF.  During this
-     loop we also remove any useless conversions.  */
+  /* Step 3: gimplify size expressions and the indices and operands of
+     ARRAY_REF.  During this loop we also remove any useless conversions.  */
+
   for (; expr_stack.length () > 0; )
     {
       tree t = expr_stack.pop ();
 
       if (TREE_CODE (t) == ARRAY_REF || TREE_CODE (t) == ARRAY_RANGE_REF)
 	{
+	  /* Gimplify the low bound and element type size. */
+	  tret = gimplify_expr (&TREE_OPERAND (t, 2), pre_p, post_p,
+				is_gimple_reg, fb_rvalue);
+	  ret = MIN (ret, tret);
+
+	  tret = gimplify_expr (&TREE_OPERAND (t, 3), pre_p, post_p,
+				is_gimple_reg, fb_rvalue);
+	  ret = MIN (ret, tret);
+
 	  /* Gimplify the dimension.  */
-	  if (!is_gimple_min_invariant (TREE_OPERAND (t, 1)))
-	    {
-	      tret = gimplify_expr (&TREE_OPERAND (t, 1), pre_p, post_p,
-				    is_gimple_val, fb_rvalue);
-	      ret = MIN (ret, tret);
-	    }
+	  tret = gimplify_expr (&TREE_OPERAND (t, 1), pre_p, post_p,
+				is_gimple_val, fb_rvalue);
+	  ret = MIN (ret, tret);
+	}
+      else if (TREE_CODE (t) == COMPONENT_REF)
+	{
+	  tret = gimplify_expr (&TREE_OPERAND (t, 2), pre_p, post_p,
+				is_gimple_reg, fb_rvalue);
+	  ret = MIN (ret, tret);
 	}
 
       STRIP_USELESS_TYPE_CONVERSION (TREE_OPERAND (t, 0));
@@ -6914,6 +6906,8 @@ gimplify_target_expr (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p)
 	{
 	  if (!TYPE_SIZES_GIMPLIFIED (TREE_TYPE (temp)))
 	    gimplify_type_sizes (TREE_TYPE (temp), pre_p);
+	  /* FIXME: this is correct only when the size of the type does
+	     not depend on expressions evaluated in init. */
 	  gimplify_vla_decl (temp, pre_p);
 	}
       else
diff --git a/gcc/testsuite/gcc.dg/vla-stexp-3.c b/gcc/testsuite/gcc.dg/vla-stexp-3.c
new file mode 100644
index 00000000000..e663de1cd72
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/vla-stexp-3.c
@@ -0,0 +1,11 @@
+/* PR91038 */
+/* { dg-do compile } */
+/* { dg-options "" } */
+
+
+void bar(void)
+{
+	({ int N = 2; int (*x)[9][N] = 0; x; })[1];
+	({ int N = 2; int (*x)[9][N] = 0; x; })[0];	// should not ice
+}
+
diff --git a/gcc/testsuite/gcc.dg/vla-stexp-4.c b/gcc/testsuite/gcc.dg/vla-stexp-4.c
new file mode 100644
index 00000000000..612b5a802fc
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/vla-stexp-4.c
@@ -0,0 +1,94 @@
+/* PR29970, PR91038 */
+/* { dg-do run } */
+/* { dg-options "-O0 -Wunused-variable" } */
+
+int foo3b(void)   // should not return 0
+{
+        int n = 0;
+        return sizeof *({ n = 10; int x[n]; &x; });
+}
+
+int foo4(void)   // should not ICE
+{
+        return (*({
+                        int n = 20;
+                        char (*x)[n][n] = __builtin_malloc(n * n);
+                        (*x)[12][1] = 1;
+                        x;
+                }))[12][1];
+}
+
+int foo5(void)   // should return 1, returns 0
+{
+        int n = 0;
+        return (*({
+                        n = 20;
+                        char (*x)[n][n] = __builtin_malloc(n * n);
+                        (*x)[12][1] = 1;
+                        (*x)[0][1] = 0;
+                        x;
+                }))[12][1];
+}
+
+int foo5c(void)   // should return 400 
+{
+        int n = 0;
+        return sizeof(*({
+                        n = 20;
+                        char (*x)[n][n] = __builtin_malloc(n * n);
+                        (*x)[12][1] = 1;
+                        (*x)[0][1] = 0;
+                        x;
+                }));
+}
+
+int foo5b(void)   // should return 1, returns 0
+{
+	int n = 0;			/* { dg-warning "unused variable" } */
+        return (*({
+                        int n = 20;
+                        char (*x)[n][n] = __builtin_malloc(n * n);
+                        (*x)[12][1] = 1;
+                        (*x)[0][1] = 0;
+                        x;
+                }))[12][1];
+}
+
+int foo5a(void)   // should return 1, returns 0
+{
+        return (*({
+                        int n = 20;
+                        char (*x)[n][n] = __builtin_malloc(n * n);
+                        (*x)[12][1] = 1;
+                        (*x)[0][1] = 0;
+                        x;
+                }))[12][1];
+}
+
+
+
+
+int main()
+{
+	if (sizeof(int[10]) != foo3b())
+		__builtin_abort();
+
+	if (1 != foo4())
+		__builtin_abort();
+
+	if (400 != foo5c())
+		__builtin_abort();
+
+	if (1 != foo5a())
+		__builtin_abort();
+
+	if (1 != foo5b()) // -O0
+		__builtin_abort();
+
+	if (1 != foo5())
+		__builtin_abort();
+
+	return 0;
+}
+
+
diff --git a/gcc/testsuite/gcc.dg/vla-stexp-5.c b/gcc/testsuite/gcc.dg/vla-stexp-5.c
new file mode 100644
index 00000000000..d6a7f2b34b8
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/vla-stexp-5.c
@@ -0,0 +1,30 @@
+/* PR29970 */
+/* { dg-do run } */
+/* { dg-options "-Wunused-variable" } */
+
+
+
+
+int foo2a(void)   // should not ICE
+{
+        return ({ int n = 20; struct { int x[n];} x; x.x[12] = 1; sizeof(x); });
+}
+
+
+int foo2b(void)   // should not ICE
+{
+        return sizeof *({ int n = 20; struct { int x[n];} x; x.x[12] = 1; &x; });
+}
+
+int main()
+{
+	if (sizeof(struct { int x[20]; }) != foo2a())
+		__builtin_abort();
+
+	if (sizeof(struct { int x[20]; }) != foo2b())
+		__builtin_abort();
+
+	return 0;
+}
+
+
diff --git a/gcc/testsuite/gcc.dg/vla-stexp-6.c b/gcc/testsuite/gcc.dg/vla-stexp-6.c
new file mode 100644
index 00000000000..3d96d38898b
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/vla-stexp-6.c
@@ -0,0 +1,94 @@
+/* PR29970, PR91038 */
+/* { dg-do run } */
+/* { dg-options "-O2 -Wunused-variable" } */
+
+int foo3b(void)   // should not return 0
+{
+        int n = 0;
+        return sizeof *({ n = 10; int x[n]; &x; });
+}
+
+int foo4(void)   // should not ICE
+{
+        return (*({
+                        int n = 20;
+                        char (*x)[n][n] = __builtin_malloc(n * n);
+                        (*x)[12][1] = 1;
+                        x;
+                }))[12][1];
+}
+
+int foo5(void)   // should return 1, returns 0
+{
+        int n = 0;
+        return (*({
+                        n = 20;
+                        char (*x)[n][n] = __builtin_malloc(n * n);
+                        (*x)[12][1] = 1;
+                        (*x)[0][1] = 0;
+                        x;
+                }))[12][1];
+}
+
+int foo5c(void)   // should return 400 
+{
+        int n = 0;
+        return sizeof(*({
+                        n = 20;
+                        char (*x)[n][n] = __builtin_malloc(n * n);
+                        (*x)[12][1] = 1;
+                        (*x)[0][1] = 0;
+                        x;
+                }));
+}
+
+int foo5b(void)   // should return 1, returns 0
+{
+	int n = 0;	/* { dg-warning "unused variable" } */
+        return (*({
+                        int n = 20;
+                        char (*x)[n][n] = __builtin_malloc(n * n);
+                        (*x)[12][1] = 1;
+                        (*x)[0][1] = 0;
+                        x;
+                }))[12][1];
+}
+
+int foo5a(void)   // should return 1, returns 0
+{
+        return (*({
+                        int n = 20;
+                        char (*x)[n][n] = __builtin_malloc(n * n);
+                        (*x)[12][1] = 1;
+                        (*x)[0][1] = 0;
+                        x;
+                }))[12][1];
+}
+
+
+
+
+int main()
+{
+	if (sizeof(int[10]) != foo3b())
+		__builtin_abort();
+
+	if (1 != foo4())
+		__builtin_abort();
+
+	if (400 != foo5c())
+		__builtin_abort();
+
+	if (1 != foo5a())
+		__builtin_abort();
+
+	if (1 != foo5b()) // -O0
+		__builtin_abort();
+
+	if (1 != foo5())
+		__builtin_abort();
+
+	return 0;
+}
+
+
diff --git a/gcc/testsuite/gcc.dg/vla-stexp-7.c b/gcc/testsuite/gcc.dg/vla-stexp-7.c
new file mode 100644
index 00000000000..3091b9184c2
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/vla-stexp-7.c
@@ -0,0 +1,44 @@
+/* PR91038 */
+/* { dg-do run } */
+/* { dg-options "-O2 -Wunused-variable" } */
+
+
+#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
+
+struct lbm {
+
+	int D;
+	const int* DQ;
+
+} D2Q9 = { 2,
+	(const int*)&(const int[9][2]){
+		{ 0, 0 },
+		{ 1, 0 }, { 0, 1 }, { -1, 0 }, { 0, -1 },
+		{ 1, 1 }, { -1, 1 }, { -1, -1 }, { 1, -1 },
+	}
+};
+
+void zouhe_left(void)
+{
+	__auto_type xx = (*({ int N = 2; struct lbm __x = D2Q9; ((const int(*)[9][N])__x.DQ); }));
+
+	if (1 != xx[1][0])
+		__builtin_abort();
+
+	if (2 != ARRAY_SIZE(xx[1]))
+		__builtin_abort();
+
+	if (1 != (*({ int N = 2; struct lbm __x = D2Q9; ((const int(*)[9][N])__x.DQ); }))[1][0])
+		__builtin_abort();
+
+	if (2 != ARRAY_SIZE(*({ int N = 2; struct lbm __x = D2Q9; ((const int(*)[9][N])__x.DQ);
})[1]))
+		__builtin_abort();
+}
+
+int main()
+{
+	zouhe_left();
+	return 0;
+}
+
+
diff --git a/gcc/testsuite/gcc.dg/vla-stexp-8.c b/gcc/testsuite/gcc.dg/vla-stexp-8.c
new file mode 100644
index 00000000000..5b475eb6cf2
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/vla-stexp-8.c
@@ -0,0 +1,47 @@
+/* PR29970, PR91038 */
+/* { dg-do compile } */
+/* { dg-options "-O2 -Wunused-variable" } */
+
+
+int foo0(void)
+{
+	int c = *(*(*({ int n = 10; int (*x)[n][n] = __builtin_malloc(sizeof *x); x; }) + 5) + 5);
+	return c;
+}
+
+int foo1(void)
+{
+	int c = *(5 + *(5 + *({ int n = 10; int (*x)[n][n] = __builtin_malloc(sizeof *x); x; })));
+	return c;
+}
+
+int bar2(void)
+{
+	int c = (*({ int n = 10; struct { int y[n]; int z; }* x = __builtin_malloc(sizeof *x); x;
})).z;
+	return c;
+}
+
+int bar3(void)
+{
+	int n = 2;	/* { dg-warning "unused variable" } */
+	int c = (*({ int n = 3; 	/* { dg-warning "unused variable" } */
+		({ int n = 10; int (*x)[n][n] = __builtin_malloc(sizeof *x); x; }); }))[5][5];
+	return c;
+}
+
+int bar3b(void)
+{
+	int n = 2;	/* { dg-warning "unused variable" } */
+	int c = (*({ int n = 3; 	/* { dg-warning "unused variable" } */
+		({ int n = 10; int (*x)[n][n] = __builtin_malloc(sizeof *x); x; }); }))[0][0];
+	return c;
+}
+
+int bar4(void)
+{
+	int n = 2;	/* { dg-warning "unused variable" } */
+	int c = *(5 + *( 5 + *({ int n = 3;	/* { dg-warning "unused variable" } */
+		({ int n = 10; int (*x)[n][n] = __builtin_malloc(sizeof *x); x; }); })));
+	return c;
+}
+
diff --git a/gcc/testsuite/gcc.dg/vla-stexp-9.c b/gcc/testsuite/gcc.dg/vla-stexp-9.c
new file mode 100644
index 00000000000..3593a790785
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/vla-stexp-9.c
@@ -0,0 +1,53 @@
+/* PR91038 */
+/* { dg-do run } */
+/* { dg-options "-O2 -Wunused-variable" } */
+
+
+
+void foo(void)
+{
+	if (2 * sizeof(int) != sizeof((*({ int N = 2; int (*x)[9][N] = 0; x; })[1])))
+		__builtin_abort();
+}
+
+void bar(void)
+{
+	if (2 * sizeof(int) != sizeof((*({ int N = 2; int (*x)[9][N] = 0; x; })[0])))
+		__builtin_abort();
+}
+
+void bar0(void)
+{
+	if (2 * 9 *  sizeof(int) != sizeof((*({ int N = 2; int (*x)[9][N] = 0; x; }))))
+		__builtin_abort();
+}
+
+void bar11(void)
+{
+	sizeof(*((*({ int N = 2; int (*x)[9][N] = 0; x; }) + 0)));
+}
+
+void bar12(void)
+{
+	if (2 * sizeof(int) != sizeof(*((*({ int N = 2; int (*x)[9][N] = 0; x; })    ))))
+		__builtin_abort();
+}
+
+void bar1(void)
+{
+	if (2 * sizeof(int) != sizeof(*((*({ int N = 2; int (*x)[9][N] = 0; x; }) + 0))))
+		__builtin_abort();
+}
+
+
+
+
+int main()
+{
+	foo();
+	bar0();
+	bar12();
+	bar1();
+	bar();
+}
+

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

[Jason Merrill]

On 10/31/21 05:22, Uecker, Martin wrote:
> 
> Hi Jason,
> 
> here is the fourth version of the patch.
> 
> I followed your suggestion and now make this
> transformation sooner in pointer_int_sum.
> I also added a check to only do this
> transformation when the pointer is not a
> VAR_DECL, which avoids it in the most
> common cases where it is not necessary.
> 
> Looking for BIND_EXPR seems complicated
> and I could not convince myself that it is
> worth it.  I also see the risk that this
> makes potential failure cases even more
> subtle. What do you think?

That makes sense.  Though see some minor comments below.

> Bootstrapped and regression tested
> on x86-64 for all languages.
> 
> 
> 
> Martin
> 
> 
> 
> 
> Fix ICE when mixing VLAs and statement expressions [PR91038]
> 
> When returning VM-types from statement expressions, this can
> lead to an ICE when declarations from the statement expression
> are referred to later. Most of these issues can be addressed by
> gimplifying the base expression earlier in gimplify_compound_lval.
> Another issue is fixed by wrapping the pointer expression in
> pointer_int_sum. This fixes PR91038 and some of the test cases
> from PR29970 (structs with VLA members need further work).
> 
>      
>      2021-10-30  Martin Uecker  <muecker@gwdg.de>
>      
>      gcc/
>          PR c/91038
>          PR c/29970
> 	* c-family/c-common.c (pointer_int_sum): Make sure
> 	pointer expressions are evaluated first when the size
> 	expression depends on for variably-modified types.
>          * gimplify.c (gimplify_var_or_parm_decl): Update comment.
>          (gimplify_compound_lval): Gimplify base expression first.
>          (gimplify_target_expr): Add comment.
>      
>      gcc/testsuite/
>          PR c/91038
>          PR c/29970
>          * gcc.dg/vla-stexp-3.c: New test.
>          * gcc.dg/vla-stexp-4.c: New test.
>          * gcc.dg/vla-stexp-5.c: New test.
>          * gcc.dg/vla-stexp-6.c: New test.
>          * gcc.dg/vla-stexp-7.c: New test.
>          * gcc.dg/vla-stexp-8.c: New test.
>          * gcc.dg/vla-stexp-9.c: New test.
> diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c
> index 32c7e3e8972..a10b374dbed 100644
> --- a/gcc/c-family/c-common.c
> +++ b/gcc/c-family/c-common.c
> @@ -3301,7 +3301,20 @@ pointer_int_sum (location_t loc, enum tree_code resultcode,
>   				 TREE_TYPE (result_type)))
>       size_exp = integer_one_node;
>     else
> -    size_exp = size_in_bytes_loc (loc, TREE_TYPE (result_type));
> +    {
> +      size_exp = size_in_bytes_loc (loc, TREE_TYPE (result_type));
> +      /* Wrap the pointer expression in a SAVE_EXPR to make sure it
> +       * is evaluated first when the size expression may depend
> +       * on it for VM types.
> +       */

We usually don't put * at the beginning of new comment lines, and we put 
the closing */ at the end of the last line of text.

> +      if (TREE_SIDE_EFFECTS (size_exp)
> +	  && variably_modified_type_p (TREE_TYPE (ptrop), NULL)
> +	  && (VAR_DECL != TREE_CODE (ptrop)))

How about checking TREE_SIDE_EFFECTS (ptrop) instead of TREE_CODE to 
avoid this in more cases?

> +	{
> +	  ptrop = build1_loc (loc, SAVE_EXPR, TREE_TYPE (ptrop), ptrop);

Why not use save_expr()?

> +	  size_exp = build2 (COMPOUND_EXPR, TREE_TYPE (intop), ptrop, size_exp);
> +	}
> +    }
>   
>     /* We are manipulating pointer values, so we don't need to warn
>        about relying on undefined signed overflow.  We disable the
> diff --git a/gcc/gimplify.c b/gcc/gimplify.c
> index 8bb54fd7481..7b6874a3142 100644
> --- a/gcc/gimplify.c
> +++ b/gcc/gimplify.c
> @@ -2958,7 +2958,10 @@ gimplify_var_or_parm_decl (tree *expr_p)
>        declaration, for which we've already issued an error.  It would
>        be really nice if the front end wouldn't leak these at all.
>        Currently the only known culprit is C++ destructors, as seen
> -     in g++.old-deja/g++.jason/binding.C.  */
> +     in g++.old-deja/g++.jason/binding.C.
> +     Another possible culpit are size expressions for variably modified
> +     types which are lost in the FE or not gimplified correctly.
> +  */

Again, */ on the last line of text.

>     if (VAR_P (decl)
>         && !DECL_SEEN_IN_BIND_EXPR_P (decl)
>         && !TREE_STATIC (decl) && !DECL_EXTERNAL (decl)
> @@ -3103,16 +3106,22 @@ gimplify_compound_lval (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p,
>        expression until we deal with any variable bounds, sizes, or
>        positions in order to deal with PLACEHOLDER_EXPRs.
>   
> -     So we do this in three steps.  First we deal with the annotations
> -     for any variables in the components, then we gimplify the base,
> -     then we gimplify any indices, from left to right.  */
> +     The base expression may contain a statement expression that
> +     has declarations used in size expressions, so has to be
> +     gimplified before gimplifying the size expressions.
> +
> +     So we do this in three steps.  First we deal with variable
> +     bounds, sizes, and positions, then we gimplify the base,
> +     then we deal with the annotations for any variables in the
> +     components and any indices, from left to right.  */
> +
>     for (i = expr_stack.length () - 1; i >= 0; i--)
>       {
>         tree t = expr_stack[i];
>   
>         if (TREE_CODE (t) == ARRAY_REF || TREE_CODE (t) == ARRAY_RANGE_REF)
>   	{
> -	  /* Gimplify the low bound and element type size and put them into
> +	  /* Deal with the low bound and element type size and put them into
>   	     the ARRAY_REF.  If these values are set, they have already been
>   	     gimplified.  */
>   	  if (TREE_OPERAND (t, 2) == NULL_TREE)
> @@ -3121,18 +3130,8 @@ gimplify_compound_lval (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p,
>   	      if (!is_gimple_min_invariant (low))
>   		{
>   		  TREE_OPERAND (t, 2) = low;
> -		  tret = gimplify_expr (&TREE_OPERAND (t, 2), pre_p,
> -					post_p, is_gimple_reg,
> -					fb_rvalue);
> -		  ret = MIN (ret, tret);
>   		}
>   	    }
> -	  else
> -	    {
> -	      tret = gimplify_expr (&TREE_OPERAND (t, 2), pre_p, post_p,
> -				    is_gimple_reg, fb_rvalue);
> -	      ret = MIN (ret, tret);
> -	    }
>   
>   	  if (TREE_OPERAND (t, 3) == NULL_TREE)
>   	    {
> @@ -3149,18 +3148,8 @@ gimplify_compound_lval (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p,
>   					      elmt_size, factor);
>   
>   		  TREE_OPERAND (t, 3) = elmt_size;
> -		  tret = gimplify_expr (&TREE_OPERAND (t, 3), pre_p,
> -					post_p, is_gimple_reg,
> -					fb_rvalue);
> -		  ret = MIN (ret, tret);
>   		}
>   	    }
> -	  else
> -	    {
> -	      tret = gimplify_expr (&TREE_OPERAND (t, 3), pre_p, post_p,
> -				    is_gimple_reg, fb_rvalue);
> -	      ret = MIN (ret, tret);
> -	    }
>   	}
>         else if (TREE_CODE (t) == COMPONENT_REF)
>   	{
> @@ -3180,18 +3169,8 @@ gimplify_compound_lval (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p,
>   					   offset, factor);
>   
>   		  TREE_OPERAND (t, 2) = offset;
> -		  tret = gimplify_expr (&TREE_OPERAND (t, 2), pre_p,
> -					post_p, is_gimple_reg,
> -					fb_rvalue);
> -		  ret = MIN (ret, tret);
>   		}
>   	    }
> -	  else
> -	    {
> -	      tret = gimplify_expr (&TREE_OPERAND (t, 2), pre_p, post_p,
> -				    is_gimple_reg, fb_rvalue);
> -	      ret = MIN (ret, tret);
> -	    }
>   	}
>       }
>   
> @@ -3202,21 +3181,34 @@ gimplify_compound_lval (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p,
>   			fallback | fb_lvalue);
>     ret = MIN (ret, tret);
>   
> -  /* And finally, the indices and operands of ARRAY_REF.  During this
> -     loop we also remove any useless conversions.  */
> +  /* Step 3: gimplify size expressions and the indices and operands of
> +     ARRAY_REF.  During this loop we also remove any useless conversions.  */
> +
>     for (; expr_stack.length () > 0; )
>       {
>         tree t = expr_stack.pop ();
>   
>         if (TREE_CODE (t) == ARRAY_REF || TREE_CODE (t) == ARRAY_RANGE_REF)
>   	{
> +	  /* Gimplify the low bound and element type size. */
> +	  tret = gimplify_expr (&TREE_OPERAND (t, 2), pre_p, post_p,
> +				is_gimple_reg, fb_rvalue);
> +	  ret = MIN (ret, tret);
> +
> +	  tret = gimplify_expr (&TREE_OPERAND (t, 3), pre_p, post_p,
> +				is_gimple_reg, fb_rvalue);
> +	  ret = MIN (ret, tret);
> +
>   	  /* Gimplify the dimension.  */
> -	  if (!is_gimple_min_invariant (TREE_OPERAND (t, 1)))
> -	    {
> -	      tret = gimplify_expr (&TREE_OPERAND (t, 1), pre_p, post_p,
> -				    is_gimple_val, fb_rvalue);
> -	      ret = MIN (ret, tret);
> -	    }
> +	  tret = gimplify_expr (&TREE_OPERAND (t, 1), pre_p, post_p,
> +				is_gimple_val, fb_rvalue);
> +	  ret = MIN (ret, tret);
> +	}
> +      else if (TREE_CODE (t) == COMPONENT_REF)
> +	{
> +	  tret = gimplify_expr (&TREE_OPERAND (t, 2), pre_p, post_p,
> +				is_gimple_reg, fb_rvalue);
> +	  ret = MIN (ret, tret);
>   	}
>   
>         STRIP_USELESS_TYPE_CONVERSION (TREE_OPERAND (t, 0));
> @@ -6914,6 +6906,8 @@ gimplify_target_expr (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p)
>   	{
>   	  if (!TYPE_SIZES_GIMPLIFIED (TREE_TYPE (temp)))
>   	    gimplify_type_sizes (TREE_TYPE (temp), pre_p);
> +	  /* FIXME: this is correct only when the size of the type does
> +	     not depend on expressions evaluated in init. */
>   	  gimplify_vla_decl (temp, pre_p);
>   	}
>         else
> diff --git a/gcc/testsuite/gcc.dg/vla-stexp-3.c b/gcc/testsuite/gcc.dg/vla-stexp-3.c
> new file mode 100644
> index 00000000000..e663de1cd72
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/vla-stexp-3.c
> @@ -0,0 +1,11 @@
> +/* PR91038 */
> +/* { dg-do compile } */
> +/* { dg-options "" } */
> +
> +
> +void bar(void)
> +{
> +	({ int N = 2; int (*x)[9][N] = 0; x; })[1];
> +	({ int N = 2; int (*x)[9][N] = 0; x; })[0];	// should not ice
> +}
> +
> diff --git a/gcc/testsuite/gcc.dg/vla-stexp-4.c b/gcc/testsuite/gcc.dg/vla-stexp-4.c
> new file mode 100644
> index 00000000000..612b5a802fc
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/vla-stexp-4.c
> @@ -0,0 +1,94 @@
> +/* PR29970, PR91038 */
> +/* { dg-do run } */
> +/* { dg-options "-O0 -Wunused-variable" } */
> +
> +int foo3b(void)   // should not return 0
> +{
> +        int n = 0;
> +        return sizeof *({ n = 10; int x[n]; &x; });
> +}
> +
> +int foo4(void)   // should not ICE
> +{
> +        return (*({
> +                        int n = 20;
> +                        char (*x)[n][n] = __builtin_malloc(n * n);
> +                        (*x)[12][1] = 1;
> +                        x;
> +                }))[12][1];
> +}
> +
> +int foo5(void)   // should return 1, returns 0
> +{
> +        int n = 0;
> +        return (*({
> +                        n = 20;
> +                        char (*x)[n][n] = __builtin_malloc(n * n);
> +                        (*x)[12][1] = 1;
> +                        (*x)[0][1] = 0;
> +                        x;
> +                }))[12][1];
> +}
> +
> +int foo5c(void)   // should return 400
> +{
> +        int n = 0;
> +        return sizeof(*({
> +                        n = 20;
> +                        char (*x)[n][n] = __builtin_malloc(n * n);
> +                        (*x)[12][1] = 1;
> +                        (*x)[0][1] = 0;
> +                        x;
> +                }));
> +}
> +
> +int foo5b(void)   // should return 1, returns 0
> +{
> +	int n = 0;			/* { dg-warning "unused variable" } */
> +        return (*({
> +                        int n = 20;
> +                        char (*x)[n][n] = __builtin_malloc(n * n);
> +                        (*x)[12][1] = 1;
> +                        (*x)[0][1] = 0;
> +                        x;
> +                }))[12][1];
> +}
> +
> +int foo5a(void)   // should return 1, returns 0
> +{
> +        return (*({
> +                        int n = 20;
> +                        char (*x)[n][n] = __builtin_malloc(n * n);
> +                        (*x)[12][1] = 1;
> +                        (*x)[0][1] = 0;
> +                        x;
> +                }))[12][1];
> +}
> +
> +
> +
> +
> +int main()
> +{
> +	if (sizeof(int[10]) != foo3b())
> +		__builtin_abort();
> +
> +	if (1 != foo4())
> +		__builtin_abort();
> +
> +	if (400 != foo5c())
> +		__builtin_abort();
> +
> +	if (1 != foo5a())
> +		__builtin_abort();
> +
> +	if (1 != foo5b()) // -O0
> +		__builtin_abort();
> +
> +	if (1 != foo5())
> +		__builtin_abort();
> +
> +	return 0;
> +}
> +
> +
> diff --git a/gcc/testsuite/gcc.dg/vla-stexp-5.c b/gcc/testsuite/gcc.dg/vla-stexp-5.c
> new file mode 100644
> index 00000000000..d6a7f2b34b8
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/vla-stexp-5.c
> @@ -0,0 +1,30 @@
> +/* PR29970 */
> +/* { dg-do run } */
> +/* { dg-options "-Wunused-variable" } */
> +
> +
> +
> +
> +int foo2a(void)   // should not ICE
> +{
> +        return ({ int n = 20; struct { int x[n];} x; x.x[12] = 1; sizeof(x); });
> +}
> +
> +
> +int foo2b(void)   // should not ICE
> +{
> +        return sizeof *({ int n = 20; struct { int x[n];} x; x.x[12] = 1; &x; });
> +}
> +
> +int main()
> +{
> +	if (sizeof(struct { int x[20]; }) != foo2a())
> +		__builtin_abort();
> +
> +	if (sizeof(struct { int x[20]; }) != foo2b())
> +		__builtin_abort();
> +
> +	return 0;
> +}
> +
> +
> diff --git a/gcc/testsuite/gcc.dg/vla-stexp-6.c b/gcc/testsuite/gcc.dg/vla-stexp-6.c
> new file mode 100644
> index 00000000000..3d96d38898b
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/vla-stexp-6.c
> @@ -0,0 +1,94 @@
> +/* PR29970, PR91038 */
> +/* { dg-do run } */
> +/* { dg-options "-O2 -Wunused-variable" } */
> +
> +int foo3b(void)   // should not return 0
> +{
> +        int n = 0;
> +        return sizeof *({ n = 10; int x[n]; &x; });
> +}
> +
> +int foo4(void)   // should not ICE
> +{
> +        return (*({
> +                        int n = 20;
> +                        char (*x)[n][n] = __builtin_malloc(n * n);
> +                        (*x)[12][1] = 1;
> +                        x;
> +                }))[12][1];
> +}
> +
> +int foo5(void)   // should return 1, returns 0
> +{
> +        int n = 0;
> +        return (*({
> +                        n = 20;
> +                        char (*x)[n][n] = __builtin_malloc(n * n);
> +                        (*x)[12][1] = 1;
> +                        (*x)[0][1] = 0;
> +                        x;
> +                }))[12][1];
> +}
> +
> +int foo5c(void)   // should return 400
> +{
> +        int n = 0;
> +        return sizeof(*({
> +                        n = 20;
> +                        char (*x)[n][n] = __builtin_malloc(n * n);
> +                        (*x)[12][1] = 1;
> +                        (*x)[0][1] = 0;
> +                        x;
> +                }));
> +}
> +
> +int foo5b(void)   // should return 1, returns 0
> +{
> +	int n = 0;	/* { dg-warning "unused variable" } */
> +        return (*({
> +                        int n = 20;
> +                        char (*x)[n][n] = __builtin_malloc(n * n);
> +                        (*x)[12][1] = 1;
> +                        (*x)[0][1] = 0;
> +                        x;
> +                }))[12][1];
> +}
> +
> +int foo5a(void)   // should return 1, returns 0
> +{
> +        return (*({
> +                        int n = 20;
> +                        char (*x)[n][n] = __builtin_malloc(n * n);
> +                        (*x)[12][1] = 1;
> +                        (*x)[0][1] = 0;
> +                        x;
> +                }))[12][1];
> +}
> +
> +
> +
> +
> +int main()
> +{
> +	if (sizeof(int[10]) != foo3b())
> +		__builtin_abort();
> +
> +	if (1 != foo4())
> +		__builtin_abort();
> +
> +	if (400 != foo5c())
> +		__builtin_abort();
> +
> +	if (1 != foo5a())
> +		__builtin_abort();
> +
> +	if (1 != foo5b()) // -O0
> +		__builtin_abort();
> +
> +	if (1 != foo5())
> +		__builtin_abort();
> +
> +	return 0;
> +}
> +
> +
> diff --git a/gcc/testsuite/gcc.dg/vla-stexp-7.c b/gcc/testsuite/gcc.dg/vla-stexp-7.c
> new file mode 100644
> index 00000000000..3091b9184c2
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/vla-stexp-7.c
> @@ -0,0 +1,44 @@
> +/* PR91038 */
> +/* { dg-do run } */
> +/* { dg-options "-O2 -Wunused-variable" } */
> +
> +
> +#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
> +
> +struct lbm {
> +
> +	int D;
> +	const int* DQ;
> +
> +} D2Q9 = { 2,
> +	(const int*)&(const int[9][2]){
> +		{ 0, 0 },
> +		{ 1, 0 }, { 0, 1 }, { -1, 0 }, { 0, -1 },
> +		{ 1, 1 }, { -1, 1 }, { -1, -1 }, { 1, -1 },
> +	}
> +};
> +
> +void zouhe_left(void)
> +{
> +	__auto_type xx = (*({ int N = 2; struct lbm __x = D2Q9; ((const int(*)[9][N])__x.DQ); }));
> +
> +	if (1 != xx[1][0])
> +		__builtin_abort();
> +
> +	if (2 != ARRAY_SIZE(xx[1]))
> +		__builtin_abort();
> +
> +	if (1 != (*({ int N = 2; struct lbm __x = D2Q9; ((const int(*)[9][N])__x.DQ); }))[1][0])
> +		__builtin_abort();
> +
> +	if (2 != ARRAY_SIZE(*({ int N = 2; struct lbm __x = D2Q9; ((const int(*)[9][N])__x.DQ);
> })[1]))
> +		__builtin_abort();
> +}
> +
> +int main()
> +{
> +	zouhe_left();
> +	return 0;
> +}
> +
> +
> diff --git a/gcc/testsuite/gcc.dg/vla-stexp-8.c b/gcc/testsuite/gcc.dg/vla-stexp-8.c
> new file mode 100644
> index 00000000000..5b475eb6cf2
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/vla-stexp-8.c
> @@ -0,0 +1,47 @@
> +/* PR29970, PR91038 */
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -Wunused-variable" } */
> +
> +
> +int foo0(void)
> +{
> +	int c = *(*(*({ int n = 10; int (*x)[n][n] = __builtin_malloc(sizeof *x); x; }) + 5) + 5);
> +	return c;
> +}
> +
> +int foo1(void)
> +{
> +	int c = *(5 + *(5 + *({ int n = 10; int (*x)[n][n] = __builtin_malloc(sizeof *x); x; })));
> +	return c;
> +}
> +
> +int bar2(void)
> +{
> +	int c = (*({ int n = 10; struct { int y[n]; int z; }* x = __builtin_malloc(sizeof *x); x;
> })).z;
> +	return c;
> +}
> +
> +int bar3(void)
> +{
> +	int n = 2;	/* { dg-warning "unused variable" } */
> +	int c = (*({ int n = 3; 	/* { dg-warning "unused variable" } */
> +		({ int n = 10; int (*x)[n][n] = __builtin_malloc(sizeof *x); x; }); }))[5][5];
> +	return c;
> +}
> +
> +int bar3b(void)
> +{
> +	int n = 2;	/* { dg-warning "unused variable" } */
> +	int c = (*({ int n = 3; 	/* { dg-warning "unused variable" } */
> +		({ int n = 10; int (*x)[n][n] = __builtin_malloc(sizeof *x); x; }); }))[0][0];
> +	return c;
> +}
> +
> +int bar4(void)
> +{
> +	int n = 2;	/* { dg-warning "unused variable" } */
> +	int c = *(5 + *( 5 + *({ int n = 3;	/* { dg-warning "unused variable" } */
> +		({ int n = 10; int (*x)[n][n] = __builtin_malloc(sizeof *x); x; }); })));
> +	return c;
> +}
> +
> diff --git a/gcc/testsuite/gcc.dg/vla-stexp-9.c b/gcc/testsuite/gcc.dg/vla-stexp-9.c
> new file mode 100644
> index 00000000000..3593a790785
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/vla-stexp-9.c
> @@ -0,0 +1,53 @@
> +/* PR91038 */
> +/* { dg-do run } */
> +/* { dg-options "-O2 -Wunused-variable" } */
> +
> +
> +
> +void foo(void)
> +{
> +	if (2 * sizeof(int) != sizeof((*({ int N = 2; int (*x)[9][N] = 0; x; })[1])))
> +		__builtin_abort();
> +}
> +
> +void bar(void)
> +{
> +	if (2 * sizeof(int) != sizeof((*({ int N = 2; int (*x)[9][N] = 0; x; })[0])))
> +		__builtin_abort();
> +}
> +
> +void bar0(void)
> +{
> +	if (2 * 9 *  sizeof(int) != sizeof((*({ int N = 2; int (*x)[9][N] = 0; x; }))))
> +		__builtin_abort();
> +}
> +
> +void bar11(void)
> +{
> +	sizeof(*((*({ int N = 2; int (*x)[9][N] = 0; x; }) + 0)));
> +}
> +
> +void bar12(void)
> +{
> +	if (2 * sizeof(int) != sizeof(*((*({ int N = 2; int (*x)[9][N] = 0; x; })    ))))
> +		__builtin_abort();
> +}
> +
> +void bar1(void)
> +{
> +	if (2 * sizeof(int) != sizeof(*((*({ int N = 2; int (*x)[9][N] = 0; x; }) + 0))))
> +		__builtin_abort();
> +}
> +
> +
> +
> +
> +int main()
> +{
> +	foo();
> +	bar0();
> +	bar12();
> +	bar1();
> +	bar();
> +}
> +
> 

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

[Uecker, Martin]

Am Mittwoch, den 03.11.2021, 10:18 -0400 schrieb Jason Merrill:
> On 10/31/21 05:22, Uecker, Martin wrote:
> > Hi Jason,
> > 
> > here is the fourth version of the patch.
> > 
> > I followed your suggestion and now make this
> > transformation sooner in pointer_int_sum.
> > I also added a check to only do this
> > transformation when the pointer is not a
> > VAR_DECL, which avoids it in the most
> > common cases where it is not necessary.
> > 
> > Looking for BIND_EXPR seems complicated
> > and I could not convince myself that it is
> > worth it.  I also see the risk that this
> > makes potential failure cases even more
> > subtle. What do you think?
> 
> That makes sense.  Though see some minor comments below.

Thank you! I made these changes and ran
bootstrap and tests again.

Ok for trunk?


Any idea how to fix returning structs with 
VLA member from statement expressions? 

Otherwise, I will add an error message to
the FE in another patch. 

Martin


> > 
> > Fix ICE when mixing VLAs and statement expressions [PR91038]
> > 
> > When returning VM-types from statement expressions, this can
> > lead to an ICE when declarations from the statement expression
> > are referred to later. Most of these issues can be addressed by
> > gimplifying the base expression earlier in gimplify_compound_lval.
> > Another issue is fixed by wrapping the pointer expression in
> > pointer_int_sum. This fixes PR91038 and some of the test cases
> > from PR29970 (structs with VLA members need further work).
> > 
> >      
> >      2021-10-30  Martin Uecker  <muecker@gwdg.de>
> >      
> >      gcc/
> >          PR c/91038
> >          PR c/29970
> > 	* c-family/c-common.c (pointer_int_sum): Make sure
> > 	pointer expressions are evaluated first when the size
> > 	expression depends on for variably-modified types.
> >          * gimplify.c (gimplify_var_or_parm_decl): Update comment.
> >          (gimplify_compound_lval): Gimplify base expression first.
> >          (gimplify_target_expr): Add comment.
> >      
> >      gcc/testsuite/
> >          PR c/91038
> >          PR c/29970
> >          * gcc.dg/vla-stexp-3.c: New test.
> >          * gcc.dg/vla-stexp-4.c: New test.
> >          * gcc.dg/vla-stexp-5.c: New test.
> >          * gcc.dg/vla-stexp-6.c: New test.
> >          * gcc.dg/vla-stexp-7.c: New test.
> >          * gcc.dg/vla-stexp-8.c: New test.
> >          * gcc.dg/vla-stexp-9.c: New test.
> 

diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c
index 436df45df68..668a2a129c6 100644
--- a/gcc/c-family/c-common.c
+++ b/gcc/c-family/c-common.c
@@ -3306,7 +3306,19 @@ pointer_int_sum (location_t loc, enum tree_code resultcode,
 				 TREE_TYPE (result_type)))
     size_exp = integer_one_node;
   else
-    size_exp = size_in_bytes_loc (loc, TREE_TYPE (result_type));
+    {
+      size_exp = size_in_bytes_loc (loc, TREE_TYPE (result_type));
+      /* Wrap the pointer expression in a SAVE_EXPR to make sure it
+	 is evaluated first when the size expression may depend
+	 on it for VM types.  */
+      if (TREE_SIDE_EFFECTS (size_exp)
+	  && TREE_SIDE_EFFECTS (ptrop)
+	  && variably_modified_type_p (TREE_TYPE (ptrop), NULL))
+	{
+	  ptrop = build1_loc (loc, SAVE_EXPR, TREE_TYPE (ptrop), ptrop);
+	  size_exp = build2 (COMPOUND_EXPR, TREE_TYPE (intop), ptrop, size_exp);
+	}
+    }
 
   /* We are manipulating pointer values, so we don't need to warn
      about relying on undefined signed overflow.  We disable the
diff --git a/gcc/gimplify.c b/gcc/gimplify.c
index c2ab96e7e18..84f7dc3c248 100644
--- a/gcc/gimplify.c
+++ b/gcc/gimplify.c
@@ -2964,7 +2964,9 @@ gimplify_var_or_parm_decl (tree *expr_p)
      declaration, for which we've already issued an error.  It would
      be really nice if the front end wouldn't leak these at all.
      Currently the only known culprit is C++ destructors, as seen
-     in g++.old-deja/g++.jason/binding.C.  */
+     in g++.old-deja/g++.jason/binding.C.
+     Another possible culpit are size expressions for variably modified
+     types which are lost in the FE or not gimplified correctly.  */
   if (VAR_P (decl)
       && !DECL_SEEN_IN_BIND_EXPR_P (decl)
       && !TREE_STATIC (decl) && !DECL_EXTERNAL (decl)
@@ -3109,16 +3111,22 @@ gimplify_compound_lval (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p,
      expression until we deal with any variable bounds, sizes, or
      positions in order to deal with PLACEHOLDER_EXPRs.
 
-     So we do this in three steps.  First we deal with the annotations
-     for any variables in the components, then we gimplify the base,
-     then we gimplify any indices, from left to right.  */
+     The base expression may contain a statement expression that
+     has declarations used in size expressions, so has to be
+     gimplified before gimplifying the size expressions.
+
+     So we do this in three steps.  First we deal with variable
+     bounds, sizes, and positions, then we gimplify the base,
+     then we deal with the annotations for any variables in the
+     components and any indices, from left to right.  */
+
   for (i = expr_stack.length () - 1; i >= 0; i--)
     {
       tree t = expr_stack[i];
 
       if (TREE_CODE (t) == ARRAY_REF || TREE_CODE (t) == ARRAY_RANGE_REF)
 	{
-	  /* Gimplify the low bound and element type size and put them into
+	  /* Deal with the low bound and element type size and put them into
 	     the ARRAY_REF.  If these values are set, they have already been
 	     gimplified.  */
 	  if (TREE_OPERAND (t, 2) == NULL_TREE)
@@ -3127,18 +3135,8 @@ gimplify_compound_lval (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p,
 	      if (!is_gimple_min_invariant (low))
 		{
 		  TREE_OPERAND (t, 2) = low;
-		  tret = gimplify_expr (&TREE_OPERAND (t, 2), pre_p,
-					post_p, is_gimple_reg,
-					fb_rvalue);
-		  ret = MIN (ret, tret);
 		}
 	    }
-	  else
-	    {
-	      tret = gimplify_expr (&TREE_OPERAND (t, 2), pre_p, post_p,
-				    is_gimple_reg, fb_rvalue);
-	      ret = MIN (ret, tret);
-	    }
 
 	  if (TREE_OPERAND (t, 3) == NULL_TREE)
 	    {
@@ -3155,18 +3153,8 @@ gimplify_compound_lval (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p,
 					      elmt_size, factor);
 
 		  TREE_OPERAND (t, 3) = elmt_size;
-		  tret = gimplify_expr (&TREE_OPERAND (t, 3), pre_p,
-					post_p, is_gimple_reg,
-					fb_rvalue);
-		  ret = MIN (ret, tret);
 		}
 	    }
-	  else
-	    {
-	      tret = gimplify_expr (&TREE_OPERAND (t, 3), pre_p, post_p,
-				    is_gimple_reg, fb_rvalue);
-	      ret = MIN (ret, tret);
-	    }
 	}
       else if (TREE_CODE (t) == COMPONENT_REF)
 	{
@@ -3186,18 +3174,8 @@ gimplify_compound_lval (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p,
 					   offset, factor);
 
 		  TREE_OPERAND (t, 2) = offset;
-		  tret = gimplify_expr (&TREE_OPERAND (t, 2), pre_p,
-					post_p, is_gimple_reg,
-					fb_rvalue);
-		  ret = MIN (ret, tret);
 		}
 	    }
-	  else
-	    {
-	      tret = gimplify_expr (&TREE_OPERAND (t, 2), pre_p, post_p,
-				    is_gimple_reg, fb_rvalue);
-	      ret = MIN (ret, tret);
-	    }
 	}
     }
 
@@ -3208,21 +3186,34 @@ gimplify_compound_lval (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p,
 			fallback | fb_lvalue);
   ret = MIN (ret, tret);
 
-  /* And finally, the indices and operands of ARRAY_REF.  During this
-     loop we also remove any useless conversions.  */
+  /* Step 3: gimplify size expressions and the indices and operands of
+     ARRAY_REF.  During this loop we also remove any useless conversions.  */
+
   for (; expr_stack.length () > 0; )
     {
       tree t = expr_stack.pop ();
 
       if (TREE_CODE (t) == ARRAY_REF || TREE_CODE (t) == ARRAY_RANGE_REF)
 	{
+	  /* Gimplify the low bound and element type size. */
+	  tret = gimplify_expr (&TREE_OPERAND (t, 2), pre_p, post_p,
+				is_gimple_reg, fb_rvalue);
+	  ret = MIN (ret, tret);
+
+	  tret = gimplify_expr (&TREE_OPERAND (t, 3), pre_p, post_p,
+				is_gimple_reg, fb_rvalue);
+	  ret = MIN (ret, tret);
+
 	  /* Gimplify the dimension.  */
-	  if (!is_gimple_min_invariant (TREE_OPERAND (t, 1)))
-	    {
-	      tret = gimplify_expr (&TREE_OPERAND (t, 1), pre_p, post_p,
-				    is_gimple_val, fb_rvalue);
-	      ret = MIN (ret, tret);
-	    }
+	  tret = gimplify_expr (&TREE_OPERAND (t, 1), pre_p, post_p,
+				is_gimple_val, fb_rvalue);
+	  ret = MIN (ret, tret);
+	}
+      else if (TREE_CODE (t) == COMPONENT_REF)
+	{
+	  tret = gimplify_expr (&TREE_OPERAND (t, 2), pre_p, post_p,
+				is_gimple_reg, fb_rvalue);
+	  ret = MIN (ret, tret);
 	}
 
       STRIP_USELESS_TYPE_CONVERSION (TREE_OPERAND (t, 0));
@@ -6927,6 +6918,8 @@ gimplify_target_expr (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p)
 	{
 	  if (!TYPE_SIZES_GIMPLIFIED (TREE_TYPE (temp)))
 	    gimplify_type_sizes (TREE_TYPE (temp), pre_p);
+	  /* FIXME: this is correct only when the size of the type does
+	     not depend on expressions evaluated in init.  */
 	  gimplify_vla_decl (temp, pre_p);
 	}
       else

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

[Jason Merrill]

On 11/7/21 01:40, Uecker, Martin wrote:
> Am Mittwoch, den 03.11.2021, 10:18 -0400 schrieb Jason Merrill:
>> On 10/31/21 05:22, Uecker, Martin wrote:
>>> Hi Jason,
>>>
>>> here is the fourth version of the patch.
>>>
>>> I followed your suggestion and now make this
>>> transformation sooner in pointer_int_sum.
>>> I also added a check to only do this
>>> transformation when the pointer is not a
>>> VAR_DECL, which avoids it in the most
>>> common cases where it is not necessary.
>>>
>>> Looking for BIND_EXPR seems complicated
>>> and I could not convince myself that it is
>>> worth it.  I also see the risk that this
>>> makes potential failure cases even more
>>> subtle. What do you think?
>>
>> That makes sense.  Though see some minor comments below.
> 
> Thank you! I made these changes and ran
> bootstrap and tests again.

Hmm, it doesn't look like you made the change to use the save_expr 
function instead of build1?

> Ok for trunk?
> 
> 
> Any idea how to fix returning structs with
> VLA member from statement expressions?

Testcase?

> Otherwise, I will add an error message to
> the FE in another patch.
> 
> Martin
> 
> 
>>>
>>> Fix ICE when mixing VLAs and statement expressions [PR91038]
>>>
>>> When returning VM-types from statement expressions, this can
>>> lead to an ICE when declarations from the statement expression
>>> are referred to later. Most of these issues can be addressed by
>>> gimplifying the base expression earlier in gimplify_compound_lval.
>>> Another issue is fixed by wrapping the pointer expression in
>>> pointer_int_sum. This fixes PR91038 and some of the test cases
>>> from PR29970 (structs with VLA members need further work).
>>>
>>>       
>>>       2021-10-30  Martin Uecker  <muecker@gwdg.de>
>>>       
>>>       gcc/
>>>           PR c/91038
>>>           PR c/29970
>>> 	* c-family/c-common.c (pointer_int_sum): Make sure
>>> 	pointer expressions are evaluated first when the size
>>> 	expression depends on for variably-modified types.
>>>           * gimplify.c (gimplify_var_or_parm_decl): Update comment.
>>>           (gimplify_compound_lval): Gimplify base expression first.
>>>           (gimplify_target_expr): Add comment.
>>>       
>>>       gcc/testsuite/
>>>           PR c/91038
>>>           PR c/29970
>>>           * gcc.dg/vla-stexp-3.c: New test.
>>>           * gcc.dg/vla-stexp-4.c: New test.
>>>           * gcc.dg/vla-stexp-5.c: New test.
>>>           * gcc.dg/vla-stexp-6.c: New test.
>>>           * gcc.dg/vla-stexp-7.c: New test.
>>>           * gcc.dg/vla-stexp-8.c: New test.
>>>           * gcc.dg/vla-stexp-9.c: New test.
>>
> 
> diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c
> index 436df45df68..668a2a129c6 100644
> --- a/gcc/c-family/c-common.c
> +++ b/gcc/c-family/c-common.c
> @@ -3306,7 +3306,19 @@ pointer_int_sum (location_t loc, enum tree_code resultcode,
>   				 TREE_TYPE (result_type)))
>       size_exp = integer_one_node;
>     else
> -    size_exp = size_in_bytes_loc (loc, TREE_TYPE (result_type));
> +    {
> +      size_exp = size_in_bytes_loc (loc, TREE_TYPE (result_type));
> +      /* Wrap the pointer expression in a SAVE_EXPR to make sure it
> +	 is evaluated first when the size expression may depend
> +	 on it for VM types.  */
> +      if (TREE_SIDE_EFFECTS (size_exp)
> +	  && TREE_SIDE_EFFECTS (ptrop)
> +	  && variably_modified_type_p (TREE_TYPE (ptrop), NULL))
> +	{
> +	  ptrop = build1_loc (loc, SAVE_EXPR, TREE_TYPE (ptrop), ptrop);

I still wonder why you are using build1 instead of the save_expr function?

> +	  size_exp = build2 (COMPOUND_EXPR, TREE_TYPE (intop), ptrop, size_exp);
> +	}
> +    }
>   
>     /* We are manipulating pointer values, so we don't need to warn
>        about relying on undefined signed overflow.  We disable the
> diff --git a/gcc/gimplify.c b/gcc/gimplify.c
> index c2ab96e7e18..84f7dc3c248 100644
> --- a/gcc/gimplify.c
> +++ b/gcc/gimplify.c
> @@ -2964,7 +2964,9 @@ gimplify_var_or_parm_decl (tree *expr_p)
>        declaration, for which we've already issued an error.  It would
>        be really nice if the front end wouldn't leak these at all.
>        Currently the only known culprit is C++ destructors, as seen
> -     in g++.old-deja/g++.jason/binding.C.  */
> +     in g++.old-deja/g++.jason/binding.C.
> +     Another possible culpit are size expressions for variably modified
> +     types which are lost in the FE or not gimplified correctly.  */
>     if (VAR_P (decl)
>         && !DECL_SEEN_IN_BIND_EXPR_P (decl)
>         && !TREE_STATIC (decl) && !DECL_EXTERNAL (decl)
> @@ -3109,16 +3111,22 @@ gimplify_compound_lval (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p,
>        expression until we deal with any variable bounds, sizes, or
>        positions in order to deal with PLACEHOLDER_EXPRs.
>   
> -     So we do this in three steps.  First we deal with the annotations
> -     for any variables in the components, then we gimplify the base,
> -     then we gimplify any indices, from left to right.  */
> +     The base expression may contain a statement expression that
> +     has declarations used in size expressions, so has to be
> +     gimplified before gimplifying the size expressions.
> +
> +     So we do this in three steps.  First we deal with variable
> +     bounds, sizes, and positions, then we gimplify the base,
> +     then we deal with the annotations for any variables in the
> +     components and any indices, from left to right.  */
> +
>     for (i = expr_stack.length () - 1; i >= 0; i--)
>       {
>         tree t = expr_stack[i];
>   
>         if (TREE_CODE (t) == ARRAY_REF || TREE_CODE (t) == ARRAY_RANGE_REF)
>   	{
> -	  /* Gimplify the low bound and element type size and put them into
> +	  /* Deal with the low bound and element type size and put them into
>   	     the ARRAY_REF.  If these values are set, they have already been
>   	     gimplified.  */
>   	  if (TREE_OPERAND (t, 2) == NULL_TREE)
> @@ -3127,18 +3135,8 @@ gimplify_compound_lval (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p,
>   	      if (!is_gimple_min_invariant (low))
>   		{
>   		  TREE_OPERAND (t, 2) = low;
> -		  tret = gimplify_expr (&TREE_OPERAND (t, 2), pre_p,
> -					post_p, is_gimple_reg,
> -					fb_rvalue);
> -		  ret = MIN (ret, tret);
>   		}
>   	    }
> -	  else
> -	    {
> -	      tret = gimplify_expr (&TREE_OPERAND (t, 2), pre_p, post_p,
> -				    is_gimple_reg, fb_rvalue);
> -	      ret = MIN (ret, tret);
> -	    }
>   
>   	  if (TREE_OPERAND (t, 3) == NULL_TREE)
>   	    {
> @@ -3155,18 +3153,8 @@ gimplify_compound_lval (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p,
>   					      elmt_size, factor);
>   
>   		  TREE_OPERAND (t, 3) = elmt_size;
> -		  tret = gimplify_expr (&TREE_OPERAND (t, 3), pre_p,
> -					post_p, is_gimple_reg,
> -					fb_rvalue);
> -		  ret = MIN (ret, tret);
>   		}
>   	    }
> -	  else
> -	    {
> -	      tret = gimplify_expr (&TREE_OPERAND (t, 3), pre_p, post_p,
> -				    is_gimple_reg, fb_rvalue);
> -	      ret = MIN (ret, tret);
> -	    }
>   	}
>         else if (TREE_CODE (t) == COMPONENT_REF)
>   	{
> @@ -3186,18 +3174,8 @@ gimplify_compound_lval (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p,
>   					   offset, factor);
>   
>   		  TREE_OPERAND (t, 2) = offset;
> -		  tret = gimplify_expr (&TREE_OPERAND (t, 2), pre_p,
> -					post_p, is_gimple_reg,
> -					fb_rvalue);
> -		  ret = MIN (ret, tret);
>   		}
>   	    }
> -	  else
> -	    {
> -	      tret = gimplify_expr (&TREE_OPERAND (t, 2), pre_p, post_p,
> -				    is_gimple_reg, fb_rvalue);
> -	      ret = MIN (ret, tret);
> -	    }
>   	}
>       }
>   
> @@ -3208,21 +3186,34 @@ gimplify_compound_lval (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p,
>   			fallback | fb_lvalue);
>     ret = MIN (ret, tret);
>   
> -  /* And finally, the indices and operands of ARRAY_REF.  During this
> -     loop we also remove any useless conversions.  */
> +  /* Step 3: gimplify size expressions and the indices and operands of
> +     ARRAY_REF.  During this loop we also remove any useless conversions.  */
> +
>     for (; expr_stack.length () > 0; )
>       {
>         tree t = expr_stack.pop ();
>   
>         if (TREE_CODE (t) == ARRAY_REF || TREE_CODE (t) == ARRAY_RANGE_REF)
>   	{
> +	  /* Gimplify the low bound and element type size. */
> +	  tret = gimplify_expr (&TREE_OPERAND (t, 2), pre_p, post_p,
> +				is_gimple_reg, fb_rvalue);
> +	  ret = MIN (ret, tret);
> +
> +	  tret = gimplify_expr (&TREE_OPERAND (t, 3), pre_p, post_p,
> +				is_gimple_reg, fb_rvalue);
> +	  ret = MIN (ret, tret);
> +
>   	  /* Gimplify the dimension.  */
> -	  if (!is_gimple_min_invariant (TREE_OPERAND (t, 1)))
> -	    {
> -	      tret = gimplify_expr (&TREE_OPERAND (t, 1), pre_p, post_p,
> -				    is_gimple_val, fb_rvalue);
> -	      ret = MIN (ret, tret);
> -	    }
> +	  tret = gimplify_expr (&TREE_OPERAND (t, 1), pre_p, post_p,
> +				is_gimple_val, fb_rvalue);
> +	  ret = MIN (ret, tret);
> +	}
> +      else if (TREE_CODE (t) == COMPONENT_REF)
> +	{
> +	  tret = gimplify_expr (&TREE_OPERAND (t, 2), pre_p, post_p,
> +				is_gimple_reg, fb_rvalue);
> +	  ret = MIN (ret, tret);
>   	}
>   
>         STRIP_USELESS_TYPE_CONVERSION (TREE_OPERAND (t, 0));
> @@ -6927,6 +6918,8 @@ gimplify_target_expr (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p)
>   	{
>   	  if (!TYPE_SIZES_GIMPLIFIED (TREE_TYPE (temp)))
>   	    gimplify_type_sizes (TREE_TYPE (temp), pre_p);
> +	  /* FIXME: this is correct only when the size of the type does
> +	     not depend on expressions evaluated in init.  */
>   	  gimplify_vla_decl (temp, pre_p);
>   	}
>         else
> 

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

[Uecker, Martin]

Am Montag, den 08.11.2021, 12:13 -0500 schrieb Jason Merrill:
> On 11/7/21 01:40, Uecker, Martin wrote:
> > Am Mittwoch, den 03.11.2021, 10:18 -0400 schrieb Jason Merrill:

...

> > 
> > Thank you! I made these changes and ran
> > bootstrap and tests again.
> 
> Hmm, it doesn't look like you made the change to use the save_expr 
> function instead of build1?

Oh, sorry. I wanted to change it and then forgot.
Now also with this change (changelog as before).

> > Ok for trunk?
> > 
> > 
> > Any idea how to fix returning structs with
> > VLA member from statement expressions?
> 
> Testcase?

void foo(void)
{
      ({ int N = 3; struct { char x[N]; } x; x; });
}

The difference to the tests in this patch (which
also forgot to include in the last version) is that
the object of variable size is returned from the
statement expression and not a pointer to it.
This can not happen with arrays because they decay
to pointers.


Martin


> > Otherwise, I will add an error message to
> > the FE in another patch.
> > 
> > Martin
> > 

diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c
index 436df45df68..95083f95442 100644
--- a/gcc/c-family/c-common.c
+++ b/gcc/c-family/c-common.c
@@ -3306,7 +3306,19 @@ pointer_int_sum (location_t loc, enum tree_code resultcode,
 				 TREE_TYPE (result_type)))
     size_exp = integer_one_node;
   else
-    size_exp = size_in_bytes_loc (loc, TREE_TYPE (result_type));
+    {
+      size_exp = size_in_bytes_loc (loc, TREE_TYPE (result_type));
+      /* Wrap the pointer expression in a SAVE_EXPR to make sure it
+	 is evaluated first when the size expression may depend
+	 on it for VM types.  */
+      if (TREE_SIDE_EFFECTS (size_exp)
+	  && TREE_SIDE_EFFECTS (ptrop)
+	  && variably_modified_type_p (TREE_TYPE (ptrop), NULL))
+	{
+	  ptrop = save_expr (ptrop);
+	  size_exp = build2 (COMPOUND_EXPR, TREE_TYPE (intop), ptrop, size_exp);
+	}
+    }
 
   /* We are manipulating pointer values, so we don't need to warn
      about relying on undefined signed overflow.  We disable the
diff --git a/gcc/gimplify.c b/gcc/gimplify.c
index c2ab96e7e18..84f7dc3c248 100644
--- a/gcc/gimplify.c
+++ b/gcc/gimplify.c
@@ -2964,7 +2964,9 @@ gimplify_var_or_parm_decl (tree *expr_p)
      declaration, for which we've already issued an error.  It would
      be really nice if the front end wouldn't leak these at all.
      Currently the only known culprit is C++ destructors, as seen
-     in g++.old-deja/g++.jason/binding.C.  */
+     in g++.old-deja/g++.jason/binding.C.
+     Another possible culpit are size expressions for variably modified
+     types which are lost in the FE or not gimplified correctly.  */
   if (VAR_P (decl)
       && !DECL_SEEN_IN_BIND_EXPR_P (decl)
       && !TREE_STATIC (decl) && !DECL_EXTERNAL (decl)
@@ -3109,16 +3111,22 @@ gimplify_compound_lval (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p,
      expression until we deal with any variable bounds, sizes, or
      positions in order to deal with PLACEHOLDER_EXPRs.
 
-     So we do this in three steps.  First we deal with the annotations
-     for any variables in the components, then we gimplify the base,
-     then we gimplify any indices, from left to right.  */
+     The base expression may contain a statement expression that
+     has declarations used in size expressions, so has to be
+     gimplified before gimplifying the size expressions.
+
+     So we do this in three steps.  First we deal with variable
+     bounds, sizes, and positions, then we gimplify the base,
+     then we deal with the annotations for any variables in the
+     components and any indices, from left to right.  */
+
   for (i = expr_stack.length () - 1; i >= 0; i--)
     {
       tree t = expr_stack[i];
 
       if (TREE_CODE (t) == ARRAY_REF || TREE_CODE (t) == ARRAY_RANGE_REF)
 	{
-	  /* Gimplify the low bound and element type size and put them into
+	  /* Deal with the low bound and element type size and put them into
 	     the ARRAY_REF.  If these values are set, they have already been
 	     gimplified.  */
 	  if (TREE_OPERAND (t, 2) == NULL_TREE)
@@ -3127,18 +3135,8 @@ gimplify_compound_lval (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p,
 	      if (!is_gimple_min_invariant (low))
 		{
 		  TREE_OPERAND (t, 2) = low;
-		  tret = gimplify_expr (&TREE_OPERAND (t, 2), pre_p,
-					post_p, is_gimple_reg,
-					fb_rvalue);
-		  ret = MIN (ret, tret);
 		}
 	    }
-	  else
-	    {
-	      tret = gimplify_expr (&TREE_OPERAND (t, 2), pre_p, post_p,
-				    is_gimple_reg, fb_rvalue);
-	      ret = MIN (ret, tret);
-	    }
 
 	  if (TREE_OPERAND (t, 3) == NULL_TREE)
 	    {
@@ -3155,18 +3153,8 @@ gimplify_compound_lval (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p,
 					      elmt_size, factor);
 
 		  TREE_OPERAND (t, 3) = elmt_size;
-		  tret = gimplify_expr (&TREE_OPERAND (t, 3), pre_p,
-					post_p, is_gimple_reg,
-					fb_rvalue);
-		  ret = MIN (ret, tret);
 		}
 	    }
-	  else
-	    {
-	      tret = gimplify_expr (&TREE_OPERAND (t, 3), pre_p, post_p,
-				    is_gimple_reg, fb_rvalue);
-	      ret = MIN (ret, tret);
-	    }
 	}
       else if (TREE_CODE (t) == COMPONENT_REF)
 	{
@@ -3186,18 +3174,8 @@ gimplify_compound_lval (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p,
 					   offset, factor);
 
 		  TREE_OPERAND (t, 2) = offset;
-		  tret = gimplify_expr (&TREE_OPERAND (t, 2), pre_p,
-					post_p, is_gimple_reg,
-					fb_rvalue);
-		  ret = MIN (ret, tret);
 		}
 	    }
-	  else
-	    {
-	      tret = gimplify_expr (&TREE_OPERAND (t, 2), pre_p, post_p,
-				    is_gimple_reg, fb_rvalue);
-	      ret = MIN (ret, tret);
-	    }
 	}
     }
 
@@ -3208,21 +3186,34 @@ gimplify_compound_lval (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p,
 			fallback | fb_lvalue);
   ret = MIN (ret, tret);
 
-  /* And finally, the indices and operands of ARRAY_REF.  During this
-     loop we also remove any useless conversions.  */
+  /* Step 3: gimplify size expressions and the indices and operands of
+     ARRAY_REF.  During this loop we also remove any useless conversions.  */
+
   for (; expr_stack.length () > 0; )
     {
       tree t = expr_stack.pop ();
 
       if (TREE_CODE (t) == ARRAY_REF || TREE_CODE (t) == ARRAY_RANGE_REF)
 	{
+	  /* Gimplify the low bound and element type size. */
+	  tret = gimplify_expr (&TREE_OPERAND (t, 2), pre_p, post_p,
+				is_gimple_reg, fb_rvalue);
+	  ret = MIN (ret, tret);
+
+	  tret = gimplify_expr (&TREE_OPERAND (t, 3), pre_p, post_p,
+				is_gimple_reg, fb_rvalue);
+	  ret = MIN (ret, tret);
+
 	  /* Gimplify the dimension.  */
-	  if (!is_gimple_min_invariant (TREE_OPERAND (t, 1)))
-	    {
-	      tret = gimplify_expr (&TREE_OPERAND (t, 1), pre_p, post_p,
-				    is_gimple_val, fb_rvalue);
-	      ret = MIN (ret, tret);
-	    }
+	  tret = gimplify_expr (&TREE_OPERAND (t, 1), pre_p, post_p,
+				is_gimple_val, fb_rvalue);
+	  ret = MIN (ret, tret);
+	}
+      else if (TREE_CODE (t) == COMPONENT_REF)
+	{
+	  tret = gimplify_expr (&TREE_OPERAND (t, 2), pre_p, post_p,
+				is_gimple_reg, fb_rvalue);
+	  ret = MIN (ret, tret);
 	}
 
       STRIP_USELESS_TYPE_CONVERSION (TREE_OPERAND (t, 0));
@@ -6927,6 +6918,8 @@ gimplify_target_expr (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p)
 	{
 	  if (!TYPE_SIZES_GIMPLIFIED (TREE_TYPE (temp)))
 	    gimplify_type_sizes (TREE_TYPE (temp), pre_p);
+	  /* FIXME: this is correct only when the size of the type does
+	     not depend on expressions evaluated in init.  */
 	  gimplify_vla_decl (temp, pre_p);
 	}
       else
diff --git a/gcc/testsuite/gcc.dg/vla-stexp-3.c b/gcc/testsuite/gcc.dg/vla-stexp-3.c
new file mode 100644
index 00000000000..e663de1cd72
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/vla-stexp-3.c
@@ -0,0 +1,11 @@
+/* PR91038 */
+/* { dg-do compile } */
+/* { dg-options "" } */
+
+
+void bar(void)
+{
+	({ int N = 2; int (*x)[9][N] = 0; x; })[1];
+	({ int N = 2; int (*x)[9][N] = 0; x; })[0];	// should not ice
+}
+
diff --git a/gcc/testsuite/gcc.dg/vla-stexp-4.c b/gcc/testsuite/gcc.dg/vla-stexp-4.c
new file mode 100644
index 00000000000..612b5a802fc
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/vla-stexp-4.c
@@ -0,0 +1,94 @@
+/* PR29970, PR91038 */
+/* { dg-do run } */
+/* { dg-options "-O0 -Wunused-variable" } */
+
+int foo3b(void)   // should not return 0
+{
+        int n = 0;
+        return sizeof *({ n = 10; int x[n]; &x; });
+}
+
+int foo4(void)   // should not ICE
+{
+        return (*({
+                        int n = 20;
+                        char (*x)[n][n] = __builtin_malloc(n * n);
+                        (*x)[12][1] = 1;
+                        x;
+                }))[12][1];
+}
+
+int foo5(void)   // should return 1, returns 0
+{
+        int n = 0;
+        return (*({
+                        n = 20;
+                        char (*x)[n][n] = __builtin_malloc(n * n);
+                        (*x)[12][1] = 1;
+                        (*x)[0][1] = 0;
+                        x;
+                }))[12][1];
+}
+
+int foo5c(void)   // should return 400 
+{
+        int n = 0;
+        return sizeof(*({
+                        n = 20;
+                        char (*x)[n][n] = __builtin_malloc(n * n);
+                        (*x)[12][1] = 1;
+                        (*x)[0][1] = 0;
+                        x;
+                }));
+}
+
+int foo5b(void)   // should return 1, returns 0
+{
+	int n = 0;			/* { dg-warning "unused variable" } */
+        return (*({
+                        int n = 20;
+                        char (*x)[n][n] = __builtin_malloc(n * n);
+                        (*x)[12][1] = 1;
+                        (*x)[0][1] = 0;
+                        x;
+                }))[12][1];
+}
+
+int foo5a(void)   // should return 1, returns 0
+{
+        return (*({
+                        int n = 20;
+                        char (*x)[n][n] = __builtin_malloc(n * n);
+                        (*x)[12][1] = 1;
+                        (*x)[0][1] = 0;
+                        x;
+                }))[12][1];
+}
+
+
+
+
+int main()
+{
+	if (sizeof(int[10]) != foo3b())
+		__builtin_abort();
+
+	if (1 != foo4())
+		__builtin_abort();
+
+	if (400 != foo5c())
+		__builtin_abort();
+
+	if (1 != foo5a())
+		__builtin_abort();
+
+	if (1 != foo5b()) // -O0
+		__builtin_abort();
+
+	if (1 != foo5())
+		__builtin_abort();
+
+	return 0;
+}
+
+
diff --git a/gcc/testsuite/gcc.dg/vla-stexp-5.c b/gcc/testsuite/gcc.dg/vla-stexp-5.c
new file mode 100644
index 00000000000..d6a7f2b34b8
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/vla-stexp-5.c
@@ -0,0 +1,30 @@
+/* PR29970 */
+/* { dg-do run } */
+/* { dg-options "-Wunused-variable" } */
+
+
+
+
+int foo2a(void)   // should not ICE
+{
+        return ({ int n = 20; struct { int x[n];} x; x.x[12] = 1; sizeof(x); });
+}
+
+
+int foo2b(void)   // should not ICE
+{
+        return sizeof *({ int n = 20; struct { int x[n];} x; x.x[12] = 1; &x; });
+}
+
+int main()
+{
+	if (sizeof(struct { int x[20]; }) != foo2a())
+		__builtin_abort();
+
+	if (sizeof(struct { int x[20]; }) != foo2b())
+		__builtin_abort();
+
+	return 0;
+}
+
+
diff --git a/gcc/testsuite/gcc.dg/vla-stexp-6.c b/gcc/testsuite/gcc.dg/vla-stexp-6.c
new file mode 100644
index 00000000000..3d96d38898b
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/vla-stexp-6.c
@@ -0,0 +1,94 @@
+/* PR29970, PR91038 */
+/* { dg-do run } */
+/* { dg-options "-O2 -Wunused-variable" } */
+
+int foo3b(void)   // should not return 0
+{
+        int n = 0;
+        return sizeof *({ n = 10; int x[n]; &x; });
+}
+
+int foo4(void)   // should not ICE
+{
+        return (*({
+                        int n = 20;
+                        char (*x)[n][n] = __builtin_malloc(n * n);
+                        (*x)[12][1] = 1;
+                        x;
+                }))[12][1];
+}
+
+int foo5(void)   // should return 1, returns 0
+{
+        int n = 0;
+        return (*({
+                        n = 20;
+                        char (*x)[n][n] = __builtin_malloc(n * n);
+                        (*x)[12][1] = 1;
+                        (*x)[0][1] = 0;
+                        x;
+                }))[12][1];
+}
+
+int foo5c(void)   // should return 400 
+{
+        int n = 0;
+        return sizeof(*({
+                        n = 20;
+                        char (*x)[n][n] = __builtin_malloc(n * n);
+                        (*x)[12][1] = 1;
+                        (*x)[0][1] = 0;
+                        x;
+                }));
+}
+
+int foo5b(void)   // should return 1, returns 0
+{
+	int n = 0;	/* { dg-warning "unused variable" } */
+        return (*({
+                        int n = 20;
+                        char (*x)[n][n] = __builtin_malloc(n * n);
+                        (*x)[12][1] = 1;
+                        (*x)[0][1] = 0;
+                        x;
+                }))[12][1];
+}
+
+int foo5a(void)   // should return 1, returns 0
+{
+        return (*({
+                        int n = 20;
+                        char (*x)[n][n] = __builtin_malloc(n * n);
+                        (*x)[12][1] = 1;
+                        (*x)[0][1] = 0;
+                        x;
+                }))[12][1];
+}
+
+
+
+
+int main()
+{
+	if (sizeof(int[10]) != foo3b())
+		__builtin_abort();
+
+	if (1 != foo4())
+		__builtin_abort();
+
+	if (400 != foo5c())
+		__builtin_abort();
+
+	if (1 != foo5a())
+		__builtin_abort();
+
+	if (1 != foo5b()) // -O0
+		__builtin_abort();
+
+	if (1 != foo5())
+		__builtin_abort();
+
+	return 0;
+}
+
+
diff --git a/gcc/testsuite/gcc.dg/vla-stexp-7.c b/gcc/testsuite/gcc.dg/vla-stexp-7.c
new file mode 100644
index 00000000000..3091b9184c2
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/vla-stexp-7.c
@@ -0,0 +1,44 @@
+/* PR91038 */
+/* { dg-do run } */
+/* { dg-options "-O2 -Wunused-variable" } */
+
+
+#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
+
+struct lbm {
+
+	int D;
+	const int* DQ;
+
+} D2Q9 = { 2,
+	(const int*)&(const int[9][2]){
+		{ 0, 0 },
+		{ 1, 0 }, { 0, 1 }, { -1, 0 }, { 0, -1 },
+		{ 1, 1 }, { -1, 1 }, { -1, -1 }, { 1, -1 },
+	}
+};
+
+void zouhe_left(void)
+{
+	__auto_type xx = (*({ int N = 2; struct lbm __x = D2Q9; ((const int(*)[9][N])__x.DQ); }));
+
+	if (1 != xx[1][0])
+		__builtin_abort();
+
+	if (2 != ARRAY_SIZE(xx[1]))
+		__builtin_abort();
+
+	if (1 != (*({ int N = 2; struct lbm __x = D2Q9; ((const int(*)[9][N])__x.DQ); }))[1][0])
+		__builtin_abort();
+
+	if (2 != ARRAY_SIZE(*({ int N = 2; struct lbm __x = D2Q9; ((const int(*)[9][N])__x.DQ);
})[1]))
+		__builtin_abort();
+}
+
+int main()
+{
+	zouhe_left();
+	return 0;
+}
+
+
diff --git a/gcc/testsuite/gcc.dg/vla-stexp-8.c b/gcc/testsuite/gcc.dg/vla-stexp-8.c
new file mode 100644
index 00000000000..5b475eb6cf2
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/vla-stexp-8.c
@@ -0,0 +1,47 @@
+/* PR29970, PR91038 */
+/* { dg-do compile } */
+/* { dg-options "-O2 -Wunused-variable" } */
+
+
+int foo0(void)
+{
+	int c = *(*(*({ int n = 10; int (*x)[n][n] = __builtin_malloc(sizeof *x); x; }) + 5) + 5);
+	return c;
+}
+
+int foo1(void)
+{
+	int c = *(5 + *(5 + *({ int n = 10; int (*x)[n][n] = __builtin_malloc(sizeof *x); x; })));
+	return c;
+}
+
+int bar2(void)
+{
+	int c = (*({ int n = 10; struct { int y[n]; int z; }* x = __builtin_malloc(sizeof *x); x;
})).z;
+	return c;
+}
+
+int bar3(void)
+{
+	int n = 2;	/* { dg-warning "unused variable" } */
+	int c = (*({ int n = 3; 	/* { dg-warning "unused variable" } */
+		({ int n = 10; int (*x)[n][n] = __builtin_malloc(sizeof *x); x; }); }))[5][5];
+	return c;
+}
+
+int bar3b(void)
+{
+	int n = 2;	/* { dg-warning "unused variable" } */
+	int c = (*({ int n = 3; 	/* { dg-warning "unused variable" } */
+		({ int n = 10; int (*x)[n][n] = __builtin_malloc(sizeof *x); x; }); }))[0][0];
+	return c;
+}
+
+int bar4(void)
+{
+	int n = 2;	/* { dg-warning "unused variable" } */
+	int c = *(5 + *( 5 + *({ int n = 3;	/* { dg-warning "unused variable" } */
+		({ int n = 10; int (*x)[n][n] = __builtin_malloc(sizeof *x); x; }); })));
+	return c;
+}
+
diff --git a/gcc/testsuite/gcc.dg/vla-stexp-9.c b/gcc/testsuite/gcc.dg/vla-stexp-9.c
new file mode 100644
index 00000000000..3593a790785
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/vla-stexp-9.c
@@ -0,0 +1,53 @@
+/* PR91038 */
+/* { dg-do run } */
+/* { dg-options "-O2 -Wunused-variable" } */
+
+
+
+void foo(void)
+{
+	if (2 * sizeof(int) != sizeof((*({ int N = 2; int (*x)[9][N] = 0; x; })[1])))
+		__builtin_abort();
+}
+
+void bar(void)
+{
+	if (2 * sizeof(int) != sizeof((*({ int N = 2; int (*x)[9][N] = 0; x; })[0])))
+		__builtin_abort();
+}
+
+void bar0(void)
+{
+	if (2 * 9 *  sizeof(int) != sizeof((*({ int N = 2; int (*x)[9][N] = 0; x; }))))
+		__builtin_abort();
+}
+
+void bar11(void)
+{
+	sizeof(*((*({ int N = 2; int (*x)[9][N] = 0; x; }) + 0)));
+}
+
+void bar12(void)
+{
+	if (2 * sizeof(int) != sizeof(*((*({ int N = 2; int (*x)[9][N] = 0; x; })    ))))
+		__builtin_abort();
+}
+
+void bar1(void)
+{
+	if (2 * sizeof(int) != sizeof(*((*({ int N = 2; int (*x)[9][N] = 0; x; }) + 0))))
+		__builtin_abort();
+}
+
+
+
+
+int main()
+{
+	foo();
+	bar0();
+	bar12();
+	bar1();
+	bar();
+}
+

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

[Uecker, Martin]

Am Montag, den 08.11.2021, 19:13 +0100 schrieb Martin Uecker:
> Am Montag, den 08.11.2021, 12:13 -0500 schrieb Jason Merrill:
> > On 11/7/21 01:40, Uecker, Martin wrote:
> > > Am Mittwoch, den 03.11.2021, 10:18 -0400 schrieb Jason Merrill:
> 
> ...
> 
> > > Thank you! I made these changes and ran
> > > bootstrap and tests again.
> > 
> > Hmm, it doesn't look like you made the change to use the save_expr 
> > function instead of build1?
> 
> Oh, sorry. I wanted to change it and then forgot.
> Now also with this change (changelog as before).


Ok, with is this change?

Best,
Martin



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

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

[Jason Merrill]

On 11/16/21 08:48, Uecker, Martin wrote:
> Am Montag, den 08.11.2021, 19:13 +0100 schrieb Martin Uecker:
>> Am Montag, den 08.11.2021, 12:13 -0500 schrieb Jason Merrill:
>>> On 11/7/21 01:40, Uecker, Martin wrote:
>>>> Am Mittwoch, den 03.11.2021, 10:18 -0400 schrieb Jason Merrill:
>>
>> ...
>>
>>>> Thank you! I made these changes and ran
>>>> bootstrap and tests again.
>>>
>>> Hmm, it doesn't look like you made the change to use the save_expr
>>> function instead of build1?
>>
>> Oh, sorry. I wanted to change it and then forgot.
>> Now also with this change (changelog as before).
> 
> 
> Ok, with is this change?

OK.

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

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

[Jason Merrill]

On 10/20/21 01:58, Uecker, Martin wrote:
> Am Montag, den 18.10.2021, 12:35 -0400 schrieb Jason Merrill:
>> On 10/17/21 09:52, Uecker, Martin wrote:
>>>
>>> Here is the 4th version of the patch. I tried to implement
>>> Jason's suggestion and this also fixes the problem. But
>>> I am not sure I understand the condition on
>>> the TREE_SIDE_EFFECTS ...
>>
>> Checking TREE_SIDE_EFFECTS filters out many trivial cases that we don't
>> need to worry about.
> 
> Yes, but are we sure there are always side effects in the
> cases we care about? I assume that the problematic case
> are only those where the size expression depends on a
> variable and then there was a write to one.. But I am not
> sure.

The problematic case should always involve a SAVE_EXPR or TARGET_EXPR, 
which has TREE_SIDE_EFFECTS.

>> I think we also want to check
>> variably_modified_type_p, which ought to avoid the OMP problem below.
> 
> I don't think so because the problem below involves VM types.

Ah, true.

>>> And there is now another problem:
>>>
>>> c_finish_omp_for in c-family/c-omp.c does not seem
>>> to understand the expressions anymore and I get a test
>>> failure in
>>>
>>> testsuite/c-c++-common/gomp/for-5.c
>>>
>>> where I now get an "invalid increment expression"
>>> instead of the expected error.
>>> (bootstrapping and all other tests work fine)

Ah, yes, because the variable size is still wrapped in a SAVE_EXPR even 
if the inner expression is just a variable reference.

I think we only want to do this transformation if size_exp involves a 
BIND_EXPR.

And probably do it sooner, changing size_exp rather than intop.

Jason

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

[Uecker, Martin]

Am Montag, den 18.10.2021, 12:35 -0400 schrieb Jason Merrill:
> On 10/17/21 09:52, Uecker, Martin wrote:
> > 
> > Here is the 4th version of the patch. I tried to implement
> > Jason's suggestion and this also fixes the problem. But
> > I am not sure I understand the condition on
> > the TREE_SIDE_EFFECTS ...
> 
> Checking TREE_SIDE_EFFECTS filters out many trivial cases that we don't 
> need to worry about. 

Yes, but are we sure there are always side effects in the
cases we care about? I assume that the problematic case
are only those where the size expression depends on a
variable and then there was a write to one.. But I am not
sure.

>  I think we also want to check 
> variably_modified_type_p, which ought to avoid the OMP problem below.

I don't think so because the problem below involves VM types.

Martin

> > And there is now another problem:
> > 
> > c_finish_omp_for in c-family/c-omp.c does not seem
> > to understand the expressions anymore and I get a test
> > failure in
> > 
> > testsuite/c-c++-common/gomp/for-5.c
> > 
> > where I now get an "invalid increment expression"
> > instead of the expected error.
> > (bootstrapping and all other tests work fine)
> > 
> > 
> > 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 adding SAVE_EXPRs in pointer_int_sum
> > in the FE to force a correct order of evaluation. 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>
> >      
> >     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): Add comment.
> >          * c-family/c-
> > common.c (pointer_int_sum): Wrap pointer
> > 	operand in SAVE_EXPR and also it to the integer
> > argument.
> >      
> >      gcc/testsuite/
> >          PR c/91038
> >          PR c/29970
> >          * gcc.dg/vla-stexp-3.c:
> > New test.
> >          * gcc.dg/vla-stexp-4.c: New test.
> >          * gcc.dg/vla-stexp-5.c: New test.
> >          *
> > gcc.dg/vla-stexp-6.c: New test.
> >          * gcc.dg/vla-stexp-7.c: New test.
> >          * gcc.dg/vla-stexp-
> > 8.c: New test.
> >          * gcc.dg/vla-stexp-9.c: New test.
> > 
> > 
> > diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c
> > index 9d19e352725..522085664f5 100644
> > --- a/gcc/c-family/c-common.c
> > +++ b/gcc/c-family/c-common.c
> > @@ -3348,6 +3348,16 @@ pointer_int_sum (location_t loc, enum tree_code resultcode,
> >       intop = convert (c_common_type_for_size (TYPE_PRECISION (sizetype),
> >   					     TYPE_UNSIGNED (sizetype)), intop);
> >   
> > +  /* Wrap the pointer expression in a SAVE_EXPR to make sure it
> > +   * is evaluated first because the size expression may depend on it
> > +   * for VM types.
> > +   */
> 
> We usually don't give the trailing */ its own line.
> 
> > +  if (TREE_SIDE_EFFECTS (size_exp))
> > +    {
> > +	 ptrop = build1_loc (loc, SAVE_EXPR, TREE_TYPE (ptrop), ptrop);
> 
> Why not use the save_expr function?
> 
> > +	 intop = build2 (COMPOUND_EXPR, TREE_TYPE (intop), ptrop, intop);
> > +    }
> > +
> >     /* Replace the integer argument with a suitable product by the object size.
> >        Do this multiplication as signed, then convert to the appropriate type
> >        for the pointer operation and disregard an overflow that occurred only
> > diff --git a/gcc/gimplify.c b/gcc/gimplify.c
> > index d8e4b139349..be5b00b6716 100644
> > --- a/gcc/gimplify.c
> > +++ b/gcc/gimplify.c
> > @@ -2958,7 +2958,10 @@ gimplify_var_or_parm_decl (tree *expr_p)
> >        declaration, for which we've already issued an error.  It would
> >        be really nice if the front end wouldn't leak these at all.
> >        Currently the only known culprit is C++ destructors, as seen
> > -     in g++.old-deja/g++.jason/binding.C.  */
> > +     in g++.old-deja/g++.jason/binding.C.
> > +     Another possible culpit are size expressions for variably modified
> > +     types which are lost in the FE or not gimplified correctly.
> > +  */
> 
> As above.
> 
> >     if (VAR_P (decl)
> >         && !DECL_SEEN_IN_BIND_EXPR_P (decl)
> >         && !TREE_STATIC (decl) && !DECL_EXTERNAL (decl)
> > @@ -3103,16 +3106,22 @@ gimplify_compound_lval (tree *expr_p, gimple_seq *pre_p, gimple_seq
> > *post_p,
> >        expression until we deal with any variable bounds, sizes, or
> >        positions in order to deal with PLACEHOLDER_EXPRs.
> >   
> > -     So we do this in three steps.  First we deal with the annotations
> > -     for any variables in the components, then we gimplify the base,
> > -     then we gimplify any indices, from left to right.  */
> > +     The base expression may contain a statement expression that
> > +     has declarations used in size expressions, so has to be
> > +     gimplified before gimplifying the size expressions.
> > +
> > +     So we do this in three steps.  First we deal with variable
> > +     bounds, sizes, and positions, then we gimplify the base,
> > +     then we deal with the annotations for any variables in the
> > +     components and any indices, from left to right.  */
> > +
> >     for (i = expr_stack.length () - 1; i >= 0; i--)
> >       {
> >         tree t = expr_stack[i];
> >   
> >         if (TREE_CODE (t) == ARRAY_REF || TREE_CODE (t) == ARRAY_RANGE_REF)
> >   	{
> > -	  /* Gimplify the low bound and element type size and put them into
> > +	  /* Deal with the low bound and element type size and put them into
> >   	     the ARRAY_REF.  If these values are set, they have already been
> >   	     gimplified.  */
> >   	  if (TREE_OPERAND (t, 2) == NULL_TREE)
> > @@ -3121,18 +3130,8 @@ gimplify_compound_lval (tree *expr_p, gimple_seq *pre_p, gimple_seq
> > *post_p,
> >   	      if (!is_gimple_min_invariant (low))
> >   		{
> >   		  TREE_OPERAND (t, 2) = low;
> > -		  tret = gimplify_expr (&TREE_OPERAND (t, 2), pre_p,
> > -					post_p, is_gimple_reg,
> > -					fb_rvalue);
> > -		  ret = MIN (ret, tret);
> >   		}
> >   	    }
> > -	  else
> > -	    {
> > -	      tret = gimplify_expr (&TREE_OPERAND (t, 2), pre_p, post_p,
> > -				    is_gimple_reg, fb_rvalue);
> > -	      ret = MIN (ret, tret);
> > -	    }
> >   
> >   	  if (TREE_OPERAND (t, 3) == NULL_TREE)
> >   	    {
> > @@ -3149,18 +3148,8 @@ gimplify_compound_lval (tree *expr_p, gimple_seq *pre_p, gimple_seq
> > *post_p,
> >   					      elmt_size, factor);
> >   
> >   		  TREE_OPERAND (t, 3) = elmt_size;
> > -		  tret = gimplify_expr (&TREE_OPERAND (t, 3), pre_p,
> > -					post_p, is_gimple_reg,
> > -					fb_rvalue);
> > -		  ret = MIN (ret, tret);
> >   		}
> >   	    }
> > -	  else
> > -	    {
> > -	      tret = gimplify_expr (&TREE_OPERAND (t, 3), pre_p, post_p,
> > -				    is_gimple_reg, fb_rvalue);
> > -	      ret = MIN (ret, tret);
> > -	    }
> >   	}
> >         else if (TREE_CODE (t) == COMPONENT_REF)
> >   	{
> > @@ -3180,18 +3169,8 @@ gimplify_compound_lval (tree *expr_p, gimple_seq *pre_p, gimple_seq
> > *post_p,
> >   					   offset, factor);
> >   
> >   		  TREE_OPERAND (t, 2) = offset;
> > -		  tret = gimplify_expr (&TREE_OPERAND (t, 2), pre_p,
> > -					post_p, is_gimple_reg,
> > -					fb_rvalue);
> > -		  ret = MIN (ret, tret);
> >   		}
> >   	    }
> > -	  else
> > -	    {
> > -	      tret = gimplify_expr (&TREE_OPERAND (t, 2), pre_p, post_p,
> > -				    is_gimple_reg, fb_rvalue);
> > -	      ret = MIN (ret, tret);
> > -	    }
> >   	}
> >       }
> >   
> > @@ -3202,21 +3181,34 @@ gimplify_compound_lval (tree *expr_p, gimple_seq *pre_p, gimple_seq
> > *post_p,
> >   			fallback | fb_lvalue);
> >     ret = MIN (ret, tret);
> >   
> > -  /* And finally, the indices and operands of ARRAY_REF.  During this
> > -     loop we also remove any useless conversions.  */
> > +  /* Step 3: gimplify size expressions and the indices and operands of
> > +     ARRAY_REF.  During this loop we also remove any useless conversions.  */
> > +
> >     for (; expr_stack.length () > 0; )
> >       {
> >         tree t = expr_stack.pop ();
> >   
> >         if (TREE_CODE (t) == ARRAY_REF || TREE_CODE (t) == ARRAY_RANGE_REF)
> >   	{
> > +	  /* Gimplify the low bound and element type size. */
> > +	  tret = gimplify_expr (&TREE_OPERAND (t, 2), pre_p, post_p,
> > +				is_gimple_reg, fb_rvalue);
> > +	  ret = MIN (ret, tret);
> > +
> > +	  tret = gimplify_expr (&TREE_OPERAND (t, 3), pre_p, post_p,
> > +				is_gimple_reg, fb_rvalue);
> > +	  ret = MIN (ret, tret);
> > +
> >   	  /* Gimplify the dimension.  */
> > -	  if (!is_gimple_min_invariant (TREE_OPERAND (t, 1)))
> > -	    {
> > -	      tret = gimplify_expr (&TREE_OPERAND (t, 1), pre_p, post_p,
> > -				    is_gimple_val, fb_rvalue);
> > -	      ret = MIN (ret, tret);
> > -	    }
> > +	  tret = gimplify_expr (&TREE_OPERAND (t, 1), pre_p, post_p,
> > +				is_gimple_val, fb_rvalue);
> > +	  ret = MIN (ret, tret);
> > +	}
> > +      else if (TREE_CODE (t) == COMPONENT_REF)
> > +	{
> > +	  tret = gimplify_expr (&TREE_OPERAND (t, 2), pre_p, post_p,
> > +				is_gimple_reg, fb_rvalue);
> > +	  ret = MIN (ret, tret);
> >   	}
> >   
> >         STRIP_USELESS_TYPE_CONVERSION (TREE_OPERAND (t, 0));
> > @@ -6914,6 +6906,8 @@ gimplify_target_expr (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p)
> >   	{
> >   	  if (!TYPE_SIZES_GIMPLIFIED (TREE_TYPE (temp)))
> >   	    gimplify_type_sizes (TREE_TYPE (temp), pre_p);
> > +	  /* FIXME: this is correct only when the size of the type does
> > +	     not depend on expressions evaluated in init. */
> >   	  gimplify_vla_decl (temp, pre_p);
> >   	}
> >         else
> > 

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

[Jason Merrill]

On 10/17/21 09:52, Uecker, Martin wrote:
> 
> 
> Here is the 4th version of the patch. I tried to implement
> Jason's suggestion and this also fixes the problem. But
> I am not sure I understand the condition on
> the TREE_SIDE_EFFECTS ...

Checking TREE_SIDE_EFFECTS filters out many trivial cases that we don't 
need to worry about.  I think we also want to check 
variably_modified_type_p, which ought to avoid the OMP problem below.

> And there is now another problem:
> 
> c_finish_omp_for in c-family/c-omp.c does not seem
> to understand the expressions anymore and I get a test
> failure in
> 
> testsuite/c-c++-common/gomp/for-5.c
> 
> where I now get an "invalid increment expression"
> instead of the expected error.

> (bootstrapping and all other tests work fine)
> 
> 
> 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 adding SAVE_EXPRs in pointer_int_sum
> in the FE to force a correct order of evaluation. 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>
>      
>     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): Add comment.
>          * c-family/c-
> common.c (pointer_int_sum): Wrap pointer
> 	operand in SAVE_EXPR and also it to the integer
> argument.
>      
>      gcc/testsuite/
>          PR c/91038
>          PR c/29970
>          * gcc.dg/vla-stexp-3.c:
> New test.
>          * gcc.dg/vla-stexp-4.c: New test.
>          * gcc.dg/vla-stexp-5.c: New test.
>          *
> gcc.dg/vla-stexp-6.c: New test.
>          * gcc.dg/vla-stexp-7.c: New test.
>          * gcc.dg/vla-stexp-
> 8.c: New test.
>          * gcc.dg/vla-stexp-9.c: New test.
> 
> 
> diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c
> index 9d19e352725..522085664f5 100644
> --- a/gcc/c-family/c-common.c
> +++ b/gcc/c-family/c-common.c
> @@ -3348,6 +3348,16 @@ pointer_int_sum (location_t loc, enum tree_code resultcode,
>       intop = convert (c_common_type_for_size (TYPE_PRECISION (sizetype),
>   					     TYPE_UNSIGNED (sizetype)), intop);
>   
> +  /* Wrap the pointer expression in a SAVE_EXPR to make sure it
> +   * is evaluated first because the size expression may depend on it
> +   * for VM types.
> +   */

We usually don't give the trailing */ its own line.

> +  if (TREE_SIDE_EFFECTS (size_exp))
> +    {
> +	 ptrop = build1_loc (loc, SAVE_EXPR, TREE_TYPE (ptrop), ptrop);

Why not use the save_expr function?

> +	 intop = build2 (COMPOUND_EXPR, TREE_TYPE (intop), ptrop, intop);
> +    }
> +
>     /* Replace the integer argument with a suitable product by the object size.
>        Do this multiplication as signed, then convert to the appropriate type
>        for the pointer operation and disregard an overflow that occurred only
> diff --git a/gcc/gimplify.c b/gcc/gimplify.c
> index d8e4b139349..be5b00b6716 100644
> --- a/gcc/gimplify.c
> +++ b/gcc/gimplify.c
> @@ -2958,7 +2958,10 @@ gimplify_var_or_parm_decl (tree *expr_p)
>        declaration, for which we've already issued an error.  It would
>        be really nice if the front end wouldn't leak these at all.
>        Currently the only known culprit is C++ destructors, as seen
> -     in g++.old-deja/g++.jason/binding.C.  */
> +     in g++.old-deja/g++.jason/binding.C.
> +     Another possible culpit are size expressions for variably modified
> +     types which are lost in the FE or not gimplified correctly.
> +  */

As above.

>     if (VAR_P (decl)
>         && !DECL_SEEN_IN_BIND_EXPR_P (decl)
>         && !TREE_STATIC (decl) && !DECL_EXTERNAL (decl)
> @@ -3103,16 +3106,22 @@ gimplify_compound_lval (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p,
>        expression until we deal with any variable bounds, sizes, or
>        positions in order to deal with PLACEHOLDER_EXPRs.
>   
> -     So we do this in three steps.  First we deal with the annotations
> -     for any variables in the components, then we gimplify the base,
> -     then we gimplify any indices, from left to right.  */
> +     The base expression may contain a statement expression that
> +     has declarations used in size expressions, so has to be
> +     gimplified before gimplifying the size expressions.
> +
> +     So we do this in three steps.  First we deal with variable
> +     bounds, sizes, and positions, then we gimplify the base,
> +     then we deal with the annotations for any variables in the
> +     components and any indices, from left to right.  */
> +
>     for (i = expr_stack.length () - 1; i >= 0; i--)
>       {
>         tree t = expr_stack[i];
>   
>         if (TREE_CODE (t) == ARRAY_REF || TREE_CODE (t) == ARRAY_RANGE_REF)
>   	{
> -	  /* Gimplify the low bound and element type size and put them into
> +	  /* Deal with the low bound and element type size and put them into
>   	     the ARRAY_REF.  If these values are set, they have already been
>   	     gimplified.  */
>   	  if (TREE_OPERAND (t, 2) == NULL_TREE)
> @@ -3121,18 +3130,8 @@ gimplify_compound_lval (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p,
>   	      if (!is_gimple_min_invariant (low))
>   		{
>   		  TREE_OPERAND (t, 2) = low;
> -		  tret = gimplify_expr (&TREE_OPERAND (t, 2), pre_p,
> -					post_p, is_gimple_reg,
> -					fb_rvalue);
> -		  ret = MIN (ret, tret);
>   		}
>   	    }
> -	  else
> -	    {
> -	      tret = gimplify_expr (&TREE_OPERAND (t, 2), pre_p, post_p,
> -				    is_gimple_reg, fb_rvalue);
> -	      ret = MIN (ret, tret);
> -	    }
>   
>   	  if (TREE_OPERAND (t, 3) == NULL_TREE)
>   	    {
> @@ -3149,18 +3148,8 @@ gimplify_compound_lval (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p,
>   					      elmt_size, factor);
>   
>   		  TREE_OPERAND (t, 3) = elmt_size;
> -		  tret = gimplify_expr (&TREE_OPERAND (t, 3), pre_p,
> -					post_p, is_gimple_reg,
> -					fb_rvalue);
> -		  ret = MIN (ret, tret);
>   		}
>   	    }
> -	  else
> -	    {
> -	      tret = gimplify_expr (&TREE_OPERAND (t, 3), pre_p, post_p,
> -				    is_gimple_reg, fb_rvalue);
> -	      ret = MIN (ret, tret);
> -	    }
>   	}
>         else if (TREE_CODE (t) == COMPONENT_REF)
>   	{
> @@ -3180,18 +3169,8 @@ gimplify_compound_lval (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p,
>   					   offset, factor);
>   
>   		  TREE_OPERAND (t, 2) = offset;
> -		  tret = gimplify_expr (&TREE_OPERAND (t, 2), pre_p,
> -					post_p, is_gimple_reg,
> -					fb_rvalue);
> -		  ret = MIN (ret, tret);
>   		}
>   	    }
> -	  else
> -	    {
> -	      tret = gimplify_expr (&TREE_OPERAND (t, 2), pre_p, post_p,
> -				    is_gimple_reg, fb_rvalue);
> -	      ret = MIN (ret, tret);
> -	    }
>   	}
>       }
>   
> @@ -3202,21 +3181,34 @@ gimplify_compound_lval (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p,
>   			fallback | fb_lvalue);
>     ret = MIN (ret, tret);
>   
> -  /* And finally, the indices and operands of ARRAY_REF.  During this
> -     loop we also remove any useless conversions.  */
> +  /* Step 3: gimplify size expressions and the indices and operands of
> +     ARRAY_REF.  During this loop we also remove any useless conversions.  */
> +
>     for (; expr_stack.length () > 0; )
>       {
>         tree t = expr_stack.pop ();
>   
>         if (TREE_CODE (t) == ARRAY_REF || TREE_CODE (t) == ARRAY_RANGE_REF)
>   	{
> +	  /* Gimplify the low bound and element type size. */
> +	  tret = gimplify_expr (&TREE_OPERAND (t, 2), pre_p, post_p,
> +				is_gimple_reg, fb_rvalue);
> +	  ret = MIN (ret, tret);
> +
> +	  tret = gimplify_expr (&TREE_OPERAND (t, 3), pre_p, post_p,
> +				is_gimple_reg, fb_rvalue);
> +	  ret = MIN (ret, tret);
> +
>   	  /* Gimplify the dimension.  */
> -	  if (!is_gimple_min_invariant (TREE_OPERAND (t, 1)))
> -	    {
> -	      tret = gimplify_expr (&TREE_OPERAND (t, 1), pre_p, post_p,
> -				    is_gimple_val, fb_rvalue);
> -	      ret = MIN (ret, tret);
> -	    }
> +	  tret = gimplify_expr (&TREE_OPERAND (t, 1), pre_p, post_p,
> +				is_gimple_val, fb_rvalue);
> +	  ret = MIN (ret, tret);
> +	}
> +      else if (TREE_CODE (t) == COMPONENT_REF)
> +	{
> +	  tret = gimplify_expr (&TREE_OPERAND (t, 2), pre_p, post_p,
> +				is_gimple_reg, fb_rvalue);
> +	  ret = MIN (ret, tret);
>   	}
>   
>         STRIP_USELESS_TYPE_CONVERSION (TREE_OPERAND (t, 0));
> @@ -6914,6 +6906,8 @@ gimplify_target_expr (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p)
>   	{
>   	  if (!TYPE_SIZES_GIMPLIFIED (TREE_TYPE (temp)))
>   	    gimplify_type_sizes (TREE_TYPE (temp), pre_p);
> +	  /* FIXME: this is correct only when the size of the type does
> +	     not depend on expressions evaluated in init. */
>   	  gimplify_vla_decl (temp, pre_p);
>   	}
>         else
> 

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

[Uecker, Martin]

Here is the 4th version of the patch. I tried to implement
Jason's suggestion and this also fixes the problem. But
I am not sure I understand the condition on
the TREE_SIDE_EFFECTS ... 

And there is now another problem: 

c_finish_omp_for in c-family/c-omp.c does not seem
to understand the expressions anymore and I get a test
failure in 

testsuite/c-c++-common/gomp/for-5.c

where I now get an "invalid increment expression"
instead of the expected error.

(bootstrapping and all other tests work fine)


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 adding SAVE_EXPRs in pointer_int_sum
in the FE to force a correct order of evaluation. 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>
    
   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): Add comment.
        * c-family/c-
common.c (pointer_int_sum): Wrap pointer
	operand in SAVE_EXPR and also it to the integer
argument.
    
    gcc/testsuite/
        PR c/91038
        PR c/29970
        * gcc.dg/vla-stexp-3.c:
New test.
        * gcc.dg/vla-stexp-4.c: New test.
        * gcc.dg/vla-stexp-5.c: New test.
        *
gcc.dg/vla-stexp-6.c: New test.
        * gcc.dg/vla-stexp-7.c: New test.
        * gcc.dg/vla-stexp-
8.c: New test.
        * gcc.dg/vla-stexp-9.c: New test.


diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c
index 9d19e352725..522085664f5 100644
--- a/gcc/c-family/c-common.c
+++ b/gcc/c-family/c-common.c
@@ -3348,6 +3348,16 @@ pointer_int_sum (location_t loc, enum tree_code resultcode,
     intop = convert (c_common_type_for_size (TYPE_PRECISION (sizetype),
 					     TYPE_UNSIGNED (sizetype)), intop);
 
+  /* Wrap the pointer expression in a SAVE_EXPR to make sure it
+   * is evaluated first because the size expression may depend on it
+   * for VM types.
+   */
+  if (TREE_SIDE_EFFECTS (size_exp))
+    {
+	 ptrop = build1_loc (loc, SAVE_EXPR, TREE_TYPE (ptrop), ptrop);
+	 intop = build2 (COMPOUND_EXPR, TREE_TYPE (intop), ptrop, intop);
+    }
+
   /* Replace the integer argument with a suitable product by the object size.
      Do this multiplication as signed, then convert to the appropriate type
      for the pointer operation and disregard an overflow that occurred only
diff --git a/gcc/gimplify.c b/gcc/gimplify.c
index d8e4b139349..be5b00b6716 100644
--- a/gcc/gimplify.c
+++ b/gcc/gimplify.c
@@ -2958,7 +2958,10 @@ gimplify_var_or_parm_decl (tree *expr_p)
      declaration, for which we've already issued an error.  It would
      be really nice if the front end wouldn't leak these at all.
      Currently the only known culprit is C++ destructors, as seen
-     in g++.old-deja/g++.jason/binding.C.  */
+     in g++.old-deja/g++.jason/binding.C.
+     Another possible culpit are size expressions for variably modified
+     types which are lost in the FE or not gimplified correctly.
+  */
   if (VAR_P (decl)
       && !DECL_SEEN_IN_BIND_EXPR_P (decl)
       && !TREE_STATIC (decl) && !DECL_EXTERNAL (decl)
@@ -3103,16 +3106,22 @@ gimplify_compound_lval (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p,
      expression until we deal with any variable bounds, sizes, or
      positions in order to deal with PLACEHOLDER_EXPRs.
 
-     So we do this in three steps.  First we deal with the annotations
-     for any variables in the components, then we gimplify the base,
-     then we gimplify any indices, from left to right.  */
+     The base expression may contain a statement expression that
+     has declarations used in size expressions, so has to be
+     gimplified before gimplifying the size expressions.
+
+     So we do this in three steps.  First we deal with variable
+     bounds, sizes, and positions, then we gimplify the base,
+     then we deal with the annotations for any variables in the
+     components and any indices, from left to right.  */
+
   for (i = expr_stack.length () - 1; i >= 0; i--)
     {
       tree t = expr_stack[i];
 
       if (TREE_CODE (t) == ARRAY_REF || TREE_CODE (t) == ARRAY_RANGE_REF)
 	{
-	  /* Gimplify the low bound and element type size and put them into
+	  /* Deal with the low bound and element type size and put them into
 	     the ARRAY_REF.  If these values are set, they have already been
 	     gimplified.  */
 	  if (TREE_OPERAND (t, 2) == NULL_TREE)
@@ -3121,18 +3130,8 @@ gimplify_compound_lval (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p,
 	      if (!is_gimple_min_invariant (low))
 		{
 		  TREE_OPERAND (t, 2) = low;
-		  tret = gimplify_expr (&TREE_OPERAND (t, 2), pre_p,
-					post_p, is_gimple_reg,
-					fb_rvalue);
-		  ret = MIN (ret, tret);
 		}
 	    }
-	  else
-	    {
-	      tret = gimplify_expr (&TREE_OPERAND (t, 2), pre_p, post_p,
-				    is_gimple_reg, fb_rvalue);
-	      ret = MIN (ret, tret);
-	    }
 
 	  if (TREE_OPERAND (t, 3) == NULL_TREE)
 	    {
@@ -3149,18 +3148,8 @@ gimplify_compound_lval (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p,
 					      elmt_size, factor);
 
 		  TREE_OPERAND (t, 3) = elmt_size;
-		  tret = gimplify_expr (&TREE_OPERAND (t, 3), pre_p,
-					post_p, is_gimple_reg,
-					fb_rvalue);
-		  ret = MIN (ret, tret);
 		}
 	    }
-	  else
-	    {
-	      tret = gimplify_expr (&TREE_OPERAND (t, 3), pre_p, post_p,
-				    is_gimple_reg, fb_rvalue);
-	      ret = MIN (ret, tret);
-	    }
 	}
       else if (TREE_CODE (t) == COMPONENT_REF)
 	{
@@ -3180,18 +3169,8 @@ gimplify_compound_lval (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p,
 					   offset, factor);
 
 		  TREE_OPERAND (t, 2) = offset;
-		  tret = gimplify_expr (&TREE_OPERAND (t, 2), pre_p,
-					post_p, is_gimple_reg,
-					fb_rvalue);
-		  ret = MIN (ret, tret);
 		}
 	    }
-	  else
-	    {
-	      tret = gimplify_expr (&TREE_OPERAND (t, 2), pre_p, post_p,
-				    is_gimple_reg, fb_rvalue);
-	      ret = MIN (ret, tret);
-	    }
 	}
     }
 
@@ -3202,21 +3181,34 @@ gimplify_compound_lval (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p,
 			fallback | fb_lvalue);
   ret = MIN (ret, tret);
 
-  /* And finally, the indices and operands of ARRAY_REF.  During this
-     loop we also remove any useless conversions.  */
+  /* Step 3: gimplify size expressions and the indices and operands of
+     ARRAY_REF.  During this loop we also remove any useless conversions.  */
+
   for (; expr_stack.length () > 0; )
     {
       tree t = expr_stack.pop ();
 
       if (TREE_CODE (t) == ARRAY_REF || TREE_CODE (t) == ARRAY_RANGE_REF)
 	{
+	  /* Gimplify the low bound and element type size. */
+	  tret = gimplify_expr (&TREE_OPERAND (t, 2), pre_p, post_p,
+				is_gimple_reg, fb_rvalue);
+	  ret = MIN (ret, tret);
+
+	  tret = gimplify_expr (&TREE_OPERAND (t, 3), pre_p, post_p,
+				is_gimple_reg, fb_rvalue);
+	  ret = MIN (ret, tret);
+
 	  /* Gimplify the dimension.  */
-	  if (!is_gimple_min_invariant (TREE_OPERAND (t, 1)))
-	    {
-	      tret = gimplify_expr (&TREE_OPERAND (t, 1), pre_p, post_p,
-				    is_gimple_val, fb_rvalue);
-	      ret = MIN (ret, tret);
-	    }
+	  tret = gimplify_expr (&TREE_OPERAND (t, 1), pre_p, post_p,
+				is_gimple_val, fb_rvalue);
+	  ret = MIN (ret, tret);
+	}
+      else if (TREE_CODE (t) == COMPONENT_REF)
+	{
+	  tret = gimplify_expr (&TREE_OPERAND (t, 2), pre_p, post_p,
+				is_gimple_reg, fb_rvalue);
+	  ret = MIN (ret, tret);
 	}
 
       STRIP_USELESS_TYPE_CONVERSION (TREE_OPERAND (t, 0));
@@ -6914,6 +6906,8 @@ gimplify_target_expr (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p)
 	{
 	  if (!TYPE_SIZES_GIMPLIFIED (TREE_TYPE (temp)))
 	    gimplify_type_sizes (TREE_TYPE (temp), pre_p);
+	  /* FIXME: this is correct only when the size of the type does
+	     not depend on expressions evaluated in init. */
 	  gimplify_vla_decl (temp, pre_p);
 	}
       else