[PATCH v4] Fix ICE when mixing VLAs and statement expressions [PR91038]
Jason Merrill
jason@redhat.com
Mon Nov 8 17:13:14 GMT 2021
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
>
More information about the Gcc-patches
mailing list