On Tue, 28 Jan 2020, Jeff Law wrote:
> On Tue, 2019-10-08 at 18:04 +0300, Alexander Monakov wrote:
> > On Thu, 3 Oct 2019, Jeff Law wrote:
> >
> > > You may want to review the 2018 discussion:
> > >
> > > https://www.mail-archive.com/gcc-patches@gcc.gnu.org/msg185287.html
> > >
> > > THe 2018 discussion was primarily concerned with fixing the CFG
> > > inaccuracies which would in turn fix 61118. But would not fix 21161.
> >
> > Well, PR 61118 was incorrectly triaged to begin with.
> >
> > Let me show starting from a simplified testcase:
> >
> > void bar(jmp_buf);
> > void foo(int n)
> > {
> > jmp_buf jb;
> > for (; n; n--)
> > if (setjmp(jb))
> > bar(jb);
> > }
> >
> > Here we warn that "parameter 'n' might be clobbered..." despite that
> > "obviously" there is no way 'n' might be modified between setjmp and
> > bar... right?
> Not necessarily. You have to look at what objects are live. N is
> clearly going to be live throughout the entire loop and thus it will be
> subject to setjmp/longjmp warnings.
That N is live is not enough: we want to warn if N is also modified
before an abnormal return (otherwise we'd attempt to warn for live
readonly variables). But if you're agreeing with me that warning here
is *not a false positive*, then I'm happy.
> The inaccuracies in our CFG at the setjmp call cause objects which are
> not actually live across the call to appear to be live. See PR 21161.
For the example shown in comment #14, agreed.
> > Now suppose 'bar' looks like
> [ ... ]
> It doesn't really matter what bar looks like.
Well it did for the purpose of demonstration, but if you're in agreement
anyway then we can say it's moot.
> > Still, I can imagine one can argue that the warning in the PR is bogus, as
> > the loop assigns invariant values to problematic variables:
> I'm not sure which PR you're referring to (21161, 65041 or some other?)
I'm arguing that PR 61118 was incorrectly triaged (first sentence of my email).
> > void foo(struct ctx *arg)
> > {
> > while (arg->cond) {
> > void *var1 = &arg->field;
> > void (*var2)(void*) = globalfn;
> > jmp_buf jb;
> > if (!setjmp(jb))
> > var2(var1);
> > }
> > etc(arg);
> > }
> >
> > and since "obviously" &arg->field is invariant, there's no way setjmp will
> > overwrite 'var1' with a different value, right?.. Except:
> >
> > If the while loop is not entered, a longjmp from inside 'etc' would restore
> > default (uninitialized) values; note the compiler doesn't pay attention to
> > the
> > lifetime of jmp_buf, it simply assumes 'etc' may cause setjmp to return
> > abnormally (this is correct in case of other returns_twice functions such as
> > vfork).
> Whether or not we should warn for restoring an uninitialized state I
> think is somethign we haven't really considered. Depending on how many
> assignments to the pseudo there are and the exact control flow we may
> or may not warn.
That restored values are uninitialized is not important; the problem remains
if var1 is declared before the 'while' and initialized to 0.
> > Now regarding claims that we emit "incorrect" CFG that is causing extra phi
> > nodes, and that "fixing CFG" would magically eliminate those and help with
> > false positives.
> >
> > While in principle it is perhaps nicer to have IR that more closely models
> > the abnormal flow, I don't see how it would reduce phi nodes; we'd go from
> It certainly does move PHI nodes and it can reduce the number of false
> positives. It's not as effective as it could be because of lameness on
> the RTL side (again see PR21161).
There's some mixup here because overall this was discussing the complete
reimplementation of the warning on GIMPLE, so RTL should not be relevant.
Currently there's no concrete proposal on the table for making GIMPLE CFG
somehow more accurate for setjmps. I can see that RTL CFG could be more
accurate indeed.
The best proposal for GIMPLE so far is moving the outgoing edge from the
abnormal dispatcher to just after the setjmp:
BB1
r1 = setjmp(env);
BB2
| r2 = ABNORMAL_DISPATCHER()
| +-------------------
V V
BB3
r3 = phi(r1, r2)
if (r3) goto BB4 else goto BB5
where for setjmp in particular the BB1->BB2 edge is not necessary, but
should be there for unknown returns_twice functions.
But I can't see how this proposal substantially aids analysis on GIMPLE.
In particular, we cannot easily make some kind of threading and separate
BB1-BB3-BB5 and BB2-BB3-BB4 paths because then we wouldn't be able to
easily lower it to RTL.
Alexander
