* [PATCH] Fix ICE when mixing VLAs and statement expressions [PR91038]
@ 2021-08-01 17:36 Uecker, Martin
2021-08-02 10:16 ` Richard Biener
2021-08-16 4:30 ` Jason Merrill
0 siblings, 2 replies; 8+ messages in thread
From: Uecker, Martin @ 2021-08-01 17:36 UTC (permalink / raw)
To: gcc-patches
Here is an attempt to fix some old and annoying bugs related
to VLAs and statement expressions. In particulary, this seems
to fix the issues with variably-modified types which are
returned from statement expressions (which works on clang),
but there are still bugs remaining related to structs
with VLA members (which seems to be a FE bug).
Of course, I might be doing something stupid...
The patch survives bootstrapping and regresstion testing
on x86_64.
--Martin
Fix ICE when mixing VLAs and statement expressions [PR91038]
When returning VM-types from statement expressions, this can
lead to an ICE when declarations from the statement expression
are referred to later. Some of these issues can be addressed by
gimplifying the base expression earlier in gimplify_compound_lval.
This fixes PR91038 and some of the test cases from PR29970
(structs with VLA members need further work).
2021-08-01 Martin Uecker <muecker@gwdg.de>
gcc/
PR c/91038
PR c/29970
* gimplify.c (gimplify_var_or_parm_decl): Update comment.
(gimplify_compound_lval): Gimplify base expression first.
gcc/testsuite/
PR c/91038
PR c/29970
* gcc.dg/vla-stexp-01.c: New test.
* gcc.dg/vla-stexp-02.c: New test.
* gcc.dg/vla-stexp-03.c: New test.
* gcc.dg/vla-stexp-04.c: New test.
diff --git a/gcc/gimplify.c b/gcc/gimplify.c
index 21ff32ee4aa..885d5f73585 100644
--- a/gcc/gimplify.c
+++ b/gcc/gimplify.c
@@ -2839,7 +2839,10 @@ gimplify_var_or_parm_decl (tree *expr_p)
declaration, for which we've already issued an error. It would
be really nice if the front end wouldn't leak these at all.
Currently the only known culprit is C++ destructors, as seen
- in g++.old-deja/g++.jason/binding.C. */
+ in g++.old-deja/g++.jason/binding.C.
+ Another culpit are size expressions for variably modified
+ types which are lost in the FE or not gimplified correctly.
+ */
if (VAR_P (decl)
&& !DECL_SEEN_IN_BIND_EXPR_P (decl)
&& !TREE_STATIC (decl) && !DECL_EXTERNAL (decl)
@@ -2984,9 +2987,23 @@ gimplify_compound_lval (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p,
expression until we deal with any variable bounds, sizes, or
positions in order to deal with PLACEHOLDER_EXPRs.
- So we do this in three steps. First we deal with the annotations
- for any variables in the components, then we gimplify the base,
- then we gimplify any indices, from left to right. */
+ So we do this in three steps. First we gimplify the base,
+ then we deal with the annotations for any variables in the
+ components, then we gimplify any indices, from left to right.
+
+ The base expression may contain a statement expression that
+ has declarations used in size expressions, so has to be
+ gimplified first. */
+
+ /* Step 1 is to gimplify the base expression. Make sure lvalue is set
+ so as to match the min_lval predicate. Failure to do so may result
+ in the creation of large aggregate temporaries. */
+ tret = gimplify_expr (p, pre_p, post_p, is_gimple_min_lval,
+ fallback | fb_lvalue);
+
+ ret = MIN (ret, tret);
+
+
for (i = expr_stack.length () - 1; i >= 0; i--)
{
tree t = expr_stack[i];
@@ -3076,12 +3093,6 @@ gimplify_compound_lval (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p,
}
}
- /* Step 2 is to gimplify the base expression. Make sure lvalue is set
- so as to match the min_lval predicate. Failure to do so may result
- in the creation of large aggregate temporaries. */
- tret = gimplify_expr (p, pre_p, post_p, is_gimple_min_lval,
- fallback | fb_lvalue);
- ret = MIN (ret, tret);
/* And finally, the indices and operands of ARRAY_REF. During this
loop we also remove any useless conversions. */
diff --git a/gcc/testsuite/gcc.dg/vla-stexpr-01.c b/gcc/testsuite/gcc.dg/vla-stexpr-01.c
new file mode 100644
index 00000000000..27e1817eb63
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/vla-stexpr-01.c
@@ -0,0 +1,94 @@
+/* PR29970, PR91038 */
+/* { dg-do run } */
+/* { dg-options "-O0" } */
+
+int foo3b(void) // should not return 0
+{
+ int n = 0;
+ return sizeof *({ n = 10; int x[n]; x; &x; });
+}
+
+int foo4(void) // should not ICE
+{
+ return (*({
+ int n = 20;
+ char (*x)[n][n] = __builtin_malloc(n * n);
+ (*x)[12][1] = 1;
+ x;
+ }))[12][1];
+}
+
+int foo5(void) // should return 1, returns 0
+{
+ int n = 0;
+ return (*({
+ n = 20;
+ char (*x)[n][n] = __builtin_malloc(n * n);
+ (*x)[12][1] = 1;
+ (*x)[0][1] = 0;
+ x;
+ }))[12][1];
+}
+
+int foo5c(void) // should return 400
+{
+ int n = 0;
+ return sizeof(*({
+ n = 20;
+ char (*x)[n][n] = __builtin_malloc(n * n);
+ (*x)[12][1] = 1;
+ (*x)[0][1] = 0;
+ x;
+ }));
+}
+
+int foo5b(void) // should return 1, returns 0
+{
+ int n = 0;
+ return (*({
+ int n = 20;
+ char (*x)[n][n] = __builtin_malloc(n * n);
+ (*x)[12][1] = 1;
+ (*x)[0][1] = 0;
+ x;
+ }))[12][1];
+}
+
+int foo5a(void) // should return 1, returns 0
+{
+ return (*({
+ int n = 20;
+ char (*x)[n][n] = __builtin_malloc(n * n);
+ (*x)[12][1] = 1;
+ (*x)[0][1] = 0;
+ x;
+ }))[12][1];
+}
+
+
+
+
+int main()
+{
+ if (sizeof(int[10]) != foo3b())
+ __builtin_abort();
+
+ if (1 != foo4())
+ __builtin_abort();
+
+ if (400 != foo5c())
+ __builtin_abort();
+
+ if (1 != foo5a())
+ __builtin_abort();
+
+ if (1 != foo5b()) // -O0
+ __builtin_abort();
+
+ if (1 != foo5())
+ __builtin_abort();
+
+ return 0;
+}
+
+
diff --git a/gcc/testsuite/gcc.dg/vla-stexpr-02.c b/gcc/testsuite/gcc.dg/vla-stexpr-02.c
new file mode 100644
index 00000000000..99b4f571e52
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/vla-stexpr-02.c
@@ -0,0 +1,31 @@
+/* PR29970 */
+/* { dg-do run } */
+/* { dg-options "" } */
+
+
+
+
+int foo2a(void) // should not ICE
+{
+ return ({ int n = 20; struct { int x[n];} x; x.x[12] = 1; sizeof(x); });
+}
+
+#if 0
+int foo2b(void) // should not ICE
+{
+ return sizeof *({ int n = 20; struct { int x[n];} x; x.x[12] = 1; &x; });
+}
+#endif
+
+int main()
+{
+ if (sizeof(struct { int x[20]; }) != foo2a())
+ __builtin_abort();
+#if 0
+ if (sizeof(struct { int x[20]; }) != foo2b())
+ __builtin_abort();
+#endif
+ return 0;
+}
+
+
diff --git a/gcc/testsuite/gcc.dg/vla-stexpr-03.c b/gcc/testsuite/gcc.dg/vla-stexpr-03.c
new file mode 100644
index 00000000000..61f8986eaa3
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/vla-stexpr-03.c
@@ -0,0 +1,94 @@
+/* PR29970, PR91038 */
+/* { dg-do run } */
+/* { dg-options "-O2" } */
+
+int foo3b(void) // should not return 0
+{
+ int n = 0;
+ return sizeof *({ n = 10; int x[n]; x; &x; });
+}
+
+int foo4(void) // should not ICE
+{
+ return (*({
+ int n = 20;
+ char (*x)[n][n] = __builtin_malloc(n * n);
+ (*x)[12][1] = 1;
+ x;
+ }))[12][1];
+}
+
+int foo5(void) // should return 1, returns 0
+{
+ int n = 0;
+ return (*({
+ n = 20;
+ char (*x)[n][n] = __builtin_malloc(n * n);
+ (*x)[12][1] = 1;
+ (*x)[0][1] = 0;
+ x;
+ }))[12][1];
+}
+
+int foo5c(void) // should return 400
+{
+ int n = 0;
+ return sizeof(*({
+ n = 20;
+ char (*x)[n][n] = __builtin_malloc(n * n);
+ (*x)[12][1] = 1;
+ (*x)[0][1] = 0;
+ x;
+ }));
+}
+
+int foo5b(void) // should return 1, returns 0
+{
+ int n = 0;
+ return (*({
+ int n = 20;
+ char (*x)[n][n] = __builtin_malloc(n * n);
+ (*x)[12][1] = 1;
+ (*x)[0][1] = 0;
+ x;
+ }))[12][1];
+}
+
+int foo5a(void) // should return 1, returns 0
+{
+ return (*({
+ int n = 20;
+ char (*x)[n][n] = __builtin_malloc(n * n);
+ (*x)[12][1] = 1;
+ (*x)[0][1] = 0;
+ x;
+ }))[12][1];
+}
+
+
+
+
+int main()
+{
+ if (sizeof(int[10]) != foo3b())
+ __builtin_abort();
+
+ if (1 != foo4())
+ __builtin_abort();
+
+ if (400 != foo5c())
+ __builtin_abort();
+
+ if (1 != foo5a())
+ __builtin_abort();
+
+ if (1 != foo5b()) // -O0
+ __builtin_abort();
+
+ if (1 != foo5())
+ __builtin_abort();
+
+ return 0;
+}
+
+
diff --git a/gcc/testsuite/gcc.dg/vla-stexpr-04.c b/gcc/testsuite/gcc.dg/vla-stexpr-04.c
new file mode 100644
index 00000000000..ffc8b12fa9b
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/vla-stexpr-04.c
@@ -0,0 +1,38 @@
+/* PR91038 */
+/* { dg-do run } */
+/* { dg-options "" } */
+
+
+#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
+
+struct lbm {
+
+ int D;
+ const int* DQ;
+
+} D2Q9 = { 2,
+ (const int*)&(const int[9][2]){
+ { 0, 0 },
+ { 1, 0 }, { 0, 1 }, { -1, 0 }, { 0, -1 },
+ { 1, 1 }, { -1, 1 }, { -1, -1 }, { 1, -1 },
+ }
+};
+
+void zouhe_left(void)
+{
+ __auto_type xx = (*({ int N = 2; struct lbm __x = D2Q9; ((const int(*)[9][N])__x.DQ); }));
+
+ if (1 != xx[1][0])
+ __builtin_abort();
+
+ if (2 != ARRAY_SIZE(xx[1]))
+ __builtin_abort();
+}
+
+int main()
+{
+ zouhe_left();
+ return 0;
+}
+
+
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Fix ICE when mixing VLAs and statement expressions [PR91038]
2021-08-01 17:36 [PATCH] Fix ICE when mixing VLAs and statement expressions [PR91038] Uecker, Martin
@ 2021-08-02 10:16 ` Richard Biener
2021-08-16 4:30 ` Jason Merrill
1 sibling, 0 replies; 8+ messages in thread
From: Richard Biener @ 2021-08-02 10:16 UTC (permalink / raw)
To: Uecker, Martin; +Cc: gcc-patches
On Sun, Aug 1, 2021 at 7:37 PM Uecker, Martin
<Martin.Uecker@med.uni-goettingen.de> wrote:
>
>
>
> Here is an attempt to fix some old and annoying bugs related
> to VLAs and statement expressions. In particulary, this seems
> to fix the issues with variably-modified types which are
> returned from statement expressions (which works on clang),
> but there are still bugs remaining related to structs
> with VLA members (which seems to be a FE bug).
>
> Of course, I might be doing something stupid...
How's evaluation order of (f())[g()] defined (with f returning a pointer)?
Isn't that just f() + g()*sizeof(int) and thus undefined?
If it's undefined then I think the incoming GENERIC is ill-defined.
> The patch survives bootstrapping and regresstion testing
> on x86_64.
>
>
> --Martin
>
>
>
>
> Fix ICE when mixing VLAs and statement expressions [PR91038]
>
> When returning VM-types from statement expressions, this can
> lead to an ICE when declarations from the statement expression
> are referred to later. Some of these issues can be addressed by
> gimplifying the base expression earlier in gimplify_compound_lval.
> This fixes PR91038 and some of the test cases from PR29970
> (structs with VLA members need further work).
>
>
> 2021-08-01 Martin Uecker <muecker@gwdg.de>
>
> gcc/
> PR c/91038
> PR c/29970
> * gimplify.c (gimplify_var_or_parm_decl): Update comment.
> (gimplify_compound_lval): Gimplify base expression first.
>
> gcc/testsuite/
> PR c/91038
> PR c/29970
> * gcc.dg/vla-stexp-01.c: New test.
> * gcc.dg/vla-stexp-02.c: New test.
> * gcc.dg/vla-stexp-03.c: New test.
> * gcc.dg/vla-stexp-04.c: New test.
>
>
> diff --git a/gcc/gimplify.c b/gcc/gimplify.c
> index 21ff32ee4aa..885d5f73585 100644
> --- a/gcc/gimplify.c
> +++ b/gcc/gimplify.c
> @@ -2839,7 +2839,10 @@ gimplify_var_or_parm_decl (tree *expr_p)
> declaration, for which we've already issued an error. It would
> be really nice if the front end wouldn't leak these at all.
> Currently the only known culprit is C++ destructors, as seen
> - in g++.old-deja/g++.jason/binding.C. */
> + in g++.old-deja/g++.jason/binding.C.
> + Another culpit are size expressions for variably modified
> + types which are lost in the FE or not gimplified correctly.
> + */
> if (VAR_P (decl)
> && !DECL_SEEN_IN_BIND_EXPR_P (decl)
> && !TREE_STATIC (decl) && !DECL_EXTERNAL (decl)
> @@ -2984,9 +2987,23 @@ gimplify_compound_lval (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p,
> expression until we deal with any variable bounds, sizes, or
> positions in order to deal with PLACEHOLDER_EXPRs.
>
> - So we do this in three steps. First we deal with the annotations
> - for any variables in the components, then we gimplify the base,
> - then we gimplify any indices, from left to right. */
> + So we do this in three steps. First we gimplify the base,
> + then we deal with the annotations for any variables in the
> + components, then we gimplify any indices, from left to right.
> +
> + The base expression may contain a statement expression that
> + has declarations used in size expressions, so has to be
> + gimplified first. */
> +
> + /* Step 1 is to gimplify the base expression. Make sure lvalue is set
> + so as to match the min_lval predicate. Failure to do so may result
> + in the creation of large aggregate temporaries. */
> + tret = gimplify_expr (p, pre_p, post_p, is_gimple_min_lval,
> + fallback | fb_lvalue);
> +
> + ret = MIN (ret, tret);
> +
> +
> for (i = expr_stack.length () - 1; i >= 0; i--)
> {
> tree t = expr_stack[i];
> @@ -3076,12 +3093,6 @@ gimplify_compound_lval (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p,
> }
> }
>
> - /* Step 2 is to gimplify the base expression. Make sure lvalue is set
> - so as to match the min_lval predicate. Failure to do so may result
> - in the creation of large aggregate temporaries. */
> - tret = gimplify_expr (p, pre_p, post_p, is_gimple_min_lval,
> - fallback | fb_lvalue);
> - ret = MIN (ret, tret);
>
> /* And finally, the indices and operands of ARRAY_REF. During this
> loop we also remove any useless conversions. */
> diff --git a/gcc/testsuite/gcc.dg/vla-stexpr-01.c b/gcc/testsuite/gcc.dg/vla-stexpr-01.c
> new file mode 100644
> index 00000000000..27e1817eb63
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/vla-stexpr-01.c
> @@ -0,0 +1,94 @@
> +/* PR29970, PR91038 */
> +/* { dg-do run } */
> +/* { dg-options "-O0" } */
> +
> +int foo3b(void) // should not return 0
> +{
> + int n = 0;
> + return sizeof *({ n = 10; int x[n]; x; &x; });
> +}
> +
> +int foo4(void) // should not ICE
> +{
> + return (*({
> + int n = 20;
> + char (*x)[n][n] = __builtin_malloc(n * n);
> + (*x)[12][1] = 1;
> + x;
> + }))[12][1];
> +}
> +
> +int foo5(void) // should return 1, returns 0
> +{
> + int n = 0;
> + return (*({
> + n = 20;
> + char (*x)[n][n] = __builtin_malloc(n * n);
> + (*x)[12][1] = 1;
> + (*x)[0][1] = 0;
> + x;
> + }))[12][1];
> +}
> +
> +int foo5c(void) // should return 400
> +{
> + int n = 0;
> + return sizeof(*({
> + n = 20;
> + char (*x)[n][n] = __builtin_malloc(n * n);
> + (*x)[12][1] = 1;
> + (*x)[0][1] = 0;
> + x;
> + }));
> +}
> +
> +int foo5b(void) // should return 1, returns 0
> +{
> + int n = 0;
> + return (*({
> + int n = 20;
> + char (*x)[n][n] = __builtin_malloc(n * n);
> + (*x)[12][1] = 1;
> + (*x)[0][1] = 0;
> + x;
> + }))[12][1];
> +}
> +
> +int foo5a(void) // should return 1, returns 0
> +{
> + return (*({
> + int n = 20;
> + char (*x)[n][n] = __builtin_malloc(n * n);
> + (*x)[12][1] = 1;
> + (*x)[0][1] = 0;
> + x;
> + }))[12][1];
> +}
> +
> +
> +
> +
> +int main()
> +{
> + if (sizeof(int[10]) != foo3b())
> + __builtin_abort();
> +
> + if (1 != foo4())
> + __builtin_abort();
> +
> + if (400 != foo5c())
> + __builtin_abort();
> +
> + if (1 != foo5a())
> + __builtin_abort();
> +
> + if (1 != foo5b()) // -O0
> + __builtin_abort();
> +
> + if (1 != foo5())
> + __builtin_abort();
> +
> + return 0;
> +}
> +
> +
> diff --git a/gcc/testsuite/gcc.dg/vla-stexpr-02.c b/gcc/testsuite/gcc.dg/vla-stexpr-02.c
> new file mode 100644
> index 00000000000..99b4f571e52
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/vla-stexpr-02.c
> @@ -0,0 +1,31 @@
> +/* PR29970 */
> +/* { dg-do run } */
> +/* { dg-options "" } */
> +
> +
> +
> +
> +int foo2a(void) // should not ICE
> +{
> + return ({ int n = 20; struct { int x[n];} x; x.x[12] = 1; sizeof(x); });
> +}
> +
> +#if 0
> +int foo2b(void) // should not ICE
> +{
> + return sizeof *({ int n = 20; struct { int x[n];} x; x.x[12] = 1; &x; });
> +}
> +#endif
> +
> +int main()
> +{
> + if (sizeof(struct { int x[20]; }) != foo2a())
> + __builtin_abort();
> +#if 0
> + if (sizeof(struct { int x[20]; }) != foo2b())
> + __builtin_abort();
> +#endif
> + return 0;
> +}
> +
> +
> diff --git a/gcc/testsuite/gcc.dg/vla-stexpr-03.c b/gcc/testsuite/gcc.dg/vla-stexpr-03.c
> new file mode 100644
> index 00000000000..61f8986eaa3
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/vla-stexpr-03.c
> @@ -0,0 +1,94 @@
> +/* PR29970, PR91038 */
> +/* { dg-do run } */
> +/* { dg-options "-O2" } */
> +
> +int foo3b(void) // should not return 0
> +{
> + int n = 0;
> + return sizeof *({ n = 10; int x[n]; x; &x; });
> +}
> +
> +int foo4(void) // should not ICE
> +{
> + return (*({
> + int n = 20;
> + char (*x)[n][n] = __builtin_malloc(n * n);
> + (*x)[12][1] = 1;
> + x;
> + }))[12][1];
> +}
> +
> +int foo5(void) // should return 1, returns 0
> +{
> + int n = 0;
> + return (*({
> + n = 20;
> + char (*x)[n][n] = __builtin_malloc(n * n);
> + (*x)[12][1] = 1;
> + (*x)[0][1] = 0;
> + x;
> + }))[12][1];
> +}
> +
> +int foo5c(void) // should return 400
> +{
> + int n = 0;
> + return sizeof(*({
> + n = 20;
> + char (*x)[n][n] = __builtin_malloc(n * n);
> + (*x)[12][1] = 1;
> + (*x)[0][1] = 0;
> + x;
> + }));
> +}
> +
> +int foo5b(void) // should return 1, returns 0
> +{
> + int n = 0;
> + return (*({
> + int n = 20;
> + char (*x)[n][n] = __builtin_malloc(n * n);
> + (*x)[12][1] = 1;
> + (*x)[0][1] = 0;
> + x;
> + }))[12][1];
> +}
> +
> +int foo5a(void) // should return 1, returns 0
> +{
> + return (*({
> + int n = 20;
> + char (*x)[n][n] = __builtin_malloc(n * n);
> + (*x)[12][1] = 1;
> + (*x)[0][1] = 0;
> + x;
> + }))[12][1];
> +}
> +
> +
> +
> +
> +int main()
> +{
> + if (sizeof(int[10]) != foo3b())
> + __builtin_abort();
> +
> + if (1 != foo4())
> + __builtin_abort();
> +
> + if (400 != foo5c())
> + __builtin_abort();
> +
> + if (1 != foo5a())
> + __builtin_abort();
> +
> + if (1 != foo5b()) // -O0
> + __builtin_abort();
> +
> + if (1 != foo5())
> + __builtin_abort();
> +
> + return 0;
> +}
> +
> +
> diff --git a/gcc/testsuite/gcc.dg/vla-stexpr-04.c b/gcc/testsuite/gcc.dg/vla-stexpr-04.c
> new file mode 100644
> index 00000000000..ffc8b12fa9b
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/vla-stexpr-04.c
> @@ -0,0 +1,38 @@
> +/* PR91038 */
> +/* { dg-do run } */
> +/* { dg-options "" } */
> +
> +
> +#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
> +
> +struct lbm {
> +
> + int D;
> + const int* DQ;
> +
> +} D2Q9 = { 2,
> + (const int*)&(const int[9][2]){
> + { 0, 0 },
> + { 1, 0 }, { 0, 1 }, { -1, 0 }, { 0, -1 },
> + { 1, 1 }, { -1, 1 }, { -1, -1 }, { 1, -1 },
> + }
> +};
> +
> +void zouhe_left(void)
> +{
> + __auto_type xx = (*({ int N = 2; struct lbm __x = D2Q9; ((const int(*)[9][N])__x.DQ); }));
> +
> + if (1 != xx[1][0])
> + __builtin_abort();
> +
> + if (2 != ARRAY_SIZE(xx[1]))
> + __builtin_abort();
> +}
> +
> +int main()
> +{
> + zouhe_left();
> + return 0;
> +}
> +
> +
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Fix ICE when mixing VLAs and statement expressions [PR91038]
2021-08-01 17:36 [PATCH] Fix ICE when mixing VLAs and statement expressions [PR91038] Uecker, Martin
2021-08-02 10:16 ` Richard Biener
@ 2021-08-16 4:30 ` Jason Merrill
2021-08-16 4:49 ` Uecker, Martin
1 sibling, 1 reply; 8+ messages in thread
From: Jason Merrill @ 2021-08-16 4:30 UTC (permalink / raw)
To: Uecker, Martin, kenner; +Cc: gcc-patches
On 8/1/21 1:36 PM, Uecker, Martin wrote:
>
>
> Here is an attempt to fix some old and annoying bugs related
> to VLAs and statement expressions. In particulary, this seems
> to fix the issues with variably-modified types which are
> returned from statement expressions (which works on clang),
> but there are still bugs remaining related to structs
> with VLA members (which seems to be a FE bug).
>
> Of course, I might be doing something stupid...
>
> The patch survives bootstrapping and regresstion testing
> on x86_64.
Including Ada?
> Fix ICE when mixing VLAs and statement expressions [PR91038]
>
> When returning VM-types from statement expressions, this can
> lead to an ICE when declarations from the statement expression
> are referred to later. Some of these issues can be addressed by
> gimplifying the base expression earlier in gimplify_compound_lval.
> This fixes PR91038 and some of the test cases from PR29970
> (structs with VLA members need further work).
>
>
> 2021-08-01 Martin Uecker <muecker@gwdg.de>
>
> gcc/
> PR c/91038
> PR c/29970
> * gimplify.c (gimplify_var_or_parm_decl): Update comment.
> (gimplify_compound_lval): Gimplify base expression first.
>
> gcc/testsuite/
> PR c/91038
> PR c/29970
> * gcc.dg/vla-stexp-01.c: New test.
> * gcc.dg/vla-stexp-02.c: New test.
> * gcc.dg/vla-stexp-03.c: New test.
> * gcc.dg/vla-stexp-04.c: New test.
>
>
> diff --git a/gcc/gimplify.c b/gcc/gimplify.c
> index 21ff32ee4aa..885d5f73585 100644
> --- a/gcc/gimplify.c
> +++ b/gcc/gimplify.c
> @@ -2839,7 +2839,10 @@ gimplify_var_or_parm_decl (tree *expr_p)
> declaration, for which we've already issued an error. It would
> be really nice if the front end wouldn't leak these at all.
> Currently the only known culprit is C++ destructors, as seen
> - in g++.old-deja/g++.jason/binding.C. */
> + in g++.old-deja/g++.jason/binding.C.
> + Another culpit are size expressions for variably modified
> + types which are lost in the FE or not gimplified correctly.
> + */
> if (VAR_P (decl)
> && !DECL_SEEN_IN_BIND_EXPR_P (decl)
> && !TREE_STATIC (decl) && !DECL_EXTERNAL (decl)
> @@ -2984,9 +2987,23 @@ gimplify_compound_lval (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p,
> expression until we deal with any variable bounds, sizes, or
> positions in order to deal with PLACEHOLDER_EXPRs.
>
> - So we do this in three steps. First we deal with the annotations
> - for any variables in the components, then we gimplify the base,
> - then we gimplify any indices, from left to right. */
> + So we do this in three steps. First we gimplify the base,
> + then we deal with the annotations for any variables in the
> + components, then we gimplify any indices, from left to right.
> +
> + The base expression may contain a statement expression that
> + has declarations used in size expressions, so has to be
> + gimplified first. */
The previous paragraph says,
> But we can't gimplify the inner
> expression until we deal with any variable bounds, sizes, or
> positions in order to deal with PLACEHOLDER_EXPRs.
so I would expect your change to break examples that the current code
was designed to handle. The change to delay gimplifying the inner
expression was in r0-59131 (SVN r83474), by Richard Kenner. But there
aren't any testcases in that commit. Richard, any insight? Can you
review this patch?
> + /* Step 1 is to gimplify the base expression. Make sure lvalue is set
> + so as to match the min_lval predicate. Failure to do so may result
> + in the creation of large aggregate temporaries. */
> + tret = gimplify_expr (p, pre_p, post_p, is_gimple_min_lval,
> + fallback | fb_lvalue);
> +
> + ret = MIN (ret, tret);
> +
> +
> for (i = expr_stack.length () - 1; i >= 0; i--)
> {
> tree t = expr_stack[i];
> @@ -3076,12 +3093,6 @@ gimplify_compound_lval (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p,
> }
> }
>
> - /* Step 2 is to gimplify the base expression. Make sure lvalue is set
> - so as to match the min_lval predicate. Failure to do so may result
> - in the creation of large aggregate temporaries. */
> - tret = gimplify_expr (p, pre_p, post_p, is_gimple_min_lval,
> - fallback | fb_lvalue);
> - ret = MIN (ret, tret);
>
> /* And finally, the indices and operands of ARRAY_REF. During this
> loop we also remove any useless conversions. */
> diff --git a/gcc/testsuite/gcc.dg/vla-stexpr-01.c b/gcc/testsuite/gcc.dg/vla-stexpr-01.c
> new file mode 100644
> index 00000000000..27e1817eb63
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/vla-stexpr-01.c
> @@ -0,0 +1,94 @@
> +/* PR29970, PR91038 */
> +/* { dg-do run } */
> +/* { dg-options "-O0" } */
> +
> +int foo3b(void) // should not return 0
> +{
> + int n = 0;
> + return sizeof *({ n = 10; int x[n]; x; &x; });
> +}
> +
> +int foo4(void) // should not ICE
> +{
> + return (*({
> + int n = 20;
> + char (*x)[n][n] = __builtin_malloc(n * n);
> + (*x)[12][1] = 1;
> + x;
> + }))[12][1];
> +}
> +
> +int foo5(void) // should return 1, returns 0
> +{
> + int n = 0;
> + return (*({
> + n = 20;
> + char (*x)[n][n] = __builtin_malloc(n * n);
> + (*x)[12][1] = 1;
> + (*x)[0][1] = 0;
> + x;
> + }))[12][1];
> +}
> +
> +int foo5c(void) // should return 400
> +{
> + int n = 0;
> + return sizeof(*({
> + n = 20;
> + char (*x)[n][n] = __builtin_malloc(n * n);
> + (*x)[12][1] = 1;
> + (*x)[0][1] = 0;
> + x;
> + }));
> +}
> +
> +int foo5b(void) // should return 1, returns 0
> +{
> + int n = 0;
> + return (*({
> + int n = 20;
> + char (*x)[n][n] = __builtin_malloc(n * n);
> + (*x)[12][1] = 1;
> + (*x)[0][1] = 0;
> + x;
> + }))[12][1];
> +}
> +
> +int foo5a(void) // should return 1, returns 0
> +{
> + return (*({
> + int n = 20;
> + char (*x)[n][n] = __builtin_malloc(n * n);
> + (*x)[12][1] = 1;
> + (*x)[0][1] = 0;
> + x;
> + }))[12][1];
> +}
> +
> +
> +
> +
> +int main()
> +{
> + if (sizeof(int[10]) != foo3b())
> + __builtin_abort();
> +
> + if (1 != foo4())
> + __builtin_abort();
> +
> + if (400 != foo5c())
> + __builtin_abort();
> +
> + if (1 != foo5a())
> + __builtin_abort();
> +
> + if (1 != foo5b()) // -O0
> + __builtin_abort();
> +
> + if (1 != foo5())
> + __builtin_abort();
> +
> + return 0;
> +}
> +
> +
> diff --git a/gcc/testsuite/gcc.dg/vla-stexpr-02.c b/gcc/testsuite/gcc.dg/vla-stexpr-02.c
> new file mode 100644
> index 00000000000..99b4f571e52
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/vla-stexpr-02.c
> @@ -0,0 +1,31 @@
> +/* PR29970 */
> +/* { dg-do run } */
> +/* { dg-options "" } */
> +
> +
> +
> +
> +int foo2a(void) // should not ICE
> +{
> + return ({ int n = 20; struct { int x[n];} x; x.x[12] = 1; sizeof(x); });
> +}
> +
> +#if 0
> +int foo2b(void) // should not ICE
> +{
> + return sizeof *({ int n = 20; struct { int x[n];} x; x.x[12] = 1; &x; });
> +}
> +#endif
> +
> +int main()
> +{
> + if (sizeof(struct { int x[20]; }) != foo2a())
> + __builtin_abort();
> +#if 0
> + if (sizeof(struct { int x[20]; }) != foo2b())
> + __builtin_abort();
> +#endif
> + return 0;
> +}
> +
> +
> diff --git a/gcc/testsuite/gcc.dg/vla-stexpr-03.c b/gcc/testsuite/gcc.dg/vla-stexpr-03.c
> new file mode 100644
> index 00000000000..61f8986eaa3
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/vla-stexpr-03.c
> @@ -0,0 +1,94 @@
> +/* PR29970, PR91038 */
> +/* { dg-do run } */
> +/* { dg-options "-O2" } */
> +
> +int foo3b(void) // should not return 0
> +{
> + int n = 0;
> + return sizeof *({ n = 10; int x[n]; x; &x; });
> +}
> +
> +int foo4(void) // should not ICE
> +{
> + return (*({
> + int n = 20;
> + char (*x)[n][n] = __builtin_malloc(n * n);
> + (*x)[12][1] = 1;
> + x;
> + }))[12][1];
> +}
> +
> +int foo5(void) // should return 1, returns 0
> +{
> + int n = 0;
> + return (*({
> + n = 20;
> + char (*x)[n][n] = __builtin_malloc(n * n);
> + (*x)[12][1] = 1;
> + (*x)[0][1] = 0;
> + x;
> + }))[12][1];
> +}
> +
> +int foo5c(void) // should return 400
> +{
> + int n = 0;
> + return sizeof(*({
> + n = 20;
> + char (*x)[n][n] = __builtin_malloc(n * n);
> + (*x)[12][1] = 1;
> + (*x)[0][1] = 0;
> + x;
> + }));
> +}
> +
> +int foo5b(void) // should return 1, returns 0
> +{
> + int n = 0;
> + return (*({
> + int n = 20;
> + char (*x)[n][n] = __builtin_malloc(n * n);
> + (*x)[12][1] = 1;
> + (*x)[0][1] = 0;
> + x;
> + }))[12][1];
> +}
> +
> +int foo5a(void) // should return 1, returns 0
> +{
> + return (*({
> + int n = 20;
> + char (*x)[n][n] = __builtin_malloc(n * n);
> + (*x)[12][1] = 1;
> + (*x)[0][1] = 0;
> + x;
> + }))[12][1];
> +}
> +
> +
> +
> +
> +int main()
> +{
> + if (sizeof(int[10]) != foo3b())
> + __builtin_abort();
> +
> + if (1 != foo4())
> + __builtin_abort();
> +
> + if (400 != foo5c())
> + __builtin_abort();
> +
> + if (1 != foo5a())
> + __builtin_abort();
> +
> + if (1 != foo5b()) // -O0
> + __builtin_abort();
> +
> + if (1 != foo5())
> + __builtin_abort();
> +
> + return 0;
> +}
> +
> +
> diff --git a/gcc/testsuite/gcc.dg/vla-stexpr-04.c b/gcc/testsuite/gcc.dg/vla-stexpr-04.c
> new file mode 100644
> index 00000000000..ffc8b12fa9b
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/vla-stexpr-04.c
> @@ -0,0 +1,38 @@
> +/* PR91038 */
> +/* { dg-do run } */
> +/* { dg-options "" } */
> +
> +
> +#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
> +
> +struct lbm {
> +
> + int D;
> + const int* DQ;
> +
> +} D2Q9 = { 2,
> + (const int*)&(const int[9][2]){
> + { 0, 0 },
> + { 1, 0 }, { 0, 1 }, { -1, 0 }, { 0, -1 },
> + { 1, 1 }, { -1, 1 }, { -1, -1 }, { 1, -1 },
> + }
> +};
> +
> +void zouhe_left(void)
> +{
> + __auto_type xx = (*({ int N = 2; struct lbm __x = D2Q9; ((const int(*)[9][N])__x.DQ); }));
> +
> + if (1 != xx[1][0])
> + __builtin_abort();
> +
> + if (2 != ARRAY_SIZE(xx[1]))
> + __builtin_abort();
> +}
> +
> +int main()
> +{
> + zouhe_left();
> + return 0;
> +}
> +
> +
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Fix ICE when mixing VLAs and statement expressions [PR91038]
2021-08-16 4:30 ` Jason Merrill
@ 2021-08-16 4:49 ` Uecker, Martin
2021-08-23 6:15 ` Uecker, Martin
0 siblings, 1 reply; 8+ messages in thread
From: Uecker, Martin @ 2021-08-16 4:49 UTC (permalink / raw)
To: jason, kenner; +Cc: gcc-patches
Am Montag, den 16.08.2021, 00:30 -0400 schrieb Jason Merrill:
> On 8/1/21 1:36 PM, Uecker, Martin wrote:
> >
> > Here is an attempt to fix some old and annoying bugs related
> > to VLAs and statement expressions. In particulary, this seems
> > to fix the issues with variably-modified types which are
> > returned from statement expressions (which works on clang),
> > but there are still bugs remaining related to structs
> > with VLA members (which seems to be a FE bug).
> >
> > Of course, I might be doing something stupid...
> >
> > The patch survives bootstrapping and regresstion testing
> > on x86_64.
>
> Including Ada?
It broke PLACEHOLDER_EXPRs as you pointed out below.
Please take a look at the new version:
https://gcc.gnu.org/pipermail/gcc-patches/2021-August/577402.html
Martin
> > Fix ICE when mixing VLAs and statement expressions [PR91038]
> >
> > When returning VM-types from statement expressions, this can
> > lead to an ICE when declarations from the statement expression
> > are referred to later. Some of these issues can be addressed by
> > gimplifying the base expression earlier in gimplify_compound_lval.
> > This fixes PR91038 and some of the test cases from PR29970
> > (structs with VLA members need further work).
> >
> >
> > 2021-08-01 Martin Uecker <muecker@gwdg.de>
> >
> > gcc/
> > PR c/91038
> > PR c/29970
> > * gimplify.c (gimplify_var_or_parm_decl): Update comment.
> > (gimplify_compound_lval): Gimplify base expression first.
> >
> > gcc/testsuite/
> > PR c/91038
> > PR c/29970
> > * gcc.dg/vla-stexp-01.c: New test.
> > * gcc.dg/vla-stexp-02.c: New test.
> > * gcc.dg/vla-stexp-03.c: New test.
> > * gcc.dg/vla-stexp-04.c: New test.
> >
> >
> > diff --git a/gcc/gimplify.c b/gcc/gimplify.c
> > index 21ff32ee4aa..885d5f73585 100644
> > --- a/gcc/gimplify.c
> > +++ b/gcc/gimplify.c
> > @@ -2839,7 +2839,10 @@ gimplify_var_or_parm_decl (tree *expr_p)
> > declaration, for which we've already issued an error. It would
> > be really nice if the front end wouldn't leak these at all.
> > Currently the only known culprit is C++ destructors, as seen
> > - in g++.old-deja/g++.jason/binding.C. */
> > + in g++.old-deja/g++.jason/binding.C.
> > + Another culpit are size expressions for variably modified
> > + types which are lost in the FE or not gimplified correctly.
> > + */
> > if (VAR_P (decl)
> > && !DECL_SEEN_IN_BIND_EXPR_P (decl)
> > && !TREE_STATIC (decl) && !DECL_EXTERNAL (decl)
> > @@ -2984,9 +2987,23 @@ gimplify_compound_lval (tree *expr_p, gimple_seq *pre_p, gimple_seq
> > *post_p,
> > expression until we deal with any variable bounds, sizes, or
> > positions in order to deal with PLACEHOLDER_EXPRs.
> >
> > - So we do this in three steps. First we deal with the annotations
> > - for any variables in the components, then we gimplify the base,
> > - then we gimplify any indices, from left to right. */
> > + So we do this in three steps. First we gimplify the base,
> > + then we deal with the annotations for any variables in the
> > + components, then we gimplify any indices, from left to right.
> > +
> > + The base expression may contain a statement expression that
> > + has declarations used in size expressions, so has to be
> > + gimplified first. */
>
> The previous paragraph says,
>
> > But we can't gimplify the
> > inner
> > expression until we deal with any variable bounds, sizes,
> > or
> > positions in order to deal with PLACEHOLDER_EXPRs.
>
> so I would expect your change to break examples that the current code
> was designed to handle. The change to delay gimplifying the inner
> expression was in r0-59131 (SVN r83474), by Richard Kenner. But there
> aren't any testcases in that commit. Richard, any insight? Can you
> review this patch?
> > + /* Step 1 is to gimplify the base expression. Make sure lvalue is set
> > + so as to match the min_lval predicate. Failure to do so may result
> > + in the creation of large aggregate temporaries. */
> > + tret = gimplify_expr (p, pre_p, post_p, is_gimple_min_lval,
> > + fallback | fb_lvalue);
> > +
> > + ret = MIN (ret, tret);
> > +
> > +
> > for (i = expr_stack.length () - 1; i >= 0; i--)
> > {
> > tree t = expr_stack[i];
> > @@ -3076,12 +3093,6 @@ gimplify_compound_lval (tree *expr_p, gimple_seq *pre_p, gimple_seq
> > *post_p,
> > }
> > }
> >
> > - /* Step 2 is to gimplify the base expression. Make sure lvalue is set
> > - so as to match the min_lval predicate. Failure to do so may result
> > - in the creation of large aggregate temporaries. */
> > - tret = gimplify_expr (p, pre_p, post_p, is_gimple_min_lval,
> > - fallback | fb_lvalue);
> > - ret = MIN (ret, tret);
> >
> > /* And finally, the indices and operands of ARRAY_REF. During this
> > loop we also remove any useless conversions. */
> > diff --git a/gcc/testsuite/gcc.dg/vla-stexpr-01.c b/gcc/testsuite/gcc.dg/vla-stexpr-01.c
> > new file mode 100644
> > index 00000000000..27e1817eb63
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.dg/vla-stexpr-01.c
> > @@ -0,0 +1,94 @@
> > +/* PR29970, PR91038 */
> > +/* { dg-do run } */
> > +/* { dg-options "-O0" } */
> > +
> > +int foo3b(void) // should not return 0
> > +{
> > + int n = 0;
> > + return sizeof *({ n = 10; int x[n]; x; &x; });
> > +}
> > +
> > +int foo4(void) // should not ICE
> > +{
> > + return (*({
> > + int n = 20;
> > + char (*x)[n][n] = __builtin_malloc(n * n);
> > + (*x)[12][1] = 1;
> > + x;
> > + }))[12][1];
> > +}
> > +
> > +int foo5(void) // should return 1, returns 0
> > +{
> > + int n = 0;
> > + return (*({
> > + n = 20;
> > + char (*x)[n][n] = __builtin_malloc(n * n);
> > + (*x)[12][1] = 1;
> > + (*x)[0][1] = 0;
> > + x;
> > + }))[12][1];
> > +}
> > +
> > +int foo5c(void) // should return 400
> > +{
> > + int n = 0;
> > + return sizeof(*({
> > + n = 20;
> > + char (*x)[n][n] = __builtin_malloc(n * n);
> > + (*x)[12][1] = 1;
> > + (*x)[0][1] = 0;
> > + x;
> > + }));
> > +}
> > +
> > +int foo5b(void) // should return 1, returns 0
> > +{
> > + int n = 0;
> > + return (*({
> > + int n = 20;
> > + char (*x)[n][n] = __builtin_malloc(n * n);
> > + (*x)[12][1] = 1;
> > + (*x)[0][1] = 0;
> > + x;
> > + }))[12][1];
> > +}
> > +
> > +int foo5a(void) // should return 1, returns 0
> > +{
> > + return (*({
> > + int n = 20;
> > + char (*x)[n][n] = __builtin_malloc(n * n);
> > + (*x)[12][1] = 1;
> > + (*x)[0][1] = 0;
> > + x;
> > + }))[12][1];
> > +}
> > +
> > +
> > +
> > +
> > +int main()
> > +{
> > + if (sizeof(int[10]) != foo3b())
> > + __builtin_abort();
> > +
> > + if (1 != foo4())
> > + __builtin_abort();
> > +
> > + if (400 != foo5c())
> > + __builtin_abort();
> > +
> > + if (1 != foo5a())
> > + __builtin_abort();
> > +
> > + if (1 != foo5b()) // -O0
> > + __builtin_abort();
> > +
> > + if (1 != foo5())
> > + __builtin_abort();
> > +
> > + return 0;
> > +}
> > +
> > +
> > diff --git a/gcc/testsuite/gcc.dg/vla-stexpr-02.c b/gcc/testsuite/gcc.dg/vla-stexpr-02.c
> > new file mode 100644
> > index 00000000000..99b4f571e52
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.dg/vla-stexpr-02.c
> > @@ -0,0 +1,31 @@
> > +/* PR29970 */
> > +/* { dg-do run } */
> > +/* { dg-options "" } */
> > +
> > +
> > +
> > +
> > +int foo2a(void) // should not ICE
> > +{
> > + return ({ int n = 20; struct { int x[n];} x; x.x[12] = 1; sizeof(x); });
> > +}
> > +
> > +#if 0
> > +int foo2b(void) // should not ICE
> > +{
> > + return sizeof *({ int n = 20; struct { int x[n];} x; x.x[12] = 1; &x; });
> > +}
> > +#endif
> > +
> > +int main()
> > +{
> > + if (sizeof(struct { int x[20]; }) != foo2a())
> > + __builtin_abort();
> > +#if 0
> > + if (sizeof(struct { int x[20]; }) != foo2b())
> > + __builtin_abort();
> > +#endif
> > + return 0;
> > +}
> > +
> > +
> > diff --git a/gcc/testsuite/gcc.dg/vla-stexpr-03.c b/gcc/testsuite/gcc.dg/vla-stexpr-03.c
> > new file mode 100644
> > index 00000000000..61f8986eaa3
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.dg/vla-stexpr-03.c
> > @@ -0,0 +1,94 @@
> > +/* PR29970, PR91038 */
> > +/* { dg-do run } */
> > +/* { dg-options "-O2" } */
> > +
> > +int foo3b(void) // should not return 0
> > +{
> > + int n = 0;
> > + return sizeof *({ n = 10; int x[n]; x; &x; });
> > +}
> > +
> > +int foo4(void) // should not ICE
> > +{
> > + return (*({
> > + int n = 20;
> > + char (*x)[n][n] = __builtin_malloc(n * n);
> > + (*x)[12][1] = 1;
> > + x;
> > + }))[12][1];
> > +}
> > +
> > +int foo5(void) // should return 1, returns 0
> > +{
> > + int n = 0;
> > + return (*({
> > + n = 20;
> > + char (*x)[n][n] = __builtin_malloc(n * n);
> > + (*x)[12][1] = 1;
> > + (*x)[0][1] = 0;
> > + x;
> > + }))[12][1];
> > +}
> > +
> > +int foo5c(void) // should return 400
> > +{
> > + int n = 0;
> > + return sizeof(*({
> > + n = 20;
> > + char (*x)[n][n] = __builtin_malloc(n * n);
> > + (*x)[12][1] = 1;
> > + (*x)[0][1] = 0;
> > + x;
> > + }));
> > +}
> > +
> > +int foo5b(void) // should return 1, returns 0
> > +{
> > + int n = 0;
> > + return (*({
> > + int n = 20;
> > + char (*x)[n][n] = __builtin_malloc(n * n);
> > + (*x)[12][1] = 1;
> > + (*x)[0][1] = 0;
> > + x;
> > + }))[12][1];
> > +}
> > +
> > +int foo5a(void) // should return 1, returns 0
> > +{
> > + return (*({
> > + int n = 20;
> > + char (*x)[n][n] = __builtin_malloc(n * n);
> > + (*x)[12][1] = 1;
> > + (*x)[0][1] = 0;
> > + x;
> > + }))[12][1];
> > +}
> > +
> > +
> > +
> > +
> > +int main()
> > +{
> > + if (sizeof(int[10]) != foo3b())
> > + __builtin_abort();
> > +
> > + if (1 != foo4())
> > + __builtin_abort();
> > +
> > + if (400 != foo5c())
> > + __builtin_abort();
> > +
> > + if (1 != foo5a())
> > + __builtin_abort();
> > +
> > + if (1 != foo5b()) // -O0
> > + __builtin_abort();
> > +
> > + if (1 != foo5())
> > + __builtin_abort();
> > +
> > + return 0;
> > +}
> > +
> > +
> > diff --git a/gcc/testsuite/gcc.dg/vla-stexpr-04.c b/gcc/testsuite/gcc.dg/vla-stexpr-04.c
> > new file mode 100644
> > index 00000000000..ffc8b12fa9b
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.dg/vla-stexpr-04.c
> > @@ -0,0 +1,38 @@
> > +/* PR91038 */
> > +/* { dg-do run } */
> > +/* { dg-options "" } */
> > +
> > +
> > +#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
> > +
> > +struct lbm {
> > +
> > + int D;
> > + const int* DQ;
> > +
> > +} D2Q9 = { 2,
> > + (const int*)&(const int[9][2]){
> > + { 0, 0 },
> > + { 1, 0 }, { 0, 1 }, { -1, 0 }, { 0, -1 },
> > + { 1, 1 }, { -1, 1 }, { -1, -1 }, { 1, -1 },
> > + }
> > +};
> > +
> > +void zouhe_left(void)
> > +{
> > + __auto_type xx = (*({ int N = 2; struct lbm __x = D2Q9; ((const int(*)[9][N])__x.DQ); }));
> > +
> > + if (1 != xx[1][0])
> > + __builtin_abort();
> > +
> > + if (2 != ARRAY_SIZE(xx[1]))
> > + __builtin_abort();
> > +}
> > +
> > +int main()
> > +{
> > + zouhe_left();
> > + return 0;
> > +}
> > +
> > +
> >
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Fix ICE when mixing VLAs and statement expressions [PR91038]
2021-08-16 4:49 ` Uecker, Martin
@ 2021-08-23 6:15 ` Uecker, Martin
0 siblings, 0 replies; 8+ messages in thread
From: Uecker, Martin @ 2021-08-23 6:15 UTC (permalink / raw)
To: jason, kenner; +Cc: gcc-patches
Am Montag, den 16.08.2021, 06:49 +0200 schrieb Martin Uecker:
> Am Montag, den 16.08.2021, 00:30 -0400 schrieb Jason Merrill:
> > On 8/1/21 1:36 PM, Uecker, Martin wrote:
> > > Here is an attempt to fix some old and annoying bugs related
> > > to VLAs and statement expressions. In particulary, this seems
> > > to fix the issues with variably-modified types which are
> > > returned from statement expressions (which works on clang),
> > > but there are still bugs remaining related to structs
> > > with VLA members (which seems to be a FE bug).
> > >
> > > Of course, I might be doing something stupid...
> > >
> > > The patch survives bootstrapping and regresstion testing
> > > on x86_64.
> >
> > Including Ada?
>
> It broke PLACEHOLDER_EXPRs as you pointed out below.
>
> Please take a look at the new version:
>
> https://gcc.gnu.org/pipermail/gcc-patches/2021-August/577402.html
Richard,
any comments on this version of the patch?
Thank you!
Martin
> Martin
>
>
>
> > > Fix ICE when mixing VLAs and statement expressions [PR91038]
> > >
> > > When returning VM-types from statement expressions, this can
> > > lead to an ICE when declarations from the statement expression
> > > are referred to later. Some of these issues can be addressed by
> > > gimplifying the base expression earlier in gimplify_compound_lval.
> > > This fixes PR91038 and some of the test cases from PR29970
> > > (structs with VLA members need further work).
> > >
> > >
> > > 2021-08-01 Martin Uecker <muecker@gwdg.de>
> > >
> > > gcc/
> > > PR c/91038
> > > PR c/29970
> > > * gimplify.c (gimplify_var_or_parm_decl): Update comment.
> > > (gimplify_compound_lval): Gimplify base expression first.
> > >
> > > gcc/testsuite/
> > > PR c/91038
> > > PR c/29970
> > > * gcc.dg/vla-stexp-01.c: New test.
> > > * gcc.dg/vla-stexp-02.c: New test.
> > > * gcc.dg/vla-stexp-03.c: New test.
> > > * gcc.dg/vla-stexp-04.c: New test.
> > >
> > >
> > > diff --git a/gcc/gimplify.c b/gcc/gimplify.c
> > > index 21ff32ee4aa..885d5f73585 100644
> > > --- a/gcc/gimplify.c
> > > +++ b/gcc/gimplify.c
> > > @@ -2839,7 +2839,10 @@ gimplify_var_or_parm_decl (tree *expr_p)
> > > declaration, for which we've already issued an error. It would
> > > be really nice if the front end wouldn't leak these at all.
> > > Currently the only known culprit is C++ destructors, as seen
> > > - in g++.old-deja/g++.jason/binding.C. */
> > > + in g++.old-deja/g++.jason/binding.C.
> > > + Another culpit are size expressions for variably modified
> > > + types which are lost in the FE or not gimplified correctly.
> > > + */
> > > if (VAR_P (decl)
> > > && !DECL_SEEN_IN_BIND_EXPR_P (decl)
> > > && !TREE_STATIC (decl) && !DECL_EXTERNAL (decl)
> > > @@ -2984,9 +2987,23 @@ gimplify_compound_lval (tree *expr_p, gimple_seq *pre_p, gimple_seq
> > > *post_p,
> > > expression until we deal with any variable bounds, sizes, or
> > > positions in order to deal with PLACEHOLDER_EXPRs.
> > >
> > > - So we do this in three steps. First we deal with the annotations
> > > - for any variables in the components, then we gimplify the base,
> > > - then we gimplify any indices, from left to right. */
> > > + So we do this in three steps. First we gimplify the base,
> > > + then we deal with the annotations for any variables in the
> > > + components, then we gimplify any indices, from left to right.
> > > +
> > > + The base expression may contain a statement expression that
> > > + has declarations used in size expressions, so has to be
> > > + gimplified first. */
> >
> > The previous paragraph says,
> >
> > > But we can't gimplify the
> > > inner
> > > expression until we deal with any variable bounds, sizes,
> > > or
> > > positions in order to deal with PLACEHOLDER_EXPRs.
> >
> > so I would expect your change to break examples that the current code
> > was designed to handle. The change to delay gimplifying the inner
> > expression was in r0-59131 (SVN r83474), by Richard Kenner. But there
> > aren't any testcases in that commit. Richard, any insight? Can you
> > review this patch?
>
>
> > > + /* Step 1 is to gimplify the base expression. Make sure lvalue is set
> > > + so as to match the min_lval predicate. Failure to do so may result
> > > + in the creation of large aggregate temporaries. */
> > > + tret = gimplify_expr (p, pre_p, post_p, is_gimple_min_lval,
> > > + fallback | fb_lvalue);
> > > +
> > > + ret = MIN (ret, tret);
> > > +
> > > +
> > > for (i = expr_stack.length () - 1; i >= 0; i--)
> > > {
> > > tree t = expr_stack[i];
> > > @@ -3076,12 +3093,6 @@ gimplify_compound_lval (tree *expr_p, gimple_seq *pre_p, gimple_seq
> > > *post_p,
> > > }
> > > }
> > >
> > > - /* Step 2 is to gimplify the base expression. Make sure lvalue is set
> > > - so as to match the min_lval predicate. Failure to do so may result
> > > - in the creation of large aggregate temporaries. */
> > > - tret = gimplify_expr (p, pre_p, post_p, is_gimple_min_lval,
> > > - fallback | fb_lvalue);
> > > - ret = MIN (ret, tret);
> > >
> > > /* And finally, the indices and operands of ARRAY_REF. During this
> > > loop we also remove any useless conversions. */
> > > diff --git a/gcc/testsuite/gcc.dg/vla-stexpr-01.c b/gcc/testsuite/gcc.dg/vla-stexpr-01.c
> > > new file mode 100644
> > > index 00000000000..27e1817eb63
> > > --- /dev/null
> > > +++ b/gcc/testsuite/gcc.dg/vla-stexpr-01.c
> > > @@ -0,0 +1,94 @@
> > > +/* PR29970, PR91038 */
> > > +/* { dg-do run } */
> > > +/* { dg-options "-O0" } */
> > > +
> > > +int foo3b(void) // should not return 0
> > > +{
> > > + int n = 0;
> > > + return sizeof *({ n = 10; int x[n]; x; &x; });
> > > +}
> > > +
> > > +int foo4(void) // should not ICE
> > > +{
> > > + return (*({
> > > + int n = 20;
> > > + char (*x)[n][n] = __builtin_malloc(n * n);
> > > + (*x)[12][1] = 1;
> > > + x;
> > > + }))[12][1];
> > > +}
> > > +
> > > +int foo5(void) // should return 1, returns 0
> > > +{
> > > + int n = 0;
> > > + return (*({
> > > + n = 20;
> > > + char (*x)[n][n] = __builtin_malloc(n * n);
> > > + (*x)[12][1] = 1;
> > > + (*x)[0][1] = 0;
> > > + x;
> > > + }))[12][1];
> > > +}
> > > +
> > > +int foo5c(void) // should return 400
> > > +{
> > > + int n = 0;
> > > + return sizeof(*({
> > > + n = 20;
> > > + char (*x)[n][n] = __builtin_malloc(n * n);
> > > + (*x)[12][1] = 1;
> > > + (*x)[0][1] = 0;
> > > + x;
> > > + }));
> > > +}
> > > +
> > > +int foo5b(void) // should return 1, returns 0
> > > +{
> > > + int n = 0;
> > > + return (*({
> > > + int n = 20;
> > > + char (*x)[n][n] = __builtin_malloc(n * n);
> > > + (*x)[12][1] = 1;
> > > + (*x)[0][1] = 0;
> > > + x;
> > > + }))[12][1];
> > > +}
> > > +
> > > +int foo5a(void) // should return 1, returns 0
> > > +{
> > > + return (*({
> > > + int n = 20;
> > > + char (*x)[n][n] = __builtin_malloc(n * n);
> > > + (*x)[12][1] = 1;
> > > + (*x)[0][1] = 0;
> > > + x;
> > > + }))[12][1];
> > > +}
> > > +
> > > +
> > > +
> > > +
> > > +int main()
> > > +{
> > > + if (sizeof(int[10]) != foo3b())
> > > + __builtin_abort();
> > > +
> > > + if (1 != foo4())
> > > + __builtin_abort();
> > > +
> > > + if (400 != foo5c())
> > > + __builtin_abort();
> > > +
> > > + if (1 != foo5a())
> > > + __builtin_abort();
> > > +
> > > + if (1 != foo5b()) // -O0
> > > + __builtin_abort();
> > > +
> > > + if (1 != foo5())
> > > + __builtin_abort();
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +
> > > diff --git a/gcc/testsuite/gcc.dg/vla-stexpr-02.c b/gcc/testsuite/gcc.dg/vla-stexpr-02.c
> > > new file mode 100644
> > > index 00000000000..99b4f571e52
> > > --- /dev/null
> > > +++ b/gcc/testsuite/gcc.dg/vla-stexpr-02.c
> > > @@ -0,0 +1,31 @@
> > > +/* PR29970 */
> > > +/* { dg-do run } */
> > > +/* { dg-options "" } */
> > > +
> > > +
> > > +
> > > +
> > > +int foo2a(void) // should not ICE
> > > +{
> > > + return ({ int n = 20; struct { int x[n];} x; x.x[12] = 1; sizeof(x); });
> > > +}
> > > +
> > > +#if 0
> > > +int foo2b(void) // should not ICE
> > > +{
> > > + return sizeof *({ int n = 20; struct { int x[n];} x; x.x[12] = 1; &x; });
> > > +}
> > > +#endif
> > > +
> > > +int main()
> > > +{
> > > + if (sizeof(struct { int x[20]; }) != foo2a())
> > > + __builtin_abort();
> > > +#if 0
> > > + if (sizeof(struct { int x[20]; }) != foo2b())
> > > + __builtin_abort();
> > > +#endif
> > > + return 0;
> > > +}
> > > +
> > > +
> > > diff --git a/gcc/testsuite/gcc.dg/vla-stexpr-03.c b/gcc/testsuite/gcc.dg/vla-stexpr-03.c
> > > new file mode 100644
> > > index 00000000000..61f8986eaa3
> > > --- /dev/null
> > > +++ b/gcc/testsuite/gcc.dg/vla-stexpr-03.c
> > > @@ -0,0 +1,94 @@
> > > +/* PR29970, PR91038 */
> > > +/* { dg-do run } */
> > > +/* { dg-options "-O2" } */
> > > +
> > > +int foo3b(void) // should not return 0
> > > +{
> > > + int n = 0;
> > > + return sizeof *({ n = 10; int x[n]; x; &x; });
> > > +}
> > > +
> > > +int foo4(void) // should not ICE
> > > +{
> > > + return (*({
> > > + int n = 20;
> > > + char (*x)[n][n] = __builtin_malloc(n * n);
> > > + (*x)[12][1] = 1;
> > > + x;
> > > + }))[12][1];
> > > +}
> > > +
> > > +int foo5(void) // should return 1, returns 0
> > > +{
> > > + int n = 0;
> > > + return (*({
> > > + n = 20;
> > > + char (*x)[n][n] = __builtin_malloc(n * n);
> > > + (*x)[12][1] = 1;
> > > + (*x)[0][1] = 0;
> > > + x;
> > > + }))[12][1];
> > > +}
> > > +
> > > +int foo5c(void) // should return 400
> > > +{
> > > + int n = 0;
> > > + return sizeof(*({
> > > + n = 20;
> > > + char (*x)[n][n] = __builtin_malloc(n * n);
> > > + (*x)[12][1] = 1;
> > > + (*x)[0][1] = 0;
> > > + x;
> > > + }));
> > > +}
> > > +
> > > +int foo5b(void) // should return 1, returns 0
> > > +{
> > > + int n = 0;
> > > + return (*({
> > > + int n = 20;
> > > + char (*x)[n][n] = __builtin_malloc(n * n);
> > > + (*x)[12][1] = 1;
> > > + (*x)[0][1] = 0;
> > > + x;
> > > + }))[12][1];
> > > +}
> > > +
> > > +int foo5a(void) // should return 1, returns 0
> > > +{
> > > + return (*({
> > > + int n = 20;
> > > + char (*x)[n][n] = __builtin_malloc(n * n);
> > > + (*x)[12][1] = 1;
> > > + (*x)[0][1] = 0;
> > > + x;
> > > + }))[12][1];
> > > +}
> > > +
> > > +
> > > +
> > > +
> > > +int main()
> > > +{
> > > + if (sizeof(int[10]) != foo3b())
> > > + __builtin_abort();
> > > +
> > > + if (1 != foo4())
> > > + __builtin_abort();
> > > +
> > > + if (400 != foo5c())
> > > + __builtin_abort();
> > > +
> > > + if (1 != foo5a())
> > > + __builtin_abort();
> > > +
> > > + if (1 != foo5b()) // -O0
> > > + __builtin_abort();
> > > +
> > > + if (1 != foo5())
> > > + __builtin_abort();
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +
> > > diff --git a/gcc/testsuite/gcc.dg/vla-stexpr-04.c b/gcc/testsuite/gcc.dg/vla-stexpr-04.c
> > > new file mode 100644
> > > index 00000000000..ffc8b12fa9b
> > > --- /dev/null
> > > +++ b/gcc/testsuite/gcc.dg/vla-stexpr-04.c
> > > @@ -0,0 +1,38 @@
> > > +/* PR91038 */
> > > +/* { dg-do run } */
> > > +/* { dg-options "" } */
> > > +
> > > +
> > > +#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
> > > +
> > > +struct lbm {
> > > +
> > > + int D;
> > > + const int* DQ;
> > > +
> > > +} D2Q9 = { 2,
> > > + (const int*)&(const int[9][2]){
> > > + { 0, 0 },
> > > + { 1, 0 }, { 0, 1 }, { -1, 0 }, { 0, -1 },
> > > + { 1, 1 }, { -1, 1 }, { -1, -1 }, { 1, -1 },
> > > + }
> > > +};
> > > +
> > > +void zouhe_left(void)
> > > +{
> > > + __auto_type xx = (*({ int N = 2; struct lbm __x = D2Q9; ((const int(*)[9][N])__x.DQ); }));
> > > +
> > > + if (1 != xx[1][0])
> > > + __builtin_abort();
> > > +
> > > + if (2 != ARRAY_SIZE(xx[1]))
> > > + __builtin_abort();
> > > +}
> > > +
> > > +int main()
> > > +{
> > > + zouhe_left();
> > > + return 0;
> > > +}
> > > +
> > > +
> > >
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Fix ICE when mixing VLAs and statement expressions [PR91038]
2021-08-09 14:13 ` Martin Uecker
@ 2021-08-10 20:33 ` Eric Botcazou
0 siblings, 0 replies; 8+ messages in thread
From: Eric Botcazou @ 2021-08-10 20:33 UTC (permalink / raw)
To: Martin Uecker; +Cc: Richard Biener, gcc-patches
> Yes, it breaks Ada. I already found this out in the meanwhile.
OK, thanks for checking.
> My understanding of this is that this is for referring
> to some field of an outer struct which is then used in the
> size expression, e.g. something like this (using
> C syntax):
>
> struct foo {
> int len;
> float (*x)[3][len];
> };
Yes, just
struct foo {
int len;
float a[len];
}
but it's also used for unconstrained array types and this seems to be the
problem here, e.g. for:
package Vect1 is
type Varray is array (Integer range <>) of Long_Float;
for Varray'Alignment use 16;
procedure Add (X, Y : not null access Varray; R : not null access Varray);
end Vect1;
package body Vect1 is
procedure Add (X, Y : not null access Varray; R : not null access Varray)
is
begin
for I in X'Range loop
R(I) := X(I) + Y(I);
end loop;
end;
end Vect1;
> But then why would you gimplify the size expression before
> the base expression? I would assume that you also want
> to process the base expression first, because it is the
> source of the struct which we access in the size expression.
For the above testcase, we have a dangling PLACEHOLDER_EXPRs
+===========================GNAT BUG DETECTED==============================+
| 12.0.0 20210805 (experimental) [master revision e314cfc371d:
5eb84b79079:ead235f60139edc6eb408d8d083cbb15e417b447] (x86_64-suse-linux) GCC
error:|
| in gimplify_expr, at gimplify.c:15019 |
| Error detected around vect1.adb:6:23
probably because array_ref_low_bound, where it is normally eliminated, cannot
do its jub properly when it is called after the base expression is gimplified.
--
Eric Botcazou
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Fix ICE when mixing VLAs and statement expressions [PR91038]
2021-08-09 13:52 ` Eric Botcazou
@ 2021-08-09 14:13 ` Martin Uecker
2021-08-10 20:33 ` Eric Botcazou
0 siblings, 1 reply; 8+ messages in thread
From: Martin Uecker @ 2021-08-09 14:13 UTC (permalink / raw)
To: Eric Botcazou, Richard Biener; +Cc: gcc-patches
Am Montag, den 09.08.2021, 15:52 +0200 schrieb Eric Botcazou:
> > I think the patch makes sense but the comment says
> >
> > Java requires that we elaborated nodes in source order. That
> > means we must gimplify the inner expression followed by each
> > of
> > the indices, in order. But we can't gimplify the inner
> > expression until we deal with any variable bounds, sizes, or
> > positions in order to deal with PLACEHOLDER_EXPRs.
> >
> > and I don't really understand that (not to say Java is no longer
> > supported
> > by GCC, but Ada does use PLACEHOLDER_EXPRs). In fact the comment
> > suggests that we _should_ gimplify the inner expression first ...
> > at least
> > it seems to after your explanations ;)
>
> We already gimplify the inner expression first, i.e. before indices
> and operands, but we gimplify the "annotations" (special operands)
> before.
>
> > Eric - do you know anything about this?
>
> If the comment is right, then Martin's patch should break Ada indeed.
>
Yes, it breaks Ada. I already found this out
in the meanwhile.
But I do not understand how this works with the
PLACEHOLDER_EXPR.
My understanding of this is that this is for referring
to some field of an outer struct which is then used in the
size expression, e.g. something like this (using
C syntax):
struct foo {
int len;
float (*x)[3][len];
};
And then for
struct foo* p;
p->x[1][1];
we use the field 'len' of the struct pointed to by 'p'
the size expression.
But then why would you gimplify the size expression before
the base expression? I would assume that you also want
to process the base expression first, because it is the
source of the struct which we access in the size expression.
Best,
Martin
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Fix ICE when mixing VLAs and statement expressions [PR91038]
2021-08-09 13:21 ` Richard Biener
@ 2021-08-09 13:52 ` Eric Botcazou
2021-08-09 14:13 ` Martin Uecker
0 siblings, 1 reply; 8+ messages in thread
From: Eric Botcazou @ 2021-08-09 13:52 UTC (permalink / raw)
To: Richard Biener; +Cc: Martin Uecker, gcc-patches, gcc-patches
> I think the patch makes sense but the comment says
>
> Java requires that we elaborated nodes in source order. That
> means we must gimplify the inner expression followed by each of
> the indices, in order. But we can't gimplify the inner
> expression until we deal with any variable bounds, sizes, or
> positions in order to deal with PLACEHOLDER_EXPRs.
>
> and I don't really understand that (not to say Java is no longer supported
> by GCC, but Ada does use PLACEHOLDER_EXPRs). In fact the comment
> suggests that we _should_ gimplify the inner expression first ... at least
> it seems to after your explanations ;)
We already gimplify the inner expression first, i.e. before indices and
operands, but we gimplify the "annotations" (special operands) before.
> Eric - do you know anything about this?
If the comment is right, then Martin's patch should break Ada indeed.
--
Eric Botcazou
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2021-08-23 6:15 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-01 17:36 [PATCH] Fix ICE when mixing VLAs and statement expressions [PR91038] Uecker, Martin
2021-08-02 10:16 ` Richard Biener
2021-08-16 4:30 ` Jason Merrill
2021-08-16 4:49 ` Uecker, Martin
2021-08-23 6:15 ` Uecker, Martin
2021-08-02 14:05 Uecker, Martin
2021-08-03 19:31 ` Martin Uecker
2021-08-09 13:21 ` Richard Biener
2021-08-09 13:52 ` Eric Botcazou
2021-08-09 14:13 ` Martin Uecker
2021-08-10 20:33 ` Eric Botcazou
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).