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

[Jeff Law <law at redhat dot com>]

On 03/06/2018 01:57 AM, Richard Biener wrote:
> 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.
I believe so by nature that the setjmp dominates the longjmp sites and
thus also dominates the dispatcher.  But it's something I want to
explicitly check before resubmitting.

jeff