Closed Bug 626309 Opened 9 years ago Closed 9 years ago

ARM should use built-in atomic function

Categories

(NSPR :: NSPR, defect, P2)

ARM
Linux
defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: m_kato, Assigned: m_kato)

Details

Attachments

(1 file, 2 obsolete files)

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.
Attached patch fix (obsolete) — Splinter Review
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
Attachment #506688 - Flags: review?(wtc)
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.
(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.
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.
Also , __GCC_HAVE_SYNC_COMPARE_AND_SWAP_4 is from new gcc.  __GCC_HAVE_SYNC_COMPARE_AND_SWAP_4 check needs like i386 target.
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
Attachment #506688 - Flags: review?(wtc) → review-
Attached patch fix v2 (obsolete) — Splinter Review
Attachment #506688 - Attachment is obsolete: true
Attachment #507804 - Flags: review?(wtc)
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
Attachment #507804 - Attachment is obsolete: true
Attachment #507804 - Flags: review?(wtc)
Status: NEW → RESOLVED
Closed: 9 years ago
Priority: -- → P2
Resolution: --- → FIXED
Target Milestone: --- → 4.8.8
You need to log in before you can comment on or make changes to this bug.