Re: [patch] Fix bug with noreturn call

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

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
>