On Tue, Dec 13, 2011 at 1:00 PM, Jakub Jelinek <ja...@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 <ja...@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
