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

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

On Tue, Mar 6, 2018 at 4:41 AM, Jeff Law <law@redhat.com> wrote:
> On 03/05/2018 12:30 PM, Michael Matz wrote:
>> Hi,
>>
>> On Mon, 5 Mar 2018, Jeff Law wrote:
>>
>>>>> 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.
>>>>
>>>> Actually, without further conditions I don't see how it would be safe
>>>> for the successor to have multiple preds.  We might have this
>>>> situation:
>>>>
>>>> bb1: ret = setjmp
>>>> bb2: x0 = phi <x1 (bb1), foo(bbX)>
>>> No.  Can't happen -- we're still building the initial CFG.  There are no
>>> PHI nodes, there are no SSA_NAMEs.
>>
>> While that is currently true I think it's short-sighted.  Thinking about
>> the situation in terms of SSA names and PHI nodes clears up the mind.  In
>> addition there is already code which builds (sub-)cfgs when SSA form
>> exists (gimple_find_sub_bbs).  Currently that code can't ever generate
>> setjmp calls, so it's not an issue.
> It's not clearing up anything for me.  Clearly you're onto something
> that I'm missing, but still trying to figure out.
>
> Certainly we have to be careful WRT the implicit set of the return value
> of the setjmp call that occurs on the longjmp path.  That's worth
> investigating.  I suspect that works today more by accident of having an
> incorrect CFG than by design.
>
>
>>
>>> We have two choices we can either target the setjmp itself, which is
>>> what we've been doing and is inaccurate.  Or we can target the point
>>> immediately after the setjmp, which is accurate.
>>
>> Not precisely, because the setting of the return value of setjmp does
>> happen after both returns.  So moving the whole second-return edge target
>> to after the setjmp call (when it includes an LHS) is not correct
>> (irrespective how the situation in the successor BBs like like).
> But it does or at least it should.  It's implicitly set on the longjmp
> side.  If we get this wrong I'd expect we'll see uninit uses in the PHI.
>  That's easy enough to instrument and check for.
>
> This aspect of setjmp/longjmp is, in some ways, easier to see in RTL
> because the call returns its value in a hard reg which is implicitly set
> by the longjmp and we immediately copy it into a pseudo.   Which would
> magically DTRT if we had the longjmp edge target the point just after
> the setjmp in RTL.

While it's true that the hardreg is set by the callee the GIMPLE IL
indeed doesn't reflect this (and we have a similar issue with EH
where the exceptional return does _not_ include the assignment
to the LHS but the GIMPLE IL does...).

So with your patch we should see

 ret_1 = setjmp ();
   |                                 \
   |                              AB dispatcher
   |                                    /
   v                                   v
# ret_2 = PHI <ret_1, ret_1(ab)>
...

even w/o a PHI.  So I think we should be fine given we have that
edge from setjmp to the abnormal dispatcher.

Richard.

>
>
>
> Jeff