963033 - AArch64 support update for NSPR

Status: RESOLVED FIXED

(NSPR :: NSPR, defect, P2)

Description

[Marcin Juszkiewicz]

Attachment: aarch64-nspr.patch

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:29.0) Gecko/20100101 Firefox/29.0 (Beta/Release)
Build ID: 20140122030521

Comment 1

[(not currently active) Ted Mielczarek]

Comment on attachment 8364281 [details] [diff] [review]
aarch64-nspr.patch

Review of attachment 8364281 [details] [diff] [review]:
-----------------------------------------------------------------

This looks fine. I'll land it for you in the NSPR repo shortly, we'll need to sync that to mozilla-central to get it in Firefox trunk.

Comment 2

[Wan-Teh Chang]

Comment on attachment 8364281 [details] [diff] [review]
aarch64-nspr.patch

Review of attachment 8364281 [details] [diff] [review]:
-----------------------------------------------------------------

::: mozilla-central.orig/nsprpub/pr/include/pratom.h
@@ +109,5 @@
>             defined(__ia64__) || defined(__x86_64__) || \
>             (defined(__powerpc__) && !defined(__powerpc64__)) || \
>             (defined(__arm__) && \
>             defined(__GCC_HAVE_SYNC_COMPARE_AND_SWAP_4)) || \
> +           defined(__alpha)) || defined(__aarch64__)))

Marcin: thank you for the patch.

This ifdef should match the corresponding ifdef in
nsprpub/pr/include/md/_linux.h. That ifdef looks like this:

  #if defined(__arm__) || defined(__aarch64__)
  #if defined(__GCC_HAVE_SYNC_COMPARE_AND_SWAP_4)
  /* Use GCC built-in functions */
  #define _PR_HAVE_ATOMIC_OPS
  #define _MD_INIT_ATOMIC()

  #define _MD_ATOMIC_INCREMENT(ptr) __sync_add_and_fetch(ptr, 1)
  #define _MD_ATOMIC_DECREMENT(ptr) __sync_sub_and_fetch(ptr, 1)
  #define _MD_ATOMIC_SET(ptr, nv) __sync_lock_test_and_set(ptr, nv)
  #define _MD_ATOMIC_ADD(ptr, i) __sync_add_and_fetch(ptr, i)

  #elif defined(_PR_ARM_KUSER)
  ...

To match that, we would need to use this ifdef here:

  ((defined(__arm__) ||  defined(__aarch64__)) && \
   defined(__GCC_HAVE_SYNC_COMPARE_AND_SWAP_4)) || \

Is __GCC_HAVE_SYNC_COMPARE_AND_SWAP_4 always defined for __aarch64__?

Comment 3

[Marcin Juszkiewicz]

According to toolchain guys I asked the answer is YES, __GCC_HAVE_SYNC_COMPARE_AND_SWAP_4 is always defined for __aarch64__.

Comment 4

[(not currently active) Ted Mielczarek]

http://hg.mozilla.org/projects/nspr/rev/d301edef8121

Comment 5

[Wan-Teh Chang]

Comment on attachment 8364281 [details] [diff] [review]
aarch64-nspr.patch

Review of attachment 8364281 [details] [diff] [review]:
-----------------------------------------------------------------

::: mozilla-central.orig/nsprpub/pr/include/pratom.h
@@ +109,5 @@
>             defined(__ia64__) || defined(__x86_64__) || \
>             (defined(__powerpc__) && !defined(__powerpc64__)) || \
>             (defined(__arm__) && \
>             defined(__GCC_HAVE_SYNC_COMPARE_AND_SWAP_4)) || \
> +           defined(__alpha)) || defined(__aarch64__)))

Marcin, Ted:

I think line 113:
           defined(__alpha)) || defined(__aarch64__)))
should read
           defined(__alpha) || defined(__aarch64__))))

I also suggest swapping alpha and aarch64 so that arm and
aarch64 are listed next to each other:
           defined(__aarch64__) || defined(__alpha))))

Comment 6

[Wan-Teh Chang]

Attachment: Fix parentheses.

Comment 7

[Marcin Juszkiewicz]

Comment on attachment 8383133 [details] [diff] [review]
Fix parentheses.

Yes, you are right. I went outside of __linux__ block with original submission.

Comment 8

[Wan-Teh Chang]

Comment on attachment 8383133 [details] [diff] [review]
Fix parentheses.

https://hg.mozilla.org/projects/nspr/rev/0ddb64117375