Re: longjmp clobber warnings are utterly broken in modern gcc

Martijn van Oosterhout <kleptog(at)svana(dot)org> writes:
> Never mind, it doesn't work. It's not that GCC doesn't know setjmp() is
> special, it does (the returns_twice attribute). So GCC does the above
> effectivly itself. The problem is that local variables may be stored
> in memory over calls in the PG_TRY() block, volatile is a sledgehammer
> way of preventing that.

> The problem is, GCC doesn't know anything about what the return value
> of setjmp() means which means that it can never produce any sensible
> warnings in this area.

Yeah. There are actually two distinct issues here:

1. If local variables are kept in registers, longjmp will cause their
values to revert back to whatever they were at setjmp. Forcing them
into memory prevents that, ensuring that the CATCH block can see any
value changes made inside the TRY block.

2. The compiler might make optimizations based on the assumption that
control cannot flow from (any function call in!) the TRY block to the
CATCH block. It might well decide for example that a store to some
variable is dead and remove it.

"volatile" fixes both of these issues but as you say it's a pretty
sledgehammer way of doing it. In any case, the practical problem is
that we don't get any warnings reminding us that there's a hazard.

I've been wondering if we could improve the situation by changing the TRY
macro expansion. Instead of a straight

if (setjmp(...) == 0)
{
TRY code;
}
else
{
CATCH code;
}

we could do something like

volatile int _setjmpresult = setjmp(...);
if (_setjmpresult == 0)
{
TRY code;
}
if (_setjmpresult != 0)
{
CATCH code;
}

The "volatile" marker would prevent gcc from deducing that the CATCH
code cannot be reached after running the TRY code. Unfortunately,
that only partially solves the incorrect-optimization problem.
Given code like, say,

PG_TRY();
{
ptr = palloc...;
... stuff ...
pfree(ptr);
ptr = NULL;

... more stuff ...

ptr = palloc...;
... still more stuff ...
pfree(ptr);
ptr = NULL;

... yet more stuff ...
}
PG_CATCH();
{
if (ptr)
pfree(ptr);
}

the compiler could still decide that the first "ptr = NULL;" is a dead
store. I don't currently see any way around that without a volatile
marker on "ptr"; but maybe somebody can improve on this idea?

regards, tom lane