542113 - Add support for building NSPR on Android

Status: RESOLVED FIXED

(NSPR :: NSPR, enhancement)

Comment 1

[Michael Wu [:mwu]]

Attachment: Add support for building NSPR on Android, v1

This patch contains all the NSPR specific changes currently at http://hg.mozilla.org/users/vladimir_mozilla.com/mozilla-droid for building on Android (Bionic). Most of these are vlad's changes, though Ben Combee and I have made some small changes to configure.in.

Comment 2

[Ben Combee (:bcombee)]

Attachment: Additional patch to enable tests building

Comment 3

[Michael Wu [:mwu]]

Attachment: Add support for building NSPR on Android, v2

Roll up bcombee's changes.

Comment 4

[Michael Wu [:mwu]]

Attachment: Add support for building NSPR on Android, v3

Updates configure based on new NDK and changes in JS android patch.

Comment 5

[Michael Wu [:mwu]]

Attachment: Add support for building NSPR on Android, v3.1

Forgot to change a message in configure.in

Comment 6

[Michael Wu [:mwu]]

Attachment: Add support for building NSPR on Android, v3.2

Minor update to remove -std=g++0x

Comment 7

[(not currently active) Ted Mielczarek]

Comment on attachment 433165 [details] [diff] [review]
Add support for building NSPR on Android, v3.2

>diff --git a/nsprpub/configure b/nsprpub/configure
>diff --git a/nsprpub/configure.in b/nsprpub/configure.in
>--- a/nsprpub/configure.in
>+++ b/nsprpub/configure.in
>@@ -128,6 +128,73 @@
> fi
> 
> dnl ========================================================
>+dnl = Android uses a very custom (hacky) toolchain; we need to do this
>+dnl = here, so that the compiler checks can succeed
>+dnl ========================================================
>+
>+AC_ARG_WITH(android-ndk,
>+[  --with-android-ndk=DIR
>+                      location where the Android NDK can be found],
>+    android_ndk=$withval)
>+
>+AC_ARG_WITH(android-toolchain,
>+[  --with-android-toolchain=DIR
>+                            location of the android toolchain, default NDK/build/prebuilt/HOST/arm-eabi-4.4.0],

This help text lies (using HOST) given your XXX comment below.

>+if test "$target" = "arm-android-eabi" ; then
>+    if test -z "$android_ndk" ; then
>+       AC_MSG_ERROR([You must specify --with-android-ndk=/path/to/ndk when targeting Android.])
>+    fi
>+
>+    if test -z "$android_toolchain" ; then
>+       dnl XXX don't hardcode linux-x86 as the host; we could easily support MacOS X here
>+       android_toolchain="$android_ndk"/build/prebuilt/linux-x86/arm-eabi-4.4.0

If you're going to leave TODOs, please file an actual bug and put the bug number in the comment.

>+
>+    CPPFLAGS="-I$android_platform/usr/include -DANDROID $CPPFLAGS"
>+    CFLAGS="-mandroid -I$android_platform/usr/include -msoft-float -fno-short-enums -fno-exceptions -march=armv5te -mthumb-interwork $CFLAGS"
>+    CXXFLAGS="-mandroid -I$android_platform/usr/include -msoft-float -fpic -fno-short-enums -fno-exceptions -march=armv5te -mthumb-interwork -mthumb $CXXFLAGS"
>+    LDFLAGS="-mandroid -L$android_platform/usr/lib -Wl,-rpath-link=$android_platform/usr/lib --sysroot=$android_platform $LDFLAGS"
>+
>+    dnl prevent cross compile section from using these flags as host flags
>+    if test -z "$HOST_CPPFLAGS" ; then
>+        HOST_CPPFLAGS=" "
>+    fi

We should probably file a bug about making our HOST_ flags less stupid so you don't have to do this.

>+    ANDROID=1
>+    AC_SUBST(ANDROID)

Put this AC_SUBST down with the rest of them near the end of the file. (I think if you leave it in here, the @ANDROID@ value might not actually get substituted when not targeting Android, which is not what you want.) Also, I don't think we want this anyway, since no other platform has its own Makefile var like this.

>@@ -1411,6 +1478,33 @@
> 	esac
>     ;;
> 
>+arm-android-eabi)
>+    if test -z "$USE_NSPR_THREADS"; then
>+        USE_PTHREADS=1
>+        IMPL_STRATEGY=_PTH
>+    fi
>+    AC_DEFINE(XP_UNIX)
>+    AC_DEFINE(_GNU_SOURCE)
>+    AC_DEFINE(HAVE_FCNTL_FILE_LOCKING)
>+    AC_DEFINE(LINUX)
>+    CFLAGS="$CFLAGS -Wall"
>+    CXXFLAGS="$CXXFLAGS -Wall"
>+    MDCPUCFG_H=_linux.cfg
>+    PR_MD_CSRCS=linux.c

Seems weird to duplicate so much of the Linux block here, but I don't have a better suggestion.

>+    MKSHLIB='$(CC) $(DSO_LDOPTS) -o $@'
>+    DSO_CFLAGS=-fPIC
>+    DSO_LDOPTS='-shared -Wl,-soname -Wl,$(notdir $@)'
>+    _OPTIMIZE_FLAGS=-O2
>+    _DEBUG_FLAGS="-g -fno-inline"  # most people on linux use gcc/gdb, and that
>+                                   # combo is not yet good at debugging inlined
>+                                   # functions (even when using DWARF2 as the
>+                                   # debugging format)
>+    COMPILER_TAG=_glibc
>+    CPU_ARCH=$OS_TEST
>+    CPU_ARCH_TAG=_${CPU_ARCH}

I think you should just hardcode 'arm' in these two values to make it clearer.

>diff --git a/nsprpub/pr/include/md/_linux.h b/nsprpub/pr/include/md/_linux.h
>--- a/nsprpub/pr/include/md/_linux.h
>+++ b/nsprpub/pr/include/md/_linux.h
>@@ -284,6 +284,12 @@
> #define _PR_HAVE_GETHOST_R_INT
> #endif
> 
>+#ifdef ANDROID

Does the NDK compiler define ANDROID? I didn't see that you AC_DEFINEd it anywhere.

>+#undef _PR_HAVE_SYSV_SEMAPHORES
>+#undef PR_HAVE_SYSV_NAMED_SHARED_MEMORY

I think it'd be cleaner to wrap #ifndef ANDROID around the block that actually defines these. (Is it the glibc block above?)

>+#undef _PR_HAVE_GETPROTO_R

Where does this actually get defined? I don't see it anywhere but in configure (and not in a Linux block).

>diff --git a/nsprpub/pr/include/md/_pth.h b/nsprpub/pr/include/md/_pth.h
>--- a/nsprpub/pr/include/md/_pth.h
>+++ b/nsprpub/pr/include/md/_pth.h
>@@ -98,8 +98,13 @@
> #else
> #define _PT_PTHREAD_MUTEX_IS_LOCKED(m)    (EBUSY == pthread_mutex_trylock(&(m)))
> #endif
>+#if defined(ANDROID)
>+#define _PT_PTHREAD_CONDATTR_INIT(x)      0
>+#define _PT_PTHREAD_CONDATTR_DESTROY(x)   /* */
>+#else
> #define _PT_PTHREAD_CONDATTR_INIT         pthread_condattr_init
> #define _PT_PTHREAD_CONDATTR_DESTROY      pthread_condattr_destroy
>+#endif

Would be nice to add a comment here specifying why this isn't supported.

>diff --git a/nsprpub/pr/src/linking/prlink.c b/nsprpub/pr/src/linking/prlink.c
>--- a/nsprpub/pr/src/linking/prlink.c
>+++ b/nsprpub/pr/src/linking/prlink.c
>@@ -205,7 +205,7 @@
> 
> #elif defined(XP_UNIX)
> #ifdef HAVE_DLL
>-#ifdef USE_DLFCN
>+#if defined(USE_DLFCN) && !defined(ANDROID)

I think you should wrap the HAVE_DLL / USE_DLFCN in md/_linux.h in #ifndef ANDROID instead of doing this. (I take it there's no dlopen equivalent for Android?)

>diff --git a/nsprpub/pr/src/malloc/prmem.c b/nsprpub/pr/src/malloc/prmem.c
>--- a/nsprpub/pr/src/malloc/prmem.c
>+++ b/nsprpub/pr/src/malloc/prmem.c
>@@ -117,7 +117,7 @@
> 
> #ifdef HAVE_DLL
> 
>-#ifdef USE_DLFCN
>+#if defined(USE_DLFCN) && !defined(ANDROID)

Previous comment would fix this too...

>diff --git a/nsprpub/pr/src/misc/prnetdb.c b/nsprpub/pr/src/misc/prnetdb.c
>--- a/nsprpub/pr/src/misc/prnetdb.c
>+++ b/nsprpub/pr/src/misc/prnetdb.c
>@@ -1185,6 +1185,16 @@
>  * any usable implementation.
>  */
> 
>+#if defined(ANDROID)
>+/* Android's Bionic libc system includes prototypes for these in netdb.h,
>+ * but doesn't actually include implementations.  It uses the 5-arg form,
>+ * so these functions end up not matching the prototype.  So just rename
>+ * them if not found.
>+ */
>+#define getprotobyname_r _pr_getprotobyname_r
>+#define getprotobynumber_r _pr_getprotobynumber_r
>+#endif

This is gross, but I don't know a better way to work around it given what you describe here.

>diff --git a/nsprpub/pr/tests/Makefile.in b/nsprpub/pr/tests/Makefile.in
>--- a/nsprpub/pr/tests/Makefile.in
>+++ b/nsprpub/pr/tests/Makefile.in
>@@ -450,6 +450,11 @@
> endif
> endif
> 
>+ifeq ($(ANDROID),1)
>+LDOPTS=$(OS_LDFLAGS)
>+LIBPTHREAD=
>+XCFLAGS=${OS_CFLAGS}
>+endif

If this is the only place you use this ANDROID makefile var, then just drop that completely and use ifeq ($(OS_TARGET),Android) instead.

r- for some cleanup, but it looks pretty good.

Comment 8

[Michael Wu [:mwu]]

(In reply to comment #7)
> >+if test "$target" = "arm-android-eabi" ; then
> >+    if test -z "$android_ndk" ; then
> >+       AC_MSG_ERROR([You must specify --with-android-ndk=/path/to/ndk when targeting Android.])
> >+    fi
> >+
> >+    if test -z "$android_toolchain" ; then
> >+       dnl XXX don't hardcode linux-x86 as the host; we could easily support MacOS X here
> >+       android_toolchain="$android_ndk"/build/prebuilt/linux-x86/arm-eabi-4.4.0
> 
> If you're going to leave TODOs, please file an actual bug and put the bug
> number in the comment.
> 
err.. I fixed this already.. keep forgetting to put it in.

> >+
> >+    CPPFLAGS="-I$android_platform/usr/include -DANDROID $CPPFLAGS"
> >+    CFLAGS="-mandroid -I$android_platform/usr/include -msoft-float -fno-short-enums -fno-exceptions -march=armv5te -mthumb-interwork $CFLAGS"
> >+    CXXFLAGS="-mandroid -I$android_platform/usr/include -msoft-float -fpic -fno-short-enums -fno-exceptions -march=armv5te -mthumb-interwork -mthumb $CXXFLAGS"
> >+    LDFLAGS="-mandroid -L$android_platform/usr/lib -Wl,-rpath-link=$android_platform/usr/lib --sysroot=$android_platform $LDFLAGS"
> >+
> >+    dnl prevent cross compile section from using these flags as host flags
> >+    if test -z "$HOST_CPPFLAGS" ; then
> >+        HOST_CPPFLAGS=" "
> >+    fi
> 
> We should probably file a bug about making our HOST_ flags less stupid so you
> don't have to do this.
> 
Will do.

> >+    ANDROID=1
> >+    AC_SUBST(ANDROID)
> 
> Put this AC_SUBST down with the rest of them near the end of the file. (I think
> if you leave it in here, the @ANDROID@ value might not actually get substituted
> when not targeting Android, which is not what you want.) Also, I don't think we
> want this anyway, since no other platform has its own Makefile var like this.
> 
Ok.

> >+    MKSHLIB='$(CC) $(DSO_LDOPTS) -o $@'
> >+    DSO_CFLAGS=-fPIC
> >+    DSO_LDOPTS='-shared -Wl,-soname -Wl,$(notdir $@)'
> >+    _OPTIMIZE_FLAGS=-O2
> >+    _DEBUG_FLAGS="-g -fno-inline"  # most people on linux use gcc/gdb, and that
> >+                                   # combo is not yet good at debugging inlined
> >+                                   # functions (even when using DWARF2 as the
> >+                                   # debugging format)
> >+    COMPILER_TAG=_glibc
> >+    CPU_ARCH=$OS_TEST
> >+    CPU_ARCH_TAG=_${CPU_ARCH}
> 
> I think you should just hardcode 'arm' in these two values to make it clearer.
> 
Ok.

> >diff --git a/nsprpub/pr/include/md/_linux.h b/nsprpub/pr/include/md/_linux.h
> >--- a/nsprpub/pr/include/md/_linux.h
> >+++ b/nsprpub/pr/include/md/_linux.h
> >@@ -284,6 +284,12 @@
> > #define _PR_HAVE_GETHOST_R_INT
> > #endif
> > 
> >+#ifdef ANDROID
> 
> Does the NDK compiler define ANDROID? I didn't see that you AC_DEFINEd it
> anywhere.
> 
-DANDROID is in CPPFLAGS. I'll look for a more standard way of putting it in.

> >+#undef _PR_HAVE_SYSV_SEMAPHORES
> >+#undef PR_HAVE_SYSV_NAMED_SHARED_MEMORY
> 
> I think it'd be cleaner to wrap #ifndef ANDROID around the block that actually
> defines these. (Is it the glibc block above?)
> 
Ok.

> >+#undef _PR_HAVE_GETPROTO_R
> 
> Where does this actually get defined? I don't see it anywhere but in configure
> (and not in a Linux block).
> 
It's defined in a C file. nsprpub/pr/src/misc/prnetdb.c. I'll move this special casing over to there.

> >diff --git a/nsprpub/pr/include/md/_pth.h b/nsprpub/pr/include/md/_pth.h
> >--- a/nsprpub/pr/include/md/_pth.h
> >+++ b/nsprpub/pr/include/md/_pth.h
> >@@ -98,8 +98,13 @@
> > #else
> > #define _PT_PTHREAD_MUTEX_IS_LOCKED(m)    (EBUSY == pthread_mutex_trylock(&(m)))
> > #endif
> >+#if defined(ANDROID)
> >+#define _PT_PTHREAD_CONDATTR_INIT(x)      0
> >+#define _PT_PTHREAD_CONDATTR_DESTROY(x)   /* */
> >+#else
> > #define _PT_PTHREAD_CONDATTR_INIT         pthread_condattr_init
> > #define _PT_PTHREAD_CONDATTR_DESTROY      pthread_condattr_destroy
> >+#endif
> 
> Would be nice to add a comment here specifying why this isn't supported.
> 
I'm guessing bionic doesn't implement it. Will add a comment.

> >diff --git a/nsprpub/pr/src/linking/prlink.c b/nsprpub/pr/src/linking/prlink.c
> >--- a/nsprpub/pr/src/linking/prlink.c
> >+++ b/nsprpub/pr/src/linking/prlink.c
> >@@ -205,7 +205,7 @@
> > 
> > #elif defined(XP_UNIX)
> > #ifdef HAVE_DLL
> >-#ifdef USE_DLFCN
> >+#if defined(USE_DLFCN) && !defined(ANDROID)
> 
> I think you should wrap the HAVE_DLL / USE_DLFCN in md/_linux.h in #ifndef
> ANDROID instead of doing this. (I take it there's no dlopen equivalent for
> Android?)
> 
dlopen exists. It's just that (according to vlad's check in comment..) dlopen(NULL) isn't supported on android. 

> >diff --git a/nsprpub/pr/tests/Makefile.in b/nsprpub/pr/tests/Makefile.in
> >--- a/nsprpub/pr/tests/Makefile.in
> >+++ b/nsprpub/pr/tests/Makefile.in
> >@@ -450,6 +450,11 @@
> > endif
> > endif
> > 
> >+ifeq ($(ANDROID),1)
> >+LDOPTS=$(OS_LDFLAGS)
> >+LIBPTHREAD=
> >+XCFLAGS=${OS_CFLAGS}
> >+endif
> 
> If this is the only place you use this ANDROID makefile var, then just drop
> that completely and use ifeq ($(OS_TARGET),Android) instead.
> 
Sounds fine for NSPR. Will do.

Comment 9

[Michael Wu [:mwu]]

Filed bug 555826 for the HOST_* flags issue.

Comment 10

[Michael Wu [:mwu]]

Attachment: Add support for building NSPR on Android, v4

Turns out bionic does implement conditional attribute init and destroy. Wrapping HAVE_DLL/USE_DLFCN doesn't work since we're only trying to avoid dlopen(NULL). All other review comments should be addressed.

Comment 11

[(not currently active) Ted Mielczarek]

Comment on attachment 435724 [details] [diff] [review]
Add support for building NSPR on Android, v4

I still don't like those USE_DYLD hacks. All the ifdefs there are feature tests, and you're throwing a platform test into the mix. If the existing HAVE_DLL / USE_DYLD defines don't encompass what you want, can you make up a new one? NO_DLOPEN_NULL or something? Then define it in a header (or from configure), and use it in the ifdefs instead. I think that will make what you're doing a lot clearer.

Other than that one thing, the patch looks fine.

Comment 12

[Michael Wu [:mwu]]

Attachment: Add support for building NSPR on Android, v5

Added NO_DLOPEN_NULL.

Comment 13

[(not currently active) Ted Mielczarek]

Comment on attachment 435966 [details] [diff] [review]
Add support for building NSPR on Android, v5

Thanks, this looks good!

I'll land it in CVS for you shortly.

Comment 14

[(not currently active) Ted Mielczarek]

Landed in CVS:
Checking in configure;
/cvsroot/mozilla/nsprpub/configure,v  <--  configure
new revision: 1.273; previous revision: 1.272
done
Checking in configure.in;
/cvsroot/mozilla/nsprpub/configure.in,v  <--  configure.in
new revision: 1.277; previous revision: 1.276
done
Checking in pr/include/md/_linux.h;
/cvsroot/mozilla/nsprpub/pr/include/md/_linux.h,v  <--  _linux.h
new revision: 3.56; previous revision: 3.55
done
Checking in pr/src/linking/prlink.c;
/cvsroot/mozilla/nsprpub/pr/src/linking/prlink.c,v  <--  prlink.c
new revision: 3.108; previous revision: 3.107
done
Checking in pr/src/malloc/prmem.c;
/cvsroot/mozilla/nsprpub/pr/src/malloc/prmem.c,v  <--  prmem.c
new revision: 3.20; previous revision: 3.19
done
Checking in pr/src/misc/prnetdb.c;
/cvsroot/mozilla/nsprpub/pr/src/misc/prnetdb.c,v  <--  prnetdb.c
new revision: 3.61; previous revision: 3.60
done
Checking in pr/tests/Makefile.in;
/cvsroot/mozilla/nsprpub/pr/tests/Makefile.in,v  <--  Makefile.in
new revision: 1.65; previous revision: 1.64
done