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.

Roll up bcombee's changes.

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

Forgot to change a message in configure.in

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

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.

(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.

Filed bug 555826 for the HOST_* flags issue.

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 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.

Added NO_DLOPEN_NULL.

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.

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