On Wed, 2020-01-29 at 18:32 +0300, Alexander Monakov wrote:
> 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.
We have traditionally only bothered to analyze variables that are live at the
setjmp point. Objects which are not live at the setjmp point need not be
analyzed. That's all I was saying.
> >
> > > 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.
In general we're not going to necessarily have visibility into calls where the
longjmp may occur. Any analysis we do has to not depend on looking inside the
called routine.
>
> > > 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).
Who's triage are you referring to? :-)
It's been a couple years since I dug into 61118, but my recollection was that
the
core problem was the multiple assignments to the pseudo registers holding
cancel_arg and cancel_routine. If there was a single assignment to those
pseudos
then the existing RTL Wclobbered analysis would not issue a warning.
While the assignments use the same source, the mere existence of multiple
assignments causes real headaches for the existing -Wclobbered analysis. The
multiple assignments occur due to the actions of CCP1 and FRE1 propagating into
a
PHI node, which they arguably shouldn't do for an abnormal PHI.
>
> > > 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.
Sorry, I don't follow your statement. From the fragment above var1 is assigned
only once and inside the loop. I don't see where it would be initialized to
zero.
>
> > > 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.
True, but we have significant problems with the accuracy of the GIMPLE CFG as
well WRT setjmp/longjmp. I strongly suspect that without fixing that problem
you're going to be fighting a losing battle with a reimplementation on gimple
much like we're fighting a losing battle with the RTL implementation.
>
> 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.
They're both inaccurate :(
>
> 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
Agreed that this is the best proposal to-date to fix CFG inaccuracies which I
see
as a requirement for fixing Wclobbered. Maybe I'm too biased by looking at the
RTL version...
>
> 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.
It aids us because we don't end up with a (bogus) loop containing the setjmp.
Jeff
