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

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

On Wed, Feb 28, 2018 at 6:35 PM, Jeff Law <law@redhat.com> wrote:
> On 02/28/2018 03:43 AM, Richard Biener wrote:
>> On Wed, Feb 28, 2018 at 1:16 AM, Jeff Law <law@redhat.com> wrote:
> [ ... snip ...]
>>>
>>> Anyway, I'd already been looking at 21161 and was aware that the CFG's
>>> we're building in presence of setjmp/longjmp were slightly inaccurate.
>>>
>>> In particular, a longjmp returns to the point immediately after the
>>> setjmp, not to the setjmp itself.  But our CFG building has the edge
>>> from the abnormal dispatcher going to the block containing the setjmp call.
>>
>> Yeah...  for SJLJ EH we get this right via __builtin_setjmp_receiver.
> Right.
>
> And as you noted in a follow-up we don't carry these abnormal edges from
> gimple to RTL.  As a result they are both inaccurate, but in subtly
> different ways.  I'd like to fix that wart, but probably not during this
> cycle.

Heh, keep pushing that back as well.  As you explained in the other mail
"perserving" the edges isn't so easy but that's still what we eventually
should do (because we can't really re-create them in the same optimistic
way we could at the beginning).  Maybe we can keep them and just
have a sweep "redirecting" those we kept to/from appropriate places.

> [ Another snip ]
>
>>> 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.
> The single successor test was strictly my paranoia WRT abnormal/EH edges.
>
> I don't immediately see why the CFG would be incorrect if the successor
> of the setjmp block has multiple preds.  But maybe it's something
> downstream that I'm not familiar with -- my worry all along with this
> work was the gimple->rtl conversion and block/edge discovery at that point.

Hmm, right, maybe extra paranoia from me here as well.  Note that I think
even with EH edges out of the setjmp the abnormal return wouldn't be
wrong to the successor given it isn't going to throw after the abnormal
return.

So I guess the proper action is to remove the successor check and
not replace it with a single-pedecessor-of-the-successor check.

> Verifying the successor has a single pred seems simple enough if done at
> the right time.  If it has multiple preds, then we can always fall back
> to the less accurate CFG that we've been building for a decade or more.
> The less precise CFG works, but the imprecision can impede more precise
> analysis as we've seen with this BZ.

As said I'd rather be consistent on what we do for this instead of doing
sth different depending on "context".  So let's drop those restrictions
on the surrounding CFG.

>
>
>> -               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
>>
>>   gimple *last = last_stmt (bb);
>>   if (last && ...)
>>
>> also handle all returns-twice calls this way, not only setjmp_call_p.
>>
>>>
> [ ... snip ... ]
>
>>> We can easily see that the call to __setjmp can never be reached given
>>> that we consider the longjmp call as non-returning.  So AFAICT
>>> everything is as should be expected.  I think the right thing is to just
>>> remove this compromised test.
>>
>> I agree.  Bonus points if you look at PR57147 and see if the testcase
>> was misreduced (maybe it was just for an ICE so we can keep it
>> and just remove the dump scanning?)
> I had actually looked at 57147.  The original test is pr57147-1 which we
> still handle correctly.  pr57147-2 doesn't represent any real world
> code.  You took pr57147-1.c and twiddled it slightly based on reviewing
> the CFG cleanup code and spotting a potential issue.

Ah, I see.

> We might be able to change pr57147-2 to scan for changes in the edges
> and their flags and given the simplicity of the test that may not be
> terribly fragile.  I wouldn't want to do that in a larger test though.
> Let me give that a whirl.
>
> jeff