Message ID | 1451576417-8710-3-git-send-email-patrick@parcs.ath.cx |
---|---|
State | New |
Headers | show |
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.
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; }