Re: [RFA][PATCH][PR middle-end/61118] Improve tree CFG accuracy for setjmp/longjmp

[Richard Biener <richard.guenther@gmail.com>]

On Fri, Mar 2, 2018 at 11:18 PM, Jeff Law <law@redhat.com> wrote:
> On 02/28/2018 03:43 AM, Richard Biener wrote:
> [ More snipping ]
>
>>
>>> It's actually pretty easy to fix the CFG.  We  just need to recognize
>>> that a "returns twice" function returns not to the call, but to the
>>> point immediately after the call.  So if we have a call to a returns
>>> twice function that ends a block with a single successor, when we wire
>>> up the abnormal dispatcher, we target the single successor rather than
>>> the block containing the returns-twice call.
>>
>> Hmm, I think you need to check whether the successor has a single
>> predecessor, not whether we have a single successor (we always have
>> that unless setjmp also throws).  If you fix that you keep the CFG
>> "incorrect" if there are multiple predecessors so I think in addition
>> to properly creating the edges you have to work on the BB building
>> part to ensure that there's a single-predecessor block after
>> returns-twice function calls.  Note that currently we force returns-twice
>> to be the first (and only) stmt of a block -- your fix would relax this,
>> returns-twice no longer needs to start a new BB.
> So I found the code which makes the setjmp start a new block. But I
> haven't found the code which makes setjmp end a block.  I'm going to
> have to throw things into the debugger  to find the latter.

stmt_starts_bb_p

>
> We ought to remove the code that makes the setjmp start a new block.
> That's just unnecessary.   setjmp certainly needs to end the block though.

yes, after your change, of course.  The code in stmt_starts_bb_p
uses ECF_RETURNS_TWICE, so ...

>
>
>
>>
>> -               handle_abnormal_edges (dispatcher_bbs, bb, bb_to_omp_idx,
>> -                                      &ab_edge_call, false);
>> +               {
>> +                 bool target_after_setjmp = false;
>> +
>> +                 /* If the returns twice statement looks like a setjmp
>> +                    call at the end of a block with a single successor
>> +                    then we want the edge from the dispatcher to target
>> +                    that single successor.  That more accurately reflects
>> +                    actual control flow.  The more accurate CFG also
>> +                    results in fewer false positive warnings.  */
>> +                 if (gsi_stmt (gsi_last_nondebug_bb (bb)) == call_stmt
>> +                     && gimple_call_fndecl (call_stmt)
>> +                     && setjmp_call_p (gimple_call_fndecl (call_stmt))
>> +                     && single_succ_p (bb))
>> +                   target_after_setjmp = true;
>> +                 handle_abnormal_edges (dispatcher_bbs, bb, bb_to_omp_idx,
>> +                                        &ab_edge_call, false,
>> +                                        target_after_setjmp);
>> +               }
>>
>> I don't exactly get the hops you jump through here -- I think it's
>> better to split the returns-twice (always last stmt of a block after
>> the fixing) and the setjmp-receiver (always first stmt of a block) cases.
>> So, remove the handling of returns-twice from the above case and
>> handle returns-twice via
> Just wanted to verify the setjmp was the last statement in the block and
> the block passed control to a single successor.  If the setjmp is not
> the last statement, then having the longjmp pass control to the
> successor block potentially skips over statements between the setjmp and
> the end of the block.  That obviously would be bad.
>
> As I mentioned before the single_succ_p test was just my paranoia.
>
> Note that GSI can point to a setjmp receiver at this point.  We don't
> want to treat that like a setjmp.

True.

>
>>
>>   gimple *last = last_stmt (bb);
>>   if (last && ...)
>>
>> also handle all returns-twice calls this way, not only setjmp_call_p.
> Note that setjmp_call_p returns true for any returns-twice function.  So
> we are handling those.

... that's intended as well I think.

>
> So I think the open issue with this patch is removal of making the
> setjmp start a block and verification that we always have it end the
> block.  The latter should allow some simplifications to the code I added
> in make_edges and provide a level of consistency that is desirable.

We've abstracted that bit into GF_CALL_CTRL_ALTERING which we
compute during CFG build and only ever clear afterwards (so an
indirect call to setjmp via a type not having returns_twice will not
end up ending a BB and will not have abnormal edges associated).

So I don't think anything besides fixing CFG build is necessary.
Well - the whole RTL transition business of course.

Richard.

> Jeff
>