I said:
> Now that I think a little more, I am worried about the idea that the
> same thing could potentially happen in other modules that access shared
> memory and use spinlocks to serialize the access. Perhaps the fix I
> applied was wrong, and the correct fix is to change S_LOCK from
> #define S_UNLOCK(lock) (*(lock) = 0)
> to
> #define S_UNLOCK(lock) (*((volatile slock_t *) (lock)) = 0)
I have applied this patch also, since on reflection it seems the clearly
Right Thing. However, I find that AIX 5's compiler must have the LWLock
pointers marked volatile as well, else it still generates bad code.
Original assembly code fragment (approximately lines 244-271 of
lwlock.c):
l r3,8(r25)stb r24,44(r25)cmpi 0,r0,0stb r4,45(r25)st r23,48(r25)cal r5,0(r0)stx r23,r28,r27
<----- spinlock releasebc BO_IF_NOT,CR0_EQ,__L834st r25,12(r26) <----- store into
lock->headst r25,16(r26) <----- store into lock->taill r4,12(r25)bl .IpcSemaphoreLock{PR}
With "volatile" added in S_UNLOCK:
stb r24,44(r25)stb r3,45(r25)cmpi 0,r0,0cal r5,0(r0)st r23,48(r25)bc BO_IF_NOT,CR0_EQ,__L81cst
r25,12(r26) <----- store into lock->headstx r23,r28,r27 <----- spinlock releasel
r3,8(r25)st r25,16(r26) <----- store into lock->taill r4,12(r25)bl .IpcSemaphoreLock{PR}
With "volatile" lock pointer in LWLockAcquire:
stb r25,44(r23)stb r3,45(r23)cmpi 0,r0,0cal r5,0(r0)st r24,48(r23)bc BO_IF_NOT,CR0_EQ,__L850st
r23,12(r26) <----- store into lock->headst r23,16(r26) <----- store into lock->tailstx
r24,r28,r27 <----- spinlock releasel r3,8(r23)l r4,12(r23)bl .IpcSemaphoreLock{PR}
I believe the second of these cases is inarguably a compiler bug.
It is moving a store (into lock->tail) across a store through a
volatile-qualified pointer. As I read the spec, that's not kosher.
regards, tom lane