Re: longjmp clobber warnings are utterly broken in modern gcc - Mailing list pgsql-hackers

From Robert Haas
Subject Re: longjmp clobber warnings are utterly broken in modern gcc
Date
Msg-id CA+TgmoaV2Ew7xX88twdVVUMz4EQLdqTzm3-pZpuK=OB-UqRUgA@mail.gmail.com
Whole thread Raw
In response to Re: longjmp clobber warnings are utterly broken in modern gcc  (Andres Freund <andres@2ndquadrant.com>)
Responses Re: longjmp clobber warnings are utterly broken in modern gcc
List pgsql-hackers
> That's pretty similar to to PG_ENSURE_ERROR_CLEANUP, except that that is
> also called after FATAL errors. If we do this, we probably should try to
> come up with a easier to understand naming scheme. PG_TRY_WITH_CLEANUP
> vs. PG_ENSURE_ERROR_CLEANUP isn't very clear to a reader.

Yep.

>> Instead of doing anything with sigsetjmp(), this would just push a
>> frame onto a cleanup stack. We would call of those callbacks from
>> innermost to outermost before doing siglongjmp().  With this design,
>> we don't need any volatile-ization.
>
> On the other hand most of the callsites will need some extra state
> somewhere to keep track of what to undo. That's a bit of restructuring
> work too.  And if the cleanup function needs to reference anything done
> in the TRY block, that state will need to be volatile too.

I don't think so.

>> Aside from any reduction in the need
>> for volatile, this might actually perform slightly better, because
>> sigsetjmp() is a system call on some platforms.  There are probably
>> few cases where that actually matters, but the one in pq_getmessage(),
>> for example, might not be entirely discountable.
>
> Hm. How would you implement PG_TRY_WITH_CLEANUP without a sigsetjmp()?

Posit:

struct cleanup_entry {   void (*callback)(void *);   void *callback_arg;   struct cleanup_entry *next;
};
cleanup_entry *cleanup_stack = NULL;

So PG_TRY_WITH_CLEANUP(_cb, _cb_arg) does (approximately) this:

{
cleanup_entry e;
cleanup_entry *orige;
e.callback = (_cb);
e.callback_arg = (_cb_arg);
e.next = cleanup_stack;
orige = cleanup_stack;
cleanup_stack = &e;

And when you PG_END_TRY_WITH_CLEANUP, we just do this:

cleanup_stack = orige;
}

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.

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



pgsql-hackers by date:

Previous
From: Andrew Gierth
Date:
Subject: Abbreviated keys for Numeric (was: Re: B-Tree support function number 3 (strxfrm() optimization))
Next
From: Robert Haas
Date:
Subject: Re: Additional role attributes && superuser review