Message ID | 201006211803.49205.ebotcazou@adacore.com |
---|---|
State | New |
Headers | show |
> Hi, > > with our 4.5-based compiler, ACATS cxh1001 ICEs at -O2 -flto because of a > dangling GIMPLE_CALL statement on the MODIFIED_NORETURN_CALLS list: the > statement is replaced in cgraph_redirect_edge_call_stmt_to_callee but it > nevertheless remains on the list. Moveover the new statement doesn't inherit > the noreturn property. As you know, the noreturn discovery made several problems of this class to appear. There is one open problem WRT virutal operands I plan to look into today or tomorrow. The problems should be reproducible on branch when indirect call gets folded to direct call to noreturn functions. Overall I think the whole concept of saving statement pointers into on-side array, not clearing them and expecting statements to be sane is very fragile. There are many reasons why the statement can be removed, cgraph_redirect_edge_call_stmt_to_callee is just one of them. Taking this approach we would have to clear BB pointer everywhere (that can probably be done within gsi_replace) Perhaps we can however think of better ways to track the changes? We already track statement changes for SSA updating, this seems similar especially now when virtually all optimizations are done at SSA. > @@ -2345,7 +2345,6 @@ cgraph_redirect_edge_call_stmt_to_callee > { > tree decl = gimple_call_fndecl (e->call_stmt); > gimple new_stmt; > - gimple_stmt_iterator gsi; > #ifdef ENABLE_CHECKING > struct cgraph_node *node; > #endif > @@ -2383,13 +2382,15 @@ cgraph_redirect_edge_call_stmt_to_callee > SSA_NAME_DEF_STMT (gimple_vdef (new_stmt)) = new_stmt; > gimple_call_set_fndecl (new_stmt, e->callee->decl); > > - gsi = gsi_for_stmt (e->call_stmt); > - gsi_replace (&gsi, new_stmt, true); > + if (new_stmt != e->call_stmt) > + { > + gimple_stmt_iterator gsi = gsi_for_stmt (e->call_stmt); > + gsi_replace (&gsi, new_stmt, true); > + /* Clear its BB in case it is on the MODIFIED_NORETURN_CALLS list. */ > + gimple_set_bb (e->call_stmt, NULL); > + } > update_stmt (new_stmt); > > - /* Update EH information too, just in case. */ > - maybe_clean_or_replace_eh_stmt (e->call_stmt, new_stmt); > - This is fine with me for both mainline and branch without the BB clear and if we don't find better way to track changes, with the BB clear too. Honza > cgraph_set_call_stmt_including_clones (e->caller, e->call_stmt, new_stmt); > > if (cgraph_dump_file) > Index: gimple.c > =================================================================== > --- gimple.c (revision 161008) > +++ gimple.c (working copy) > @@ -3085,14 +3085,8 @@ gimple_call_copy_skip_args (gimple stmt, > gimple_set_block (new_stmt, gimple_block (stmt)); > if (gimple_has_location (stmt)) > gimple_set_location (new_stmt, gimple_location (stmt)); > - > - /* Carry all the flags to the new GIMPLE_CALL. */ > + gimple_call_copy_flags (new_stmt, stmt); > gimple_call_set_chain (new_stmt, gimple_call_chain (stmt)); > - gimple_call_set_tail (new_stmt, gimple_call_tail_p (stmt)); > - gimple_call_set_cannot_inline (new_stmt, gimple_call_cannot_inline_p (stmt)); > - gimple_call_set_return_slot_opt (new_stmt, gimple_call_return_slot_opt_p (stmt)); > - gimple_call_set_from_thunk (new_stmt, gimple_call_from_thunk_p (stmt)); > - gimple_call_set_va_arg_pack (new_stmt, gimple_call_va_arg_pack_p (stmt)); > > gimple_set_modified (new_stmt, true); >
> Overall I think the whole concept of saving statement pointers into on-side > array, not clearing them and expecting statements to be sane is very > fragile. There are many reasons why the statement can be removed, > cgraph_redirect_edge_call_stmt_to_callee is just one of them. Taking this > approach we would have to clear BB pointer everywhere (that can probably be > done within gsi_replace). I didn't invent it, copy_bb does exactly the same thing: /* Copy all GIMPLE_CALL flags, location and block, except GF_CALL_VA_ARG_PACK. */ gimple_call_copy_flags (new_call, stmt); gimple_call_set_va_arg_pack (new_call, false); gimple_set_location (new_call, gimple_location (stmt)); gimple_set_block (new_call, gimple_block (stmt)); gimple_call_set_lhs (new_call, gimple_call_lhs (stmt)); gsi_replace (©_gsi, new_call, false); gimple_set_bb (stmt, NULL); stmt = new_call; without even explaining why. :-) > This is fine with me for both mainline and branch without the BB clear and > if we don't find better way to track changes, with the BB clear too. Thanks, but I don't plan to work on the better way in the near future and I don't think it would be appropriate for the branch anyway.
> > Overall I think the whole concept of saving statement pointers into on-side > > array, not clearing them and expecting statements to be sane is very > > fragile. There are many reasons why the statement can be removed, > > cgraph_redirect_edge_call_stmt_to_callee is just one of them. Taking this > > approach we would have to clear BB pointer everywhere (that can probably be > > done within gsi_replace). > > I didn't invent it, copy_bb does exactly the same thing: > > /* Copy all GIMPLE_CALL flags, location and block, except > GF_CALL_VA_ARG_PACK. */ > gimple_call_copy_flags (new_call, stmt); > gimple_call_set_va_arg_pack (new_call, false); > gimple_set_location (new_call, gimple_location (stmt)); > gimple_set_block (new_call, gimple_block (stmt)); > gimple_call_set_lhs (new_call, gimple_call_lhs (stmt)); > > gsi_replace (©_gsi, new_call, false); > gimple_set_bb (stmt, NULL); > stmt = new_call; > > without even explaining why. :-) Yep, I noticed too. gsi_remove is setting BB to NULL and BB remove use it, so I guess gsi_replace is one of relatively few ways the statement can get out of instruction stream without its BB pointer being cleared. It might be better to just make gsi_replace to clear BB and test that it works than visiting all the 30 uses of gsi_replace in compiler to verify that they can't leak the noreturn call? Honza > > > This is fine with me for both mainline and branch without the BB clear and > > if we don't find better way to track changes, with the BB clear too. > > Thanks, but I don't plan to work on the better way in the near future and I > don't think it would be appropriate for the branch anyway. > > -- > Eric Botcazou
On Mon, Jun 21, 2010 at 6:40 PM, Jan Hubicka <hubicka@ucw.cz> wrote: >> > Overall I think the whole concept of saving statement pointers into on-side >> > array, not clearing them and expecting statements to be sane is very >> > fragile. There are many reasons why the statement can be removed, >> > cgraph_redirect_edge_call_stmt_to_callee is just one of them. Taking this >> > approach we would have to clear BB pointer everywhere (that can probably be >> > done within gsi_replace). >> >> I didn't invent it, copy_bb does exactly the same thing: >> >> /* Copy all GIMPLE_CALL flags, location and block, except >> GF_CALL_VA_ARG_PACK. */ >> gimple_call_copy_flags (new_call, stmt); >> gimple_call_set_va_arg_pack (new_call, false); >> gimple_set_location (new_call, gimple_location (stmt)); >> gimple_set_block (new_call, gimple_block (stmt)); >> gimple_call_set_lhs (new_call, gimple_call_lhs (stmt)); >> >> gsi_replace (©_gsi, new_call, false); >> gimple_set_bb (stmt, NULL); >> stmt = new_call; >> >> without even explaining why. :-) > > Yep, I noticed too. gsi_remove is setting BB to NULL and BB remove use it, > so I guess gsi_replace is one of relatively few ways the statement can get out of > instruction stream without its BB pointer being cleared. > > It might be better to just make gsi_replace to clear BB and test that it works > than visiting all the 30 uses of gsi_replace in compiler to verify that they can't > leak the noreturn call? Indeed gsi_replace should simply clear the BB of the old stmt. A patch to do so is pre-approved if it passes bootstrap & regtest. Thanks, Richard. > Honza >> >> > This is fine with me for both mainline and branch without the BB clear and >> > if we don't find better way to track changes, with the BB clear too. >> >> Thanks, but I don't plan to work on the better way in the near future and I >> don't think it would be appropriate for the branch anyway. >> >> -- >> Eric Botcazou >
Index: cgraphunit.c =================================================================== --- cgraphunit.c (revision 161008) +++ cgraphunit.c (working copy) @@ -2345,7 +2345,6 @@ cgraph_redirect_edge_call_stmt_to_callee { tree decl = gimple_call_fndecl (e->call_stmt); gimple new_stmt; - gimple_stmt_iterator gsi; #ifdef ENABLE_CHECKING struct cgraph_node *node; #endif @@ -2383,13 +2382,15 @@ cgraph_redirect_edge_call_stmt_to_callee SSA_NAME_DEF_STMT (gimple_vdef (new_stmt)) = new_stmt; gimple_call_set_fndecl (new_stmt, e->callee->decl); - gsi = gsi_for_stmt (e->call_stmt); - gsi_replace (&gsi, new_stmt, true); + if (new_stmt != e->call_stmt) + { + gimple_stmt_iterator gsi = gsi_for_stmt (e->call_stmt); + gsi_replace (&gsi, new_stmt, true); + /* Clear its BB in case it is on the MODIFIED_NORETURN_CALLS list. */ + gimple_set_bb (e->call_stmt, NULL); + } update_stmt (new_stmt); - /* Update EH information too, just in case. */ - maybe_clean_or_replace_eh_stmt (e->call_stmt, new_stmt); - cgraph_set_call_stmt_including_clones (e->caller, e->call_stmt, new_stmt); if (cgraph_dump_file) Index: gimple.c =================================================================== --- gimple.c (revision 161008) +++ gimple.c (working copy) @@ -3085,14 +3085,8 @@ gimple_call_copy_skip_args (gimple stmt, gimple_set_block (new_stmt, gimple_block (stmt)); if (gimple_has_location (stmt)) gimple_set_location (new_stmt, gimple_location (stmt)); - - /* Carry all the flags to the new GIMPLE_CALL. */ + gimple_call_copy_flags (new_stmt, stmt); gimple_call_set_chain (new_stmt, gimple_call_chain (stmt)); - gimple_call_set_tail (new_stmt, gimple_call_tail_p (stmt)); - gimple_call_set_cannot_inline (new_stmt, gimple_call_cannot_inline_p (stmt)); - gimple_call_set_return_slot_opt (new_stmt, gimple_call_return_slot_opt_p (stmt)); - gimple_call_set_from_thunk (new_stmt, gimple_call_from_thunk_p (stmt)); - gimple_call_set_va_arg_pack (new_stmt, gimple_call_va_arg_pack_p (stmt)); gimple_set_modified (new_stmt, true);