On Mon, May 22, 2023 at 7:24 PM Martin Uecker via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
>
>
> This version contains the middle-end changes for PR109450
> and test cases as before. The main middle-end change is that
> we use gimplify_type_sizes also for parameters and remove
> the special code that also walked into pointers (which is
> incorrect).
>
> In addition, in the C FE this patch now also adds DECL_EXPR
> for vm-types which are pointed-to by parameters declared
> as arrays. The new function created contains the exact
> code previously used only for regular pointers, and is
> now also called for parameters declared as arrays.
>
>
> Martin
>
>
>
>
>
>
>
> Fix ICEs related to VM types in C 2/2 [PR109450]
>
> Size expressions were sometimes lost and not gimplified correctly,
> leading to ICEs and incorrect evaluation order. Fix this by 1) not
> recursing pointers when gimplifying parameters in the middle-end
> (the code is merged with gimplify_type_sizes), which is incorrect
> because it might access variables declared later for incomplete
> structs, and 2) adding a decl expr for variably-modified arrays
> that are pointed to by parameters declared as arrays.
>
> PR c/109450
>
> gcc/
> * c/c-decl.cc (add_decl_expr): New function.
> (grokdeclarator): Add decl expr for size expression in
> types pointed to by parameters declared as arrays.
> * function.cc (gimplify_parm_type): Remove function.
> (gimplify_parameters): Call gimplify_parm_sizes.
> * gimplify.cc (gimplify_type_sizes): Make function static.
> (gimplify_parm_sizes): New function.
>
> gcc/testsuite/
> * gcc.dg/pr109450-1.c: New test.
> * gcc.dg/pr109450-2.c: New test.
> * gcc.dg/vla-26.c: New test.
>
> diff --git a/gcc/c/c-decl.cc b/gcc/c/c-decl.cc
> index 494d3cf1747..c35347734b2 100644
> --- a/gcc/c/c-decl.cc
> +++ b/gcc/c/c-decl.cc
> @@ -6490,6 +6490,55 @@ smallest_type_quals_location (const location_t
> *locations,
> return loc;
> }
>
> +
> +/* We attach an artificial TYPE_DECL to pointed-to type
> + and arrange for it to be included in a DECL_EXPR. This
> + forces the sizes evaluation at a safe point and ensures it
> + is not deferred until e.g. within a deeper conditional context.
> +
> + PARM contexts have no enclosing statement list that
> + can hold the DECL_EXPR, so we need to use a BIND_EXPR
> + instead, and add it to the list of expressions that
> + need to be evaluated.
> +
> + TYPENAME contexts do have an enclosing statement list,
> + but it would be incorrect to use it, as the size should
> + only be evaluated if the containing expression is
> + evaluated. We might also be in the middle of an
> + expression with side effects on the pointed-to type size
> + "arguments" prior to the pointer declaration point and
> + the fake TYPE_DECL in the enclosing context would force
> + the size evaluation prior to the side effects. We therefore
> + use BIND_EXPRs in TYPENAME contexts too. */
> +static void
> +add_decl_expr(location_t loc, enum decl_context decl_context, tree type,
> tree *expr)
> +{
> + tree bind = NULL_TREE;
> + if (decl_context == TYPENAME || decl_context == PARM || decl_context ==
> FIELD)
> + {
> + bind = build3 (BIND_EXPR, void_type_node, NULL_TREE, NULL_TREE,
> NULL_TREE);
> + TREE_SIDE_EFFECTS (bind) = 1;
> + BIND_EXPR_BODY (bind) = push_stmt_list ();
> + push_scope ();
> + }
> +
> + tree decl = build_decl (loc, TYPE_DECL, NULL_TREE, type);
> + pushdecl (decl);
> + DECL_ARTIFICIAL (decl) = 1;
> + add_stmt (build_stmt (DECL_SOURCE_LOCATION (decl), DECL_EXPR, decl));
> + TYPE_NAME (type) = decl;
> +
> + if (bind)
> + {
> + pop_scope ();
> + BIND_EXPR_BODY (bind) = pop_stmt_list (BIND_EXPR_BODY (bind));
> + if (*expr)
> + *expr = build2 (COMPOUND_EXPR, void_type_node, *expr, bind);
> + else
> + *expr = bind;
> + }
> +}
> +
> /* Given declspecs and a declarator,
> determine the name and type of the object declared
> and construct a ..._DECL node for it.
> @@ -7474,58 +7523,9 @@ grokdeclarator (const struct c_declarator *declarator,
>
> This is expected to happen automatically when the pointed-to
> type has a name/declaration of it's own, but special attention
> - is required if the type is anonymous.
> -
> - We attach an artificial TYPE_DECL to such pointed-to type
> - and arrange for it to be included in a DECL_EXPR. This
> - forces the sizes evaluation at a safe point and ensures it
> - is not deferred until e.g. within a deeper conditional context.
> -
> - PARM contexts have no enclosing statement list that
> - can hold the DECL_EXPR, so we need to use a BIND_EXPR
> - instead, and add it to the list of expressions that
> - need to be evaluated.
> -
> - TYPENAME contexts do have an enclosing statement list,
> - but it would be incorrect to use it, as the size should
> - only be evaluated if the containing expression is
> - evaluated. We might also be in the middle of an
> - expression with side effects on the pointed-to type size
> - "arguments" prior to the pointer declaration point and
> - the fake TYPE_DECL in the enclosing context would force
> - the size evaluation prior to the side effects. We therefore
> - use BIND_EXPRs in TYPENAME contexts too. */
> - if (!TYPE_NAME (type)
> - && c_type_variably_modified_p (type))
> - {
> - tree bind = NULL_TREE;
> - if (decl_context == TYPENAME || decl_context == PARM
> - || decl_context == FIELD)
> - {
> - bind = build3 (BIND_EXPR, void_type_node, NULL_TREE,
> - NULL_TREE, NULL_TREE);
> - TREE_SIDE_EFFECTS (bind) = 1;
> - BIND_EXPR_BODY (bind) = push_stmt_list ();
> - push_scope ();
> - }
> - tree decl = build_decl (loc, TYPE_DECL, NULL_TREE, type);
> - pushdecl (decl);
> - DECL_ARTIFICIAL (decl) = 1;
> - add_stmt (build_stmt (DECL_SOURCE_LOCATION (decl), DECL_EXPR,
> decl));
> - TYPE_NAME (type) = decl;
> -
> - if (bind)
> - {
> - pop_scope ();
> - BIND_EXPR_BODY (bind)
> - = pop_stmt_list (BIND_EXPR_BODY (bind));
> - if (*expr)
> - *expr = build2 (COMPOUND_EXPR, void_type_node, *expr,
> - bind);
> - else
> - *expr = bind;
> - }
> - }
> + is required if the type is anonymous. */
> + if (!TYPE_NAME (type) && c_type_variably_modified_p (type))
> + add_decl_expr (loc, decl_context, type, expr);
>
> type = c_build_pointer_type (type);
>
> @@ -7787,6 +7787,11 @@ grokdeclarator (const struct c_declarator *declarator,
> if (type_quals)
> type = c_build_qualified_type (type, type_quals, orig_qual_type,
> orig_qual_indirect);
> +
> + /* The pointed-to type may need a decl expr (see above). */
> + if (!TYPE_NAME (type) && c_type_variably_modified_p (type))
> + add_decl_expr (loc, decl_context, type, expr);
> +
> type = c_build_pointer_type (type);
> type_quals = array_ptr_quals;
> if (type_quals)
> diff --git a/gcc/function.cc b/gcc/function.cc
> index f0ae641512d..5699b9d495d 100644
> --- a/gcc/function.cc
> +++ b/gcc/function.cc
> @@ -3872,30 +3872,6 @@ assign_parms (tree fndecl)
> }
> }
>
> -/* A subroutine of gimplify_parameters, invoked via walk_tree.
> - For all seen types, gimplify their sizes. */
> -
> -static tree
> -gimplify_parm_type (tree *tp, int *walk_subtrees, void *data)
> -{
> - tree t = *tp;
> -
> - *walk_subtrees = 0;
> - if (TYPE_P (t))
> - {
> - if (POINTER_TYPE_P (t))
> - *walk_subtrees = 1;
> - else if (TYPE_SIZE (t) && !TREE_CONSTANT (TYPE_SIZE (t))
> - && !TYPE_SIZES_GIMPLIFIED (t))
> - {
> - gimplify_type_sizes (t, (gimple_seq *) data);
> - *walk_subtrees = 1;
> - }
> - }
> -
> - return NULL;
> -}
> -
> /* Gimplify the parameter list for current_function_decl. This involves
> evaluating SAVE_EXPRs of variable sized parameters and generating code
> to implement callee-copies reference parameters. Returns a sequence of
> @@ -3931,14 +3907,7 @@ gimplify_parameters (gimple_seq *cleanup)
> SAVE_EXPRs (amongst others) onto a pending sizes list. This
> turned out to be less than manageable in the gimple world.
> Now we have to hunt them down ourselves. */
> - walk_tree_without_duplicates (&data.arg.type,
> - gimplify_parm_type, &stmts);
> -
> - if (TREE_CODE (DECL_SIZE_UNIT (parm)) != INTEGER_CST)
> - {
> - gimplify_one_sizepos (&DECL_SIZE (parm), &stmts);
> - gimplify_one_sizepos (&DECL_SIZE_UNIT (parm), &stmts);
> - }
> + gimplify_parm_sizes (parm, &stmts);
>
> if (data.arg.pass_by_reference)
> {
> diff --git a/gcc/gimplify.cc b/gcc/gimplify.cc
> index d0d16a24820..fd990c328eb 100644
> --- a/gcc/gimplify.cc
> +++ b/gcc/gimplify.cc
> @@ -242,6 +242,7 @@ static struct gimplify_omp_ctx *gimplify_omp_ctxp;
> static bool in_omp_construct;
>
> /* Forward declaration. */
> +static void gimplify_type_sizes (tree type, gimple_seq *list_p);
> static enum gimplify_status gimplify_compound_expr (tree *, gimple_seq *,
> bool);
> static hash_map<tree, tree> *oacc_declare_returns;
> static enum gimplify_status gimplify_expr (tree *, gimple_seq *, gimple_seq
> *,
> @@ -17425,7 +17426,7 @@ gimplify_expr (tree *expr_p, gimple_seq *pre_p,
> gimple_seq *post_p,
> /* Look through TYPE for variable-sized objects and gimplify each such
> size that we find. Add to LIST_P any statements generated. */
>
> -void
> +static void
> gimplify_type_sizes (tree type, gimple_seq *list_p)
> {
> if (type == NULL || type == error_mark_node)
> @@ -17533,6 +17534,21 @@ gimplify_type_sizes (tree type, gimple_seq *list_p)
> }
> }
>
> +/* Gimplify sizes in parameter declarations. */
> +
> +void
> +gimplify_parm_sizes (tree parm, gimple_seq *list_p)
Can you instead inline this at the single caller in gimplify_parameters?
It looks like both gimplify_type_sizes and gimplify_one_sizepos are
already exported. So just add the missing gimplify_type_sizes () call
at the single call site.
The middle-end changes are OK with that change.
Thanks,
Richard.
> +{
> + gimplify_type_sizes (TREE_TYPE (parm), list_p);
> +
> + if (TREE_CODE (DECL_SIZE_UNIT (parm)) != INTEGER_CST)
> + {
> + gimplify_one_sizepos (&DECL_SIZE (parm), list_p);
> + gimplify_one_sizepos (&DECL_SIZE_UNIT (parm), list_p);
> + }
> +}
> +
> +
> /* A subroutine of gimplify_type_sizes to make sure that *EXPR_P,
> a size or position, has had all of its SAVE_EXPRs evaluated.
> We add any required statements to *STMT_P. */
> diff --git a/gcc/gimplify.h b/gcc/gimplify.h
> index f4a3eea2606..17ea0580647 100644
> --- a/gcc/gimplify.h
> +++ b/gcc/gimplify.h
> @@ -78,7 +78,7 @@ extern enum gimplify_status gimplify_expr (tree *,
> gimple_seq *, gimple_seq *,
>
> int omp_construct_selector_matches (enum tree_code *, int, int *);
>
> -extern void gimplify_type_sizes (tree, gimple_seq *);
> +extern void gimplify_parm_sizes (tree, gimple_seq *);
> extern void gimplify_one_sizepos (tree *, gimple_seq *);
> extern gbind *gimplify_body (tree, bool);
> extern enum gimplify_status gimplify_arg (tree *, gimple_seq *, location_t,
> diff --git a/gcc/testsuite/gcc.dg/pr109450-1.c
> b/gcc/testsuite/gcc.dg/pr109450-1.c
> new file mode 100644
> index 00000000000..aec127f2afc
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/pr109450-1.c
> @@ -0,0 +1,21 @@
> +/* PR c/109450
> + * { dg-do run }
> + * { dg-options "-std=gnu99" }
> + * */
> +
> +int bar(int n, struct foo* x) /* { dg-warning "not be visible" } */
> +{
> + int a = n;
> + struct foo { char buf[n++]; }* p = x;
> + return a;
> +}
> +
> +int main()
> +{
> + if (1 != bar(1, 0))
> + __builtin_abort();
> +}
> +
> +
> +
> +
> diff --git a/gcc/testsuite/gcc.dg/pr109450-2.c
> b/gcc/testsuite/gcc.dg/pr109450-2.c
> new file mode 100644
> index 00000000000..06799f6df23
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/pr109450-2.c
> @@ -0,0 +1,18 @@
> +/* PR c/109450
> + * { dg-do run }
> + * { dg-options "-std=gnu99" }
> + * */
> +
> +int bar(int n, struct foo *x) /* { dg-warning "not be visible" } */
> +{
> + int a = n;
> + struct foo { char buf[a++]; }* p = x;
> + return n == a;
> +}
> +
> +int main()
> +{
> + if (bar(1, 0))
> + __builtin_abort();
> +}
> +
> diff --git a/gcc/testsuite/gcc.dg/vla-26.c b/gcc/testsuite/gcc.dg/vla-26.c
> new file mode 100644
> index 00000000000..5d2fa3e280a
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/vla-26.c
> @@ -0,0 +1,15 @@
> +/* { dg-do compile } */
> +/* { dg-options "-std=c99 -O2" } */
> +
> +void ed(int n, float s[3][n])
> +{
> + for (int i = 0; i < n; i++)
> + s[1][i];
> +}
> +
> +void e(int n, float s[3][n])
> +{
> + ed(n, s);
> +}
> +
> +
>
>
