On Thu, Dec 31, 2015 at 10:40 AM, Patrick Palka <patr...@parcs.ath.cx> wrote:
> This patch makes it so that a gimplification failure is considered to be
> an internal error under normal circumstances, so that we otherwise avoid
> silently generating wrong code if e.g. a buggy frontend fed us a
> malformed tree.
>
> The rationale for this change is that it's better to abort compilation
> than to silently generate wrong code. During gimplification we should
> only see e.g. an error_mark_node if the frontend has already issued an
> error. Otherwise it is likely a bug in frontend.
>
> This patch, for example, turns the PR c++/68948 wrong-code bug into an
> ICE on invalid bug. During testing it also caught two latent "bugs"
> (patches 1 and 2 in this series).
>
> This series was tested on x86_64-pc-linux-gnu, with
> --enable-languages=all,ada,go,
> no new regressions.
>
> Does this seem like a reasonable invariant to add to the gimplifier?
>
> gcc/cp/ChangeLog:
>
> * cp-gimplify.c (gimplify_expr_stmt): Don't convert an
> error_mark_node to an empty statement.
>
> gcc/ChangeLog:
>
> * gimplify.c (gimplify_return_expr): Remove a redundant test
> for error_mark_node.
> (gimplify_decl_expr): Return GS_ERROR if an initializer is an
> error_mark_node.
> (gimplify_expr): Treat a gimplification failure as an internal
> error. Remove now-redundant GIMPLE_CHECKING checking code.
> ---
> gcc/cp/cp-gimplify.c | 5 +----
> gcc/gimplify.c | 27 +++++++++++++--------------
> 2 files changed, 14 insertions(+), 18 deletions(-)
>
> diff --git a/gcc/cp/cp-gimplify.c b/gcc/cp/cp-gimplify.c
> index 373c9e1..2b0aaaf 100644
> --- a/gcc/cp/cp-gimplify.c
> +++ b/gcc/cp/cp-gimplify.c
> @@ -424,16 +424,13 @@ gimplify_expr_stmt (tree *stmt_p)
> {
> tree stmt = EXPR_STMT_EXPR (*stmt_p);
>
> - if (stmt == error_mark_node)
> - stmt = NULL;
> -
> /* Gimplification of a statement expression will nullify the
> statement if all its side effects are moved to *PRE_P and *POST_P.
>
> In this case we will not want to emit the gimplified statement.
> However, we may still want to emit a warning, so we do that before
> gimplification. */
> - if (stmt && warn_unused_value)
> + if (stmt && stmt != error_mark_node && warn_unused_value)
> {
> if (!TREE_SIDE_EFFECTS (stmt))
> {
> diff --git a/gcc/gimplify.c b/gcc/gimplify.c
> index bc90401..9a1d723 100644
> --- a/gcc/gimplify.c
> +++ b/gcc/gimplify.c
> @@ -1288,8 +1288,7 @@ gimplify_return_expr (tree stmt, gimple_seq *pre_p)
> }
>
> if (!ret_expr
> - || TREE_CODE (ret_expr) == RESULT_DECL
> - || ret_expr == error_mark_node)
> + || TREE_CODE (ret_expr) == RESULT_DECL)
> {
> greturn *ret = gimple_build_return (ret_expr);
> gimple_set_no_warning (ret, TREE_NO_WARNING (stmt));
> @@ -1449,6 +1448,9 @@ gimplify_decl_expr (tree *stmt_p, gimple_seq *seq_p)
> {
> tree init = DECL_INITIAL (decl);
>
> + if (init == error_mark_node)
> + return GS_ERROR;
> +
> if (TREE_CODE (DECL_SIZE_UNIT (decl)) != INTEGER_CST
> || (!TREE_STATIC (decl)
> && flag_stack_check == GENERIC_STACK_CHECK
> @@ -1464,7 +1466,7 @@ gimplify_decl_expr (tree *stmt_p, gimple_seq *seq_p)
> && DECL_ARTIFICIAL (decl) && DECL_NAME (decl) == NULL_TREE)
> gimple_add_tmp_var (decl);
>
> - if (init && init != error_mark_node)
> + if (init)
> {
> if (!TREE_STATIC (decl))
> {
> @@ -11036,17 +11038,6 @@ gimplify_expr (tree *expr_p, gimple_seq *pre_p,
> gimple_seq *post_p,
> }
> else
> {
> -#ifdef ENABLE_GIMPLE_CHECKING
> - if (!(fallback & fb_mayfail))
> - {
> - fprintf (stderr, "gimplification failed:\n");
> - print_generic_expr (stderr, *expr_p, 0);
> - debug_tree (*expr_p);
> - internal_error ("gimplification failed");
> - }
> -#endif
> - gcc_assert (fallback & fb_mayfail);
> -
> /* If this is an asm statement, and the user asked for the
> impossible, don't die. Fail and let gimplify_asm_expr
> issue an error. */
> @@ -11064,6 +11055,14 @@ gimplify_expr (tree *expr_p, gimple_seq *pre_p,
> gimple_seq *post_p,
> }
>
> out:
> + /* If gimplification failed, then either such failure was explicitly
> permitted
> + (via the FB_MAYFAIL flag) or the frontend fed us a malformed tree, e.g.
> one
> + containing an ERROR_MARK node -- for which the FE should have already
> issued an
> + error diagnostic. Otherwise it is likely that an FE bug was triggered,
> in
> + which case it is better to abort than to risk silently generating wrong
> + code. */
> + if (ret == GS_ERROR)
> + gcc_assert ((fallback & fb_mayfail) || seen_error ());
> input_location = saved_location;
> return ret;
> }
> --
> 2.7.0.rc3.56.g64157c6.dirty
>
Ping.
