1139241 - Add Android/aarch64 build configure

Status: RESOLVED INCOMPLETE

(NSPR :: NSPR, defect)

Comment 1

[Makoto Kato [:m_kato]]

Attachment: Part 1. Don't define PR_ALTERNATE_INT64_TYPEDEF if Android 64bit

Comment 2

[Makoto Kato [:m_kato]]

Comment on attachment 8587152 [details] [diff] [review]
Part 1. Don't define PR_ALTERNATE_INT64_TYPEDEF if Android 64bit

In stdint.h in Android-21

...
#if __LP64__
typedef long __int64_t;
typedef unsigned long __uint64_t;
#else
typedef long long __int64_t;
typedef unsigned long long __uint64_t;
#endif
...

So we should not define PR_ALTERNATE_INT64_TYPEDEF on Android 64bit.

Comment 3

[Makoto Kato [:m_kato]]

Attachment: Part 2. Add Android aarch64 and x86_64 configure

Comment 4

[Wan-Teh Chang]

Comment on attachment 8587152 [details] [diff] [review]
Part 1. Don't define PR_ALTERNATE_INT64_TYPEDEF if Android 64bit

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

Makoto: thank you for the patch. I suggest an alternative fix.
Please try that. If that doesn't work, I will review+ this patch.

::: pr/include/prtypes.h
@@ +230,5 @@
>  ** C11. When most platforms ignore this C99 requirement, NSPR will be able
>  ** to use <stdint.h>. A patch to do that is in NSPR bug 634793.
>  */
>  
> +#if defined(__APPLE__) || (defined(__ANDROID__) && !defined(__LP64__)) || defined(__OpenBSD__)

Makoto: I believe we can simply remove "|| defined(__ANDROID__)". Please test that. Thanks!

Comment 5

[Wan-Teh Chang]

Comment on attachment 8587152 [details] [diff] [review]
Part 1. Don't define PR_ALTERNATE_INT64_TYPEDEF if Android 64bit

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

::: pr/include/prtypes.h
@@ +230,5 @@
>  ** C11. When most platforms ignore this C99 requirement, NSPR will be able
>  ** to use <stdint.h>. A patch to do that is in NSPR bug 634793.
>  */
>  
> +#if defined(__APPLE__) || (defined(__ANDROID__) && !defined(__LP64__)) || defined(__OpenBSD__)

I looked into why we added "|| defined(__ANDROID__)" before. It turns out that it was me who added it, but it's not clear why I thought it was necessary. The only documentation I can find is what I wrote at the end of bug 634793 comment 34:

  These findings showed PRInt64/PRUint64 are only different from
  int64_t/uint64_t on OpenBSD and Android. This explains why the
  sloppy code only broke OpenBSD among the "desktop" platforms, but
  I don't know why the sloppy code didn't seem to break Android.

Perhaps I was confused at that time, because I don't think we did 64-bit Android builds at that time (2012-12-06).

Also, if the "#if __LP64__" code you quoted in comment 2 was only added to the Android bionic library recently, we may need to do a bionic/NDK version check. In any case, I believe your proposed patch is equivalent to simply removing the "|| defined(__ANDROID__)" check. So please test that. Thank you!

Comment 6

[Wan-Teh Chang]

Comment on attachment 8587296 [details] [diff] [review]
Part 2. Add Android aarch64 and x86_64 configure

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

r=wtc. This seems correct. Please wait for Ted's review because
I am not very familiar with the Android build process.

::: configure.in
@@ +206,2 @@
>          esac
> +        case "$host_cpu" in

Should this be "build_cpu" instead of "host_cpu"?

build_cpu is the CPU of the build machine, right?

@@ +214,5 @@
> +                android_toolchain="$android_ndk"/toolchains/$target_name/prebuilt/$kernel_name-x86
> +            fi
> +            ;;
> +        *)
> +            AC_MSG_ERROR([No known toolchain for your host cpu])

If you change "host_cpu" to "build_cpu", please also change this error message to "build cpu".

Comment 7

[(not currently active) Ted Mielczarek]

Note that in Mozilla-land we have historically (since the introduction of autoconf) used "host" where autotools uses "build". cls admitted in a bug that that was his mistake, but we have an awful lot of usage of it and it would be painful to change at this point. NSPR's configure follows suit:
http://hg.mozilla.org/projects/nspr/file/6b30fde4148a/configure.in#l583

Comment 8

[Wan-Teh Chang]

Ted: your comment 7 seems to indicate your support of the Part 2 patch
for configure (attachment 8587296 [details] [diff] [review]). If so, could you please
check it in? Thanks.

Please do not check in the Part 1 patch (for prtypes.h).

Comment 9

[(not currently active) Ted Mielczarek]

Comment on attachment 8587296 [details] [diff] [review]
Part 2. Add Android aarch64 and x86_64 configure

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

r=me with the s/host/build/ thing wtc pointed out fixed.

::: configure.in
@@ +206,2 @@
>          esac
> +        case "$host_cpu" in

Yes. Confusingly the top-level Mozilla configure uses host/target, but NSPR switched to use the autoconf-standard build/host.

Comment 10

[Wan-Teh Chang]

Comment on attachment 8587296 [details] [diff] [review]
Part 2. Add Android aarch64 and x86_64 configure

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

::: configure.in
@@ +206,2 @@
>          esac
> +        case "$host_cpu" in

Ted Mielczarek wrote:
> Confusingly the top-level Mozilla configure uses host/target, but
> NSPR switched to use the autoconf-standard build/host.

Ted, are you sure NSPR switched? I didn't make that change when I
upgraded NSPR's configure.in from autoconf 2.13 to autoconf 2.50.

Comment 11

[(not currently active) Ted Mielczarek]

Actually now I'm not and it seems to confusingly use both in places, like:
https://dxr.mozilla.org/mozilla-central/source/nsprpub/configure.in#555

Comment 12

[(not currently active) Ted Mielczarek]

I suspect that that works in practice because the top-level configure is passing down --target, and $build is auto-detected with config.guess?

Comment 13

[(not currently active) Ted Mielczarek]

Comment on attachment 8587152 [details] [diff] [review]
Part 1. Don't define PR_ALTERNATE_INT64_TYPEDEF if Android 64bit

Clearing this because wtc already reviewed it.

Comment 14

[Chris Peterson [:cpeterson]]

Makoto, do we still need to land your NSPR patches here for Android/aarch64 builds?

Comment 15

[Makoto Kato [:m_kato]]

(In reply to Chris Peterson [:cpeterson] from comment #14)
> Makoto, do we still need to land your NSPR patches here for Android/aarch64
> builds?

Unnecessary for Gecko.  Current build system doesn't use NSPR's configure.  So I should close this.  If anyone needs configure support for NSPR, they can file a bug.