[3/3,RFC] Treat a gimplification failure as an internal error

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(-)

On Thu, Dec 31, 2015 at 10:40 AM, Patrick Palka <patrick@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.

On 01/10/2016 08:20 PM, Patrick Palka wrote:
> On Thu, Dec 31, 2015 at 10:40 AM, Patrick Palka <patrick@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.
So this passes any such error_mark_nodes through to the gimplifier, 
which will give us a nice error.  Right?

>>
>> 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.
I'd generally be in favor of a change like this; I don't offhand recall 
any rationale behind allowing gimplification to continue after hitting 
an error.

My worry is that we're potentially opening ourselves up to a slew of 
ICEs as the gimplifier as a whole I don't think has been audited to 
ensure that it handles error_mark_node is a sane fashion.

So I'd tend to want to wait for the next stage1.

jeff

On Thu, Jan 14, 2016 at 4:31 PM, Jeff Law <law@redhat.com> wrote:
> On 01/10/2016 08:20 PM, Patrick Palka wrote:
>>
>> On Thu, Dec 31, 2015 at 10:40 AM, Patrick Palka <patrick@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.
>
> So this passes any such error_mark_nodes through to the gimplifier, which
> will give us a nice error.  Right?

(Sorry for the late reply..)

Yes, this change to gimplify_expr_stmt() and the change made to
gimplfy_decl_expr() are to make sure that we propagate any relevant
internal error_mark_nodes to gimplify_expr(), which will trigger the
assertion therein.  This one is particular is pretty important since
the C++ FE seems to make a lot of EXPR_STMTs.

>
>>>
>>> gcc/ChangeLog:
>>>
>>>          * gimplify.c (gimplify_return_expr): Remove a redundant test
>>>          for error_mark_node.

This one is just a simplification -- earlier in the function we have
already tested 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.
>
> I'd generally be in favor of a change like this; I don't offhand recall any
> rationale behind allowing gimplification to continue after hitting an error.
>
> My worry is that we're potentially opening ourselves up to a slew of ICEs as
> the gimplifier as a whole I don't think has been audited to ensure that it
> handles error_mark_node is a sane fashion.
>
> So I'd tend to want to wait for the next stage1.

No problem, I will make sure to ping this patch then.