On 03/06/2018 01:57 AM, Richard Biener wrote: > On Tue, Mar 6, 2018 at 4:41 AM, Jeff Law <l...@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