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

On 19.12.2011 16:31, Robert Haas wrote:
> On Sun, Dec 18, 2011 at 5:42 PM, Martin Pitt<mpitt@debian.org>  wrote:
>>> It probably makes sense to use it on any platform where it's
>>> defined. Presumably an implementation provided by the compiler is
>>> always going to be at least as good as any magic assembler
>>> incantations we can come up with.
>>
>> I agree. How about a patch like this? It uses builtin atomics if
>> available, and falls back to the custom implementations if not.
>
> -1.  Absent some evidence that gcc's implementations are superior to
> ours, I think we should not change stuff that works now.  That's
> likely to lead to subtle bugs that are hard to find and perhaps
> dependent on the exact compiler version used.

Ok, we're in disagreement on that then. I don't feel very strongly about
it, let's see what others think.

One thing that caught my eye: if you use __sync_lock_and_test() to
implement S_LOCK(), you really should be using __sync_lock_release() for
S_UNLOCK().

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.

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