On Fri, Mar 2, 2018 at 11:18 PM, Jeff Law <l...@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
>
