Patchwork Call maybe_clean_or_redirect_eh_stmt in gimple_fold_call (PR tree-optimization/51481)

login
register
mail settings
Submitter Jakub Jelinek
Date Dec. 12, 2011, 5:01 p.m.
Message ID <20111212170149.GS1957@tyan-ft48-01.lab.bos.redhat.com>
Download mbox | patch
Permalink /patch/130799/
State New
Headers show

Comments

Jakub Jelinek - Dec. 12, 2011, 5:01 p.m.
Hi!

In gimple_fold_call (called from fold_stmt from replace_uses_by from cfg
cleanup) we weren't calling maybe_clean_or_replace_eh_stmt, so when
we've replaced a printf call (which can throw) with puts (which can throw
too), nothing would update EH stmts.  It would be problematic calling
gimple_purge_dead_eh_edges, because callers might be surprised by that,
especially when this happens during cfg cleanup, so instead I just assert
it is not needed and don't try to fold if a throwing stmt would be replaced
by non-throwing.  FAB pass can handle that instead.  No folding
has been actually disabled because of that check during bootstrap/regtest,
so it is there just in case.

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

2011-12-12  Jakub Jelinek  <jakub@redhat.com>

	PR tree-optimization/51481
	* gimple-fold.c (gimple_fold_call): Call
	maybe_clean_or_replace_eh_stmt.  Avoid optimization if stmt has EH
	edges, but gimple_fold_builtin result can't throw.

	* gcc.dg/pr51481.c: New test.


	Jakub
Richard Henderson - Dec. 12, 2011, 8:47 p.m.
On 12/12/2011 09:01 AM, Jakub Jelinek wrote:
> 2011-12-12  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR tree-optimization/51481
> 	* gimple-fold.c (gimple_fold_call): Call
> 	maybe_clean_or_replace_eh_stmt.  Avoid optimization if stmt has EH
> 	edges, but gimple_fold_builtin result can't throw.
> 
> 	* gcc.dg/pr51481.c: New test.

Ok.


r~
Richard Guenther - Dec. 13, 2011, 10:52 a.m.
On Mon, Dec 12, 2011 at 6:01 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> Hi!
>
> In gimple_fold_call (called from fold_stmt from replace_uses_by from cfg
> cleanup) we weren't calling maybe_clean_or_replace_eh_stmt, so when
> we've replaced a printf call (which can throw) with puts (which can throw
> too), nothing would update EH stmts.  It would be problematic calling
> gimple_purge_dead_eh_edges, because callers might be surprised by that,
> especially when this happens during cfg cleanup, so instead I just assert
> it is not needed and don't try to fold if a throwing stmt would be replaced
> by non-throwing.  FAB pass can handle that instead.  No folding
> has been actually disabled because of that check during bootstrap/regtest,
> so it is there just in case.

I think that all callers of fold_stmt are supposed to handle EH
updating themselves, similar to how they are supposed to
call update_stmt.  I see that replace_uses_by does call
maybe_clean_or_replace_eh_stmt - are you sure it is this path
the issue triggers on?

Richard.

> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
>
> 2011-12-12  Jakub Jelinek  <jakub@redhat.com>
>
>        PR tree-optimization/51481
>        * gimple-fold.c (gimple_fold_call): Call
>        maybe_clean_or_replace_eh_stmt.  Avoid optimization if stmt has EH
>        edges, but gimple_fold_builtin result can't throw.
>
>        * gcc.dg/pr51481.c: New test.
>
> --- gcc/gimple-fold.c.jj        2011-12-11 22:02:37.000000000 +0100
> +++ gcc/gimple-fold.c   2011-12-12 11:42:34.740168390 +0100
> @@ -1117,10 +1117,21 @@ gimple_fold_call (gimple_stmt_iterator *
>   if (callee && DECL_BUILT_IN (callee))
>     {
>       tree result = gimple_fold_builtin (stmt);
> -      if (result)
> +      if (result
> +         /* Disallow EH edge removal here.  We can't call
> +            gimple_purge_dead_eh_edges here.  */
> +         && (lookup_stmt_eh_lp (stmt) == 0
> +             || tree_could_throw_p (result)))
>        {
>           if (!update_call_from_tree (gsi, result))
>            gimplify_and_update_call_from_tree (gsi, result);
> +         if (!gsi_end_p (*gsi))
> +           {
> +             gimple new_stmt = gsi_stmt (*gsi);
> +             bool update_eh ATTRIBUTE_UNUSED
> +               = maybe_clean_or_replace_eh_stmt (stmt, new_stmt);
> +             gcc_assert (!update_eh);
> +           }
>          changed = true;
>        }
>     }
> --- gcc/testsuite/gcc.dg/pr51481.c.jj   2011-12-12 11:18:27.304678207 +0100
> +++ gcc/testsuite/gcc.dg/pr51481.c      2011-12-12 11:18:02.000000000 +0100
> @@ -0,0 +1,33 @@
> +/* PR tree-optimization/51481 */
> +/* { dg-do compile } */
> +/* { dg-options "-O -fexceptions -fipa-cp -fipa-cp-clone" } */
> +
> +extern const unsigned short int **foo (void)
> +  __attribute__ ((__nothrow__, __const__));
> +struct S { unsigned short s1; int s2; };
> +extern struct S *s[26];
> +
> +void
> +bar (int x, struct S *y, ...)
> +{
> +  static struct S *t;
> +  __builtin_va_list ap;
> +  __builtin_va_start (ap, y);
> +  if (t != s[7])
> +    {
> +      const char *p = "aAbBc";
> +      t = s[7];
> +      while ((*foo ())[(unsigned char) *p])
> +       p++;
> +    }
> +  __builtin_printf (x == 0 ? "abc\n" : "def\n");
> +  if (y != 0)
> +    __builtin_printf ("ghi %d %d", y->s2, y->s1);
> +  __builtin_va_end (ap);
> +}
> +
> +void
> +baz (char *x)
> +{
> +  bar (1, 0, x);
> +}
>
>        Jakub
Jakub Jelinek - Dec. 13, 2011, noon
On Tue, Dec 13, 2011 at 11:52:38AM +0100, Richard Guenther wrote:
> On Mon, Dec 12, 2011 at 6:01 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> > In gimple_fold_call (called from fold_stmt from replace_uses_by from cfg
> > cleanup) we weren't calling maybe_clean_or_replace_eh_stmt, so when
> > we've replaced a printf call (which can throw) with puts (which can throw
> > too), nothing would update EH stmts.  It would be problematic calling
> > gimple_purge_dead_eh_edges, because callers might be surprised by that,
> > especially when this happens during cfg cleanup, so instead I just assert
> > it is not needed and don't try to fold if a throwing stmt would be replaced
> > by non-throwing.  FAB pass can handle that instead.  No folding
> > has been actually disabled because of that check during bootstrap/regtest,
> > so it is there just in case.

Note, I've already committed the patch yesterday.

> I think that all callers of fold_stmt are supposed to handle EH
> updating themselves, similar to how they are supposed to
> call update_stmt.  I see that replace_uses_by does call

Some do something, but others don't.  Do you think it is preferable when
the callers do that (all of them)?  Even if that is chosen, while some could
purge dead eh edges, other places can't do that IMHO, so either we need to
simply not do transformations that remove EH edges in fold_stmt, or have an
argument that is passed down whether it is ok to do so.

> maybe_clean_or_replace_eh_stmt - are you sure it is this path
> the issue triggers on?

Yes, I'm sure this was in replace_uses_by.  Apparently it calls
maybe_clean_or_replace_eh_stmt, but incorrectly (similarly forwprop in some
cases doesn't call it at all, in other places incorrectly):

  maybe_clean_or_replace_eh_stmt (stmt, stmt);

This does nothing.  It must be called with the old stmt and new stmt it was
replaced with, otherwise, when it is called just with new stmt as in this
place lookup_stmt_eh_lp will just return 0 (unless fold_stmt did call it already
properly).

	Jakub
Richard Guenther - Dec. 13, 2011, 12:05 p.m.
On Tue, Dec 13, 2011 at 1:00 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Tue, Dec 13, 2011 at 11:52:38AM +0100, Richard Guenther wrote:
>> On Mon, Dec 12, 2011 at 6:01 PM, Jakub Jelinek <jakub@redhat.com> wrote:
>> > In gimple_fold_call (called from fold_stmt from replace_uses_by from cfg
>> > cleanup) we weren't calling maybe_clean_or_replace_eh_stmt, so when
>> > we've replaced a printf call (which can throw) with puts (which can throw
>> > too), nothing would update EH stmts.  It would be problematic calling
>> > gimple_purge_dead_eh_edges, because callers might be surprised by that,
>> > especially when this happens during cfg cleanup, so instead I just assert
>> > it is not needed and don't try to fold if a throwing stmt would be replaced
>> > by non-throwing.  FAB pass can handle that instead.  No folding
>> > has been actually disabled because of that check during bootstrap/regtest,
>> > so it is there just in case.
>
> Note, I've already committed the patch yesterday.
>
>> I think that all callers of fold_stmt are supposed to handle EH
>> updating themselves, similar to how they are supposed to
>> call update_stmt.  I see that replace_uses_by does call
>
> Some do something, but others don't.  Do you think it is preferable when
> the callers do that (all of them)?  Even if that is chosen, while some could
> purge dead eh edges, other places can't do that IMHO, so either we need to
> simply not do transformations that remove EH edges in fold_stmt, or have an
> argument that is passed down whether it is ok to do so.
>
>> maybe_clean_or_replace_eh_stmt - are you sure it is this path
>> the issue triggers on?
>
> Yes, I'm sure this was in replace_uses_by.  Apparently it calls
> maybe_clean_or_replace_eh_stmt, but incorrectly (similarly forwprop in some
> cases doesn't call it at all, in other places incorrectly):
>
>  maybe_clean_or_replace_eh_stmt (stmt, stmt);
>
> This does nothing.  It must be called with the old stmt and new stmt it was
> replaced with, otherwise, when it is called just with new stmt as in this
> place lookup_stmt_eh_lp will just return 0 (unless fold_stmt did call it already
> properly).

Yeah, I'm testing a followup patch that fixes the call in replace_uses_by
(that alone fixes the testcase in the PR).

Richard.

>        Jakub
Richard Guenther - Dec. 13, 2011, 12:10 p.m.
On Tue, Dec 13, 2011 at 1:00 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Tue, Dec 13, 2011 at 11:52:38AM +0100, Richard Guenther wrote:
>> On Mon, Dec 12, 2011 at 6:01 PM, Jakub Jelinek <jakub@redhat.com> wrote:
>> > In gimple_fold_call (called from fold_stmt from replace_uses_by from cfg
>> > cleanup) we weren't calling maybe_clean_or_replace_eh_stmt, so when
>> > we've replaced a printf call (which can throw) with puts (which can throw
>> > too), nothing would update EH stmts.  It would be problematic calling
>> > gimple_purge_dead_eh_edges, because callers might be surprised by that,
>> > especially when this happens during cfg cleanup, so instead I just assert
>> > it is not needed and don't try to fold if a throwing stmt would be replaced
>> > by non-throwing.  FAB pass can handle that instead.  No folding
>> > has been actually disabled because of that check during bootstrap/regtest,
>> > so it is there just in case.
>
> Note, I've already committed the patch yesterday.

Yes, I've seen that.  I think it's ugly and inconsistent - we can
certaily fold non-call stmts from throwing to non-throwing.
Think of -fnon-call-exceptions and x + 1.3 and folding after
propagating 1.0 to x.

It is not fold_stmts business to care about EH info or edges
(it works on statements in isolation).

>> I think that all callers of fold_stmt are supposed to handle EH
>> updating themselves, similar to how they are supposed to
>> call update_stmt.  I see that replace_uses_by does call
>
> Some do something, but others don't.  Do you think it is preferable when
> the callers do that (all of them)?  Even if that is chosen, while some could
> purge dead eh edges, other places can't do that IMHO, so either we need to
> simply not do transformations that remove EH edges in fold_stmt, or have an
> argument that is passed down whether it is ok to do so.

I think all callers can (should be able to) do that.  In the light of
-fnon-call-exceptions we probably miss quite a few - but we should
fix them.

[Unfortunately some of our propagation helpers do not, or do not have
a good way of returning whether EH/CFG cleanup is necessary,
notably gsi_remove, which removes the stmt from the EH tables
but does not purge edges.]

Richard.
Jakub Jelinek - Dec. 13, 2011, 12:15 p.m.
On Tue, Dec 13, 2011 at 01:05:30PM +0100, Richard Guenther wrote:
> Yeah, I'm testing a followup patch that fixes the call in replace_uses_by
> (that alone fixes the testcase in the PR).

That patch looks good, but then forwprop has the same
static void
tidy_after_forward_propagate_addr (gimple stmt)
{
  /* We may have turned a trapping insn into a non-trapping insn.  */
  if (maybe_clean_or_replace_eh_stmt (stmt, stmt)
      && gimple_purge_dead_eh_edges (gimple_bb (stmt)))
    cfg_changed = true;
...

Other spots are maybe ok, so if you want, the
maybe_cleanup_or_replace_eh_stmt call from gimple_fold_call can be nuked
afterwards.  But what about that lookup_*/tree_could_throw_p check?
Should it stay?

	Jakub
Richard Guenther - Dec. 13, 2011, 12:17 p.m.
On Tue, Dec 13, 2011 at 1:15 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Tue, Dec 13, 2011 at 01:05:30PM +0100, Richard Guenther wrote:
>> Yeah, I'm testing a followup patch that fixes the call in replace_uses_by
>> (that alone fixes the testcase in the PR).
>
> That patch looks good, but then forwprop has the same
> static void
> tidy_after_forward_propagate_addr (gimple stmt)
> {
>  /* We may have turned a trapping insn into a non-trapping insn.  */
>  if (maybe_clean_or_replace_eh_stmt (stmt, stmt)
>      && gimple_purge_dead_eh_edges (gimple_bb (stmt)))
>    cfg_changed = true;
> ...
>
> Other spots are maybe ok, so if you want, the
> maybe_cleanup_or_replace_eh_stmt call from gimple_fold_call can be nuked
> afterwards.  But what about that lookup_*/tree_could_throw_p check?
> Should it stay?

I don't think so, if we want to prevent fold_stmt from running into such
situation we should do it centrally (similar to the in-place variant).  But
I wouldn't go down this route without further evidence (read: a testcase
that we cannot fix otherwise).

Richard.

>        Jakub

Patch

--- gcc/gimple-fold.c.jj	2011-12-11 22:02:37.000000000 +0100
+++ gcc/gimple-fold.c	2011-12-12 11:42:34.740168390 +0100
@@ -1117,10 +1117,21 @@  gimple_fold_call (gimple_stmt_iterator *
   if (callee && DECL_BUILT_IN (callee))
     {
       tree result = gimple_fold_builtin (stmt);
-      if (result)
+      if (result
+	  /* Disallow EH edge removal here.  We can't call
+	     gimple_purge_dead_eh_edges here.  */
+	  && (lookup_stmt_eh_lp (stmt) == 0
+	      || tree_could_throw_p (result)))
 	{
           if (!update_call_from_tree (gsi, result))
 	    gimplify_and_update_call_from_tree (gsi, result);
+	  if (!gsi_end_p (*gsi))
+	    {
+	      gimple new_stmt = gsi_stmt (*gsi);
+	      bool update_eh ATTRIBUTE_UNUSED
+		= maybe_clean_or_replace_eh_stmt (stmt, new_stmt);
+	      gcc_assert (!update_eh);
+	    }
 	  changed = true;
 	}
     }
--- gcc/testsuite/gcc.dg/pr51481.c.jj	2011-12-12 11:18:27.304678207 +0100
+++ gcc/testsuite/gcc.dg/pr51481.c	2011-12-12 11:18:02.000000000 +0100
@@ -0,0 +1,33 @@ 
+/* PR tree-optimization/51481 */
+/* { dg-do compile } */
+/* { dg-options "-O -fexceptions -fipa-cp -fipa-cp-clone" } */
+
+extern const unsigned short int **foo (void)
+  __attribute__ ((__nothrow__, __const__));
+struct S { unsigned short s1; int s2; };
+extern struct S *s[26];
+
+void
+bar (int x, struct S *y, ...)
+{
+  static struct S *t;
+  __builtin_va_list ap;
+  __builtin_va_start (ap, y);
+  if (t != s[7])
+    {
+      const char *p = "aAbBc";
+      t = s[7];
+      while ((*foo ())[(unsigned char) *p])
+	p++;
+    }
+  __builtin_printf (x == 0 ? "abc\n" : "def\n");
+  if (y != 0)
+    __builtin_printf ("ghi %d %d", y->s2, y->s1);
+  __builtin_va_end (ap);
+}
+
+void
+baz (char *x)
+{
+  bar (1, 0, x);
+}