Re: [PATCH] Use CC atomic builtins if available [was: Re: TAS patch for building on armel/armhf thumb]

On 19.12.2011 22:03, Noah Misch wrote:
> On Mon, Dec 19, 2011 at 05:09:11PM +0200, Heikki Linnakangas wrote:
>> Actually, I believe our Itanium (and possibly ARM, too) implementation
>> of S_UNLOCK() is wrong as it is. There is no platform-specific
>> S_UNLOCK() defined for Itanium, so we're using the generic
>> implementation:
>>
>> #if !defined(S_UNLOCK)
>> #define S_UNLOCK(lock)        (*((volatile slock_t *) (lock)) = 0)
>> #endif     /* S_UNLOCK */
>>
>> That is not sufficient on platforms with a weak memory model, like Itanium.
>
> Other processors may observe the lock as held after its release, but there's no
> correctness problem.

I thought it would also be legal for a store to become visible to other
processors, *after* the releasing of the lock. Which would be bad. For
example, if you have:

volatile bool *shared = ...
SpinLockAcquire(lock);
shared->variable = true;
SpinLockRelease(lock);
more code

The macro-expanded code would look like:

<test and set> lock
shared->variable = true;
(*((volatile slock_t *) (lock)) = 0;
more code

I believe on an architecture with a weak memory model, like Itanium,
there's no guarantee that the assignments will happen in that order. The
lock might appear as released *before* the variable is set.

--
   Heikki Linnakangas
   EnterpriseDB   http://www.enterprisedb.com