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 <muec...@gwdg.de>
> >
> > 2021-08-01 Martin Uecker <muec...@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
> >
