Re: [PATCH] Call maybe_clean_or_redirect_eh_stmt in gimple_fold_call (PR tree-optimization/51481)

[Richard Guenther <richard dot guenther at gmail dot com>]

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