Martin Pitt <mpitt@debian.org> writes:
> The updated patch only uses the gcc builtins if there is no explicit
> implementation, but drops the arm one as this doesn't work on ARMv7
> and newer, as stated in the original mail.
Getting this thread back to the original patch ... I'm afraid that if we
apply this as-is, what will happen is that we fix ARMv7 and break older
versions. Some googling found this, for instance:
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=33413
which suggests that (1) ARM gcc hasn't had __sync_lock_test_and_set for
very long, and (2) what it generates doesn't work pre-ARMv6.
So I'm thinking that removing the swpb ASM option is not such a good
idea. We could possibly test for __sync_lock_test_and_set first, and
only use swpb if we're on ARM and don't have the builtin.
Another thing that is bothering me is that according to the gcc manual,
eg here,
http://gcc.gnu.org/onlinedocs/gcc-4.1.2/gcc/Atomic-Builtins.html
__sync_lock_test_and_set is nominally provided for datatypes 1, 2, 4,
or 8 bytes in length, but the underlying hardware doesn't necessarily
support all those widths natively. If you pick the wrong width then
you don't get an inline operation at all, but a call to some possibly
inefficient library subroutine. I see that your patch just assumes that
"int" will be a good width for the lock type, but it's unclear to me
what that choice is based on and whether or not it might be a really bad
choice on some platforms. A look through s_lock.h suggests that only a
minority of platforms prefer int-width locks ... but I have no idea
how many of those assembly snippets could have been coded to use a
different lock datatype without penalty. Some other evidence that
4-byte __sync_lock_test_and_set isn't universal is here:
https://svn.boost.org/trac/boost/ticket/2525
Google is also finding some rather worrisome suggestions that
__sync_lock_test_and_set might involve a kernel call on some flavors of
ARM. That would be pretty disastrous from a performance standpoint.
regards, tom lane