Fix bug with noreturn call

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.

Fixed by clearing the BB.  The patch also removes a superfluous call to 
maybe_clean_or_replace_eh_stmt (gsi_replace already calls it) and makes sure 
that all the flags are copied in gimple_call_copy_skip_args.

Tested on x86_64-suse-linux.  OK for mainline (and 4.5 branch)?


2010-06-21  Eric Botcazou  <ebotcazou@adacore.com>

	* cgraphunit.c (cgraph_redirect_edge_call_stmt_to_callee): Chain the
	new statement only if necessary and clear its BB.  Remove superfluous
	call to maybe_clean_or_replace_eh_stmt.
	* gimple.c (gimple_call_copy_skip_args): Use gimple_call_copy_flags to
	copy the flags.

> 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 (&copy_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 (&copy_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 (&copy_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
>