From: "Uecker, Martin" <Martin.Uecker@med.uni-goettingen.de>
To: "jason@redhat.com" <jason@redhat.com>, "kenner@nyu.edu" <kenner@nyu.edu>
Cc: "gcc-patches@gcc.gnu.org" <gcc-patches@gcc.gnu.org>
Subject: Re: [PATCH] Fix ICE when mixing VLAs and statement expressions [PR91038]
Date: Mon, 23 Aug 2021 06:15:04 +0000 [thread overview]
Message-ID: <c29b018fdb5267b5a6e53634528e15a495a3d310.camel@med.uni-goettingen.de> (raw)
In-Reply-To: <2dbc9989ae6e859a4ec84b9d68abcd794d605594.camel@med.uni-goettingen.de>
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;
> > > +}
> > > +
> > > +
> > >
next prev parent reply other threads:[~2021-08-23 6:15 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-08-01 17:36 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 [this message]
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=c29b018fdb5267b5a6e53634528e15a495a3d310.camel@med.uni-goettingen.de \
--to=martin.uecker@med.uni-goettingen.de \
--cc=gcc-patches@gcc.gnu.org \
--cc=jason@redhat.com \
--cc=kenner@nyu.edu \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).