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

[Jeff Law <law@redhat.com>]

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.


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.




> 
> -               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.


> 
>   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.


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.

Jeff