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

Jeff Law law@redhat.com
Wed Feb 28 17:35:00 GMT 2018


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.

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

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.



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

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



More information about the Gcc-patches mailing list