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?
Now suppose 'bar' looks like
void bar(jmp_buf jb)
{
static int cnt;
static jmp_buf stash;
if ((cnt ^= -1))
memcpy(stash, jb, sizeof stash);
else
longjmp(stash, 1);
}
So every other iteration of the loop, 'bar' will longjmp to the state
stashed on the previous iteration. Since 'n' is modified between iterations,
this is exactly the situation the warning aims to catch: if 'n' is restored
from jmp_buf, the last write to 'n' is lost.
Still, I can imagine one can argue that the warning in the PR is bogus, as
the loop assigns invariant values to problematic variables:
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).
(granted, if the loop is not entered, setjmp is not called either and cannot
possibly return normally, let alone abnormally, but our IR does not model that).
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.
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
BB2: BB99:
| ABNORMAL_DISPATCHER()
v v------/
BB3:
x_1 = phi(x_2(BB2), x_3(BB99)
ret_1 = setjmp()
to
BB2: BB99:
| ABNORMAL_DISPATCHER()
v |
BB3: |
ret_1 = setjmp() |
|\ v
| ---------------->BB4:
| ret_2 = __builtin_setjmp_receiver()
v v---------------/
BB4:
x_1 = phi(x_2(BB2), x_3(BB4)
ret_3 = phi(ret_1(BB2), ret_2(BB4)
so the phi node for 'x' simply goes to the new join BB, which also needs a
new phi for setjmp's return value 'ret'.
Plus, we'd need to somehow ensure that BB4 stays empty apart from the receiver,
and we have no infrastructure to ensure that.
So as far as I can tell it's not readily implementable and in fact brings no
benefit for setjmp and vfork. I can see a theoretical benefit for other
functions that have 'returns_twice' attribute and receive arguments: this
would eliminate bogus phi nodes for arguments. But for vfork and setjmp this
doesn't buy anything.
You even said yourself that phi nodes would still be there, just in a different
block:
> You might instead be able to look at the PHI nodes of the successor block
So in my opinion our CFG is good enough, the real issues with -Wclobbered false
positives are not due to phi nodes but other effects.
If you agree: what would be the next steps?
Alexander
