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

[Richard Guenther]

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