Is this non-volatile pointer access OK?

On 3 September 2012 08:10, Daniel Farina <daniel@heroku.com> wrote:

http://doxygen.postgresql.org/xlog_8c_source.html#l08197

On line 8197 of xlog.c:

08194 /* Get a local copy of the last safe checkpoint record. */
08195 SpinLockAcquire(&xlogctl->info_lck);
08196 lastCheckPointRecPtr = xlogctl->lastCheckPointRecPtr;
08197 memcpy(&lastCheckPoint, &XLogCtl->lastCheckPoint, sizeof(CheckPoint));
08198 SpinLockRelease(&xlogctl->info_lck);

Note the use of capital XLogCtl->lastCheckPoint, which is not the
volatile pointer.

That looks like a bug to me.

Come to think of it, the whole convention of using a lower-case
variant of the original pointer variable name seems like a foot-gun,
given the harmful and indeed very subtle consequences of making this
error.

I count 98 SpinLockAcquire() call sites (of which only a minority use
this convention, which is mostly within xlog.c, I think). Is it worth
instituting an alternative convention to make this kind of misuse more
obvious? This went unnoticed since February 2009.

--
Peter Geoghegan http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training and Services

Peter Geoghegan <peter@2ndquadrant.com> writes:

On 3 September 2012 08:10, Daniel Farina <daniel@heroku.com> wrote:

On line 8197 of xlog.c:

08194 /* Get a local copy of the last safe checkpoint record. */
08195 SpinLockAcquire(&xlogctl->info_lck);
08196 lastCheckPointRecPtr = xlogctl->lastCheckPointRecPtr;
08197 memcpy(&lastCheckPoint, &XLogCtl->lastCheckPoint, sizeof(CheckPoint));
08198 SpinLockRelease(&xlogctl->info_lck);

Note the use of capital XLogCtl->lastCheckPoint, which is not the
volatile pointer.

That looks like a bug to me.

The problem with s/XLogCtl/xlogctl/ there is that then the compiler
warns about passing a volatile pointer to memcpy. I seem to recall
we discussed this once before and decided to leave it alone.

I experimented just now with replacing the memcpy with struct
assignment, here and in the other place where xlog.c does this
(see attached patch). I don't get a complaint from my versions of
gcc, although it's not entirely clear why not, since I doubt the
assembly code for struct assignment is any more atomic than memcpy
would be. According to
http://archives.postgresql.org/pgsql-patches/2007-07/msg00025.php
g++ *does* complain about that.

Anyway, since we're already depending on struct assignment for
XLogRecPtr (in the back branches anyway), I don't see any very good
reason not to depend on it for struct CheckPoint as well, and so
propose that we apply the attached.

Come to think of it, the whole convention of using a lower-case
variant of the original pointer variable name seems like a foot-gun,
given the harmful and indeed very subtle consequences of making this
error.

Yes. The right way to fix this would be for the compiler to not ever
move assignments across a SpinLockAcquire or SpinLockRelease. Do you
have a bulletproof method for guaranteeing that?

regards, tom lane

On Mon, 2012-09-03 at 11:14 +0100, Peter Geoghegan wrote:

Come to think of it, the whole convention of using a lower-case
variant of the original pointer variable name seems like a foot-gun,
given the harmful and indeed very subtle consequences of making this
error.

With some inventive macro magic, you could probably make this safer.
I'm thinking something along the lines of replacing

SpinLockAcquire(&xlogctl->info_lck);

with

SpinLockAcquire(XLogCtl, info_lck);

which expands to

{
volatile typeof(XLogCtl) *XLogCtl_volatile = XLogCtl;
void *XLogCtl = NULL; // compiler error or crash at run time if used
OldSpinLockAcquire(XLogCtl_volatile->info_lock);
...

and then something corresponding for SpinLockRelease.

This will likely only work with modern compilers, but it could give you
some amount of static checking against this problem.