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.
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.
>
> Now suppose 'bar' looks like
[ ... ]
It doesn't really matter what bar looks like.
>
>
> 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?)
>
> 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.
>
>
> So the bottom line here that setjmps in loops are extra tricky, warnings for
> them that superficially appear to be false positives may indicate a real
> issue,
> and we can do a better job documenting that and recommending safer patterns.
I don't think setjmps in loops add any real additional complexity.
>
>
> 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).
Jeff
