Re: longjmp clobber warnings are utterly broken in modern gcc

On Mon, Jan 26, 2015 at 1:34 PM, Andres Freund <andres@2ndquadrant.com> wrote:
> I guess we'd need to tie it into PG_exception_stack levels, so it
> correctly handles nesting with sigsetjmp locations. In contrast to
> sigsetjmp() style handlers we can't rely on PG_CATCH cleaning up that
> state.

I was thinking that PG_TRY() would need to squirrel away the value of
cleanup_stack and set it to null, and PG_CATCH would need to restore
the squirreled-away value.  That way we fire handlers in
reverse-order-of-registration regardless of which style of
registration is used.

> I wonder how hard it is to make that reliable for errors thrown in a
> cleanup callback. Those certainly are possible in some of the PG_CATCHes
> we have right now.

I think what should happen is that pg_re_throw() should remove each
frame from the stack and then call the associated callback.  If
another error occurs, we won't try that particular callback again, but
we'll try the next one.  In most cases this should be impossible
anyway because the catch-block is just a variable assignment or
something, but not in all cases, of course.

>> And before doing sigsetjmp to the active handler, we run all the
>> functions on the stack.  There shouldn't be any need for volatile; the
>> compiler has to know that once we make it possible to get at a pointer
>> to cb_arg via a global variable (cleanup_stack), any function we call
>> in another translation unit could decide to call that function and it
>> would need to see up-to-date values of everything cb_arg points to.
>> So before calling such a function it had better store that data to
>> memory, not just leave it lying around in a register somewhere.
>
> Given that we, at the moment at least, throw ERRORs from signal
> handlers, I don't think that necessarily holds true. I think we're not
> that far away from getting rid of all of those though.

Well, I think the theory behind that is we should only be throwing
errors from signal handlers when ImmediateInterruptOK = true, which is
only supposed to happen when the code thereby interrupted "isn't doing
anything interesting".  So if you set up some state that can be
touched by the error-handling path and then, in the same function, set
ImmediateInterruptOK, yeah, that probably needs to be volatile.  But
if function A sets up the state and then calls function B in another
translation unit, and B sets ImmediateInterruptOK, then A has no way
of knowing that B wasn't going to just throw a garden-variety error,
so it better have left things in a tidy state.  We still need to
scrutinize the actual functions that set ImmediateInterruptOK and, if
they are static, their callers, but that isn't too many places to look
at.  It's certainly (IMHO) a lot better than trying to stick in
volatile qualifiers every place we use a try-catch block, and if you
succeed in getting rid of ImmediateInterruptOK, then it goes away
altogether.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company