626309 - ARM should use built-in atomic function

Status: RESOLVED FIXED

(NSPR :: NSPR, defect, P2)

Description

[Makoto Kato [:m_kato]]

By bug 334826, NSPR has atomic function as inline version for Windows and some GCC platform.  But ARM (that ARMv6 or later has atomic) doesn't use, so we should use it even if ARM.

Also, built-in atomic version for ARM is from GCC 4.4.

Comment 1

[Makoto Kato [:m_kato]]

Attachment: fix

Comment 2

[Makoto Kato [:m_kato]]

Comment on attachment 506688 [details] [diff] [review]
fix

I tested on gcc-4.5.2-1ubuntu6.

Latest gcc 4.5 that included in Ubuntu 11.04 has built-in atomic function for ARM.  More detail is https://wiki.linaro.org/WorkingGroups/ToolChain/AtomicMemoryOperations

Comment 3

[Wan-Teh Chang]

Comment on attachment 506688 [details] [diff] [review]
fix

Makoto, thank you for the patch!

>+           (defined(__arm__) && \
>+           defined(__GCC_HAVE_SYNC_COMPARE_AND_SWAP_4)) || \

Do all ARM processors have atomic instructions?  If so, we
can omit the __GCC_HAVE_SYNC_COMPARE_AND_SWAP_4 test.

Comment 4

[Makoto Kato [:m_kato]]

(In reply to comment #3)
> Comment on attachment 506688 [details] [diff] [review]
> fix
> 
> Makoto, thank you for the patch!
> 
> >+           (defined(__arm__) && \
> >+           defined(__GCC_HAVE_SYNC_COMPARE_AND_SWAP_4)) || \
> 
> Do all ARM processors have atomic instructions?  If so, we
> can omit the __GCC_HAVE_SYNC_COMPARE_AND_SWAP_4 test.

No. this is from ARMv6. But __GCC_HAVE_SYNC_COMPARE_AND_SWAP_4 works even if ARMv4.

But I should add test on configure to use this.

Comment 5

[Makoto Kato [:m_kato]]

Oops, previous comment is invalid.  please ignore.

(In reply to comment #3)
> Comment on attachment 506688 [details] [diff] [review]
> fix
> 
> Makoto, thank you for the patch!
> 
> >+           (defined(__arm__) && \
> >+           defined(__GCC_HAVE_SYNC_COMPARE_AND_SWAP_4)) || \
> 
> Do all ARM processors have atomic instructions?  If so, we
> can omit the __GCC_HAVE_SYNC_COMPARE_AND_SWAP_4 test.

if __GCC_HAVE_SYNC_COMPARE_AND_SWAP_4 is defined, gcc generates atomic function by -mcpu/-march flags.  __GCC_HAVE_SYNC_COMPARE_AND_SWAP_4 is enough for cpu support.

Builtin atomic function uses "STREX" and "LDREX" that are supported from ARMv6 (such as -march=armv7-a).  If ARMv4 or ARMv5, it doesn't use *EX and uses gcc atomic code.

Comment 6

[Makoto Kato [:m_kato]]

Also , __GCC_HAVE_SYNC_COMPARE_AND_SWAP_4 is from new gcc.  __GCC_HAVE_SYNC_COMPARE_AND_SWAP_4 check needs like i386 target.

Comment 7

[Wan-Teh Chang]

Makoto: thank you for answering my question.

Your patch has only one subtle problem.  We need
to make sure the PR_AtomicXXX functions are interchangeable
with the PR_ATOMIC_XXX macros.  I described this problem in
this comment in pratom.h:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/nsprpub/pr/include/pratom.h&rev=3.18&mark=106-111#97

In _linux.h, we define _PR_HAVE_ATOMIC_OPS for ARM only if
_PR_ARM_KUSER is defined:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/nsprpub/pr/include/md/_linux.h&rev=3.56&mark=219-220#219

So we will need to add the following code to _linux.h:

#if defined(__arm__)
#if defined(__GCC_HAVE_SYNC_COMPARE_AND_SWAP_4)
#define _PR_HAVE_ATOMIC_OPS
#define _MD_INIT_ATOMIC()
#define _MD_ATOMIC_INCREMENT(ptr) xxxxx
#define _MD_ATOMIC_DECREMENT(ptr) xxxxx
#define _MD_ATOMIC_ADD(ptr, n) xxxxx
#define _MD_ATOMIC_SET(ptr, nv) xxxxx
#elif defined(_PR_ARM_KUSER)
... existing code based on __kernel_cmpxchg ...
#endif
#endif

Comment 8

[Makoto Kato [:m_kato]]

Attachment: fix v2

Comment 9

[Wan-Teh Chang]

Attachment: fix v2.1, by Makoto Kato

I fixed a nit in a comment and checked in the patch
on the NSPR trunk (NSPR 4.8.8).

Checking in pratom.h;
/cvsroot/mozilla/nsprpub/pr/include/pratom.h,v  <--  pratom.h
new revision: 3.19; previous revision: 3.18
done
Checking in md/_linux.h;
/cvsroot/mozilla/nsprpub/pr/include/md/_linux.h,v  <--  _linux.h
new revision: 3.58; previous revision: 3.57
done