The new Android NDK breaks the Mozilla build in several places. This work-in-progress patch fixes some of the errors, but not all. See also bug 617074.

This fixes some issues with the "signbit" and "isinf" functions in math.h. Currently the stlport-enabled build fails in some GCC-specific hash_map code in chromium.

this builds and runs (as long as you disable crash reporter), however it crashes soon after start up. The stack just has memcpy and then garbage.

(In reply to comment #1) > Created attachment 496262 [details] [diff] [review] > WIP 2 > > This fixes some issues with the "signbit" and "isinf" functions in math.h. I hit these signbit issues with g++ 4.5 with -std=c++0x. While trying different things to fix it, I hit another "fun" issue that your patch will have too: the current Android SDK used on build bots doesn't support std::signbit: /builds/slave/try-mb-br-andrd-r7-bld/build/try/js/src/jsvalue.h:94: error: 'std::signbit' has not been declared

this patch gets us to build and run, but we crash 1-2s after about:home loads

nspr patch split out because configure changes make it huge and ugly

if I comment out this line we don't crash on startup: https://mxr.mozilla.org/mobile-browser/source/chrome/content/browser-ui.js#513 which implies that starting the content process is what's crashing the chrome process

I installed the new NDK, updated my mozconfig, but it fails on 'C compiler cannot create executables', which happens because build/prebuilt does not exist in the downloaded NDK r5. I don't see anything about creating prebuilt/ in our build docs or in the NDK build docs (everything says "unpack and it's ready to use", basically). I guess I must be missing something simple here?

blassey informed me that some additional mozconfig changes are needed, ac_add_options --with-android-ndk=".../android-ndk-r5" ac_add_options --with-android-sdk=".../android-sdk-linux_86/platforms/android-8" ac_add_options --with-android-version=5 ac_add_options --with-android-tools=".../android-sdk-linux_86/tools" ac_add_options --with-android-toolchain=".../android-ndk-r5/toolchains/arm-linux-androideabi-4.4.3/prebuilt/linux-x86" ac_add_options --with-android-platform=".../android-ndk-r5/platforms/android-5/arch-arm"

(In reply to comment #6) > if I comment out this line we don't crash on startup: > https://mxr.mozilla.org/mobile-browser/source/chrome/content/browser-ui. > js#513 > The line there is Cc["@mozilla.org/xre/app-info;1"].getService(Ci.nsIXULRuntime) .ensureContentProcess(); Turns out that what is failing is getting that service. More specifically, getting that service leads to getting the AppShell service, which tries to get the prefService, which tries to add an observer in the observerService - which fails. The failure is due to "Using observer service off the main thread!". Which seems very odd.

It looks like there is a problem with TLS. We set the TLS to mark the main thread in nsThreadManager::Init, then when we start the cycle collector thread in nsCycleCollector_startup, the CC thread mark its TLS, after which the TLS on the main thread is corrupted (1299198128, looks random). As a result, we always think we are not the main thread, even when we are, and everything breaks. Looking in the r5 docs, I see that OVERVIEW.html says > At the moment, thread-local storage defined through the __thread compiler keyword is not supported by the Bionic C library and dynamic linker. There is no corresponding file in r4 to compare to.

blassey found that this was a configure issue. His patch here makes configure not make the mistake of thinking TLS will work. After that, fennec loads, but the child process is in a frozen state. The cause is apparently many of these: I/Gecko ( 6004): WARNING: NS_ENSURE_TRUE(mMainThread) failed: file /home/alon/Dev/mozilla-central/xpcom/threads/nsThreadManager.cpp, line 280 I/Gecko ( 6004): WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0xC1F30001: file /home/alon/Dev/android/fennec/xpcom/build/nsThreadUtils.cpp, line 173 - NS_DispatchToMainThread fails, since NS_ENSURE_TRUE(mMainThread, NS_ERROR_NOT_INITIALIZED); fails in NS_GetMainThread. So nothing is dispatched and the child process is a zombie. Looks like even with no TLS, something went very wrong in initializing nsThreadManager.

The parent process sets up the threadManager, and things like NS_GetMainThread succeed. Then it forks the child, but in XRE_InitChildProcess calling NS_GetMainThread fails, since mMainThread is not initialized as mentioned before. Which seems odd as a fork of a process in which that worked ok. However, we do some tricky things with how we fork and launch the child process, and I am not sure I follow all the steps. Does anyone know, is it possible to not get an identical copy in the forked child for some reason?

Further checks show that fork() is not broken - right after the fork, we do have the proper values in memory in the child process. However, after we fork we do an execvp(), which AFAIK loads a new binary in place of the old one ("The exec() family of functions replaces the current process image with a new process image" the docs say). After we do that, we have the uninitialized values in memory (so, the threadManager is no longer initialized, and things fail). This seems expected though - we are calling LaunchApp after all, and it looks like it just uses fork() as an intermediate tool. So I guess I do not understand how things worked for us before. Did we rely on some trick with execvp to keep part of ourselves in memory?

Some poking around shows we do need to initialize XPCOM in the child process. So that it wasn't happening was a problem. Which was caused by the crashreporter stuff in ContentChild.cpp, commenting that out lets things proceed a little further. The symptoms remain the same, though - frozen unresponsive child process, even with XPCOM and so forth set up properly. Plenty of odd warnings and assertions though, including lots of 'potential deadlock' stuff. Not sure how much of that is expected. Side issue: I think that crashreporter code in ContentChild.cpp should be enclosed in #ifdef MOZ_CRASHREPORTER? Or is it intentionally run even without the crashreporter being enabled?

I worked around the deadlock, which did not fix anything. I also compared the warnings and assertions to linux desktop, and they all seem unrelated to the problem.

Patch to not try to send crash reports if crash reporter is not enabled. With this patch and the other 3, plus a clobber build (apparently necessary...), things work ok with NDK5!

With this patch, we build with crashreporter enabled. I had to hack in two changes inside google-breakpad code, which I am assuming is not a good thing. I don't immediately see an alternative, though. (Perhaps newer version of breakpad have these fixes anyhow?)

blassey tells me that ndk5 builds are slower than ndk4 ones. As an initial step to investigating this, I compared the build commands the ndks generate. The differences are: 1. ndk5 adds an -Include for stlport 2. ndk4 has |-mfpu=vfp|

(In reply to comment #18) > 2. ndk4 has |-mfpu=vfp| IIRC, this made a big change in performance when Vlad switched to use it.

tl;dr looks like NDK5 gives us 15% faster SunSpider and 12% faster V8. I ran some benchmarks on SunSpider and V8 (not Kraken, since it takes too long to load over wifi), on a Galaxy S: SS V8 NDK4 2.705 0.505 NDK5 3.110 0.560 NDK5 mfpu=vfp 3.115 0.570 NDK5 config mfpu 3.055 0.565 Run on dromaeo.com. Results are runs/sec so more is better. Each number is the average of two full runs. Seems to be no difference in the NDK5 variants. The first is just plain, the second is when hacking -mfpu=vfp into the build commands (NDK4 had it by default, NDK5 not, hence the interest in what it does). The last is a configure.in fix from blassey that is related to the mfpu stuff and looks like the correct thing to do (fixes *-android* to *android*, like the other configure.in fix we have). There are some significant changes in .apk size: NDK4 15.25MB NDK5 15.88MB NDK5 mfpu=vfp 20.69MB (!) NDK5 config mfpu 15.88MB So the apparently right option (the last) means we increase the APK size by 4% (also the same amount by which libxul.so increased). Note that I had to do ac_add_options --disable-elf-hack for the NDK5 builds. Can this perhaps be related to the size increase?

(In reply to comment #20) > ac_add_options --disable-elf-hack > > for the NDK5 builds. Can this perhaps be related to the size increase? Likely. Do a NDK4 build without elf hack to compare.

(In reply to comment #20) > Note that I had to do > > ac_add_options --disable-elf-hack > > for the NDK5 builds. What kind of problem did you have?

(In reply to comment #22) > (In reply to comment #20) > > Note that I had to do > > > > ac_add_options --disable-elf-hack > > > > for the NDK5 builds. > > What kind of problem did you have? Hmm I had that on my laptop earlier while figuring out other build problems. I can't reproduce it now on the build machine with the latest patches. Ok, a build with the ELF hack is 15.70MB, which is 3% bigger than NDK4 (compared to 4% bigger without the ELF hack). So a slight improvement but not all of it. Maybe worth it though.

Updated configure.in patch. Fixes for both TLS and mfpu stuff.

(In reply to comment #23) > Ok, a build with the ELF hack is 15.70MB, which is 3% bigger than NDK4 > (compared to 4% bigger without the ELF hack). So a slight improvement but > not all of it. Maybe worth it though. Could you attach libxul.so or put it somewhere? It might be a case of the new toolchain making it difficult for elfhack to do its job.

(In reply to comment #25) > (In reply to comment #23) > > Ok, a build with the ELF hack is 15.70MB, which is 3% bigger than NDK4 > > (compared to 4% bigger without the ELF hack). So a slight improvement but > > not all of it. Maybe worth it though. > > Could you attach libxul.so or put it somewhere? It might be a case of the > new toolchain making it difficult for elfhack to do its job. Sure, here: http://www.syntensity.com/static/staging/libxul.so.tar.bz2

this patch fixes building with ndkr4 and turns vfp back on

(In reply to comment #26) > (In reply to comment #25) > > (In reply to comment #23) > > > Ok, a build with the ELF hack is 15.70MB, which is 3% bigger than NDK4 > > > (compared to 4% bigger without the ELF hack). So a slight improvement but > > > not all of it. Maybe worth it though. > > > > Could you attach libxul.so or put it somewhere? It might be a case of the > > new toolchain making it difficult for elfhack to do its job. > > Sure, here: http://www.syntensity.com/static/staging/libxul.so.tar.bz2 There are around 300KB that could be stripped with elfhack, with R_ARM_ABS32 relocations not being supported (filed bug 658246). On the other hand, the reason why so much of these relocations are there is that libstdc++ is dynamically linked. I thought Android devices didn't come with a bundled libstdc++ ?

Ndk5 introduces libstdc++. You can either link to it statically or dynamically. I if you link to it dynamically you need to package the .so with your app. We are linking statically.

(In reply to comment #33) > Ndk5 introduces libstdc++. You can either link to it statically or > dynamically. I if you link to it dynamically you need to package the .so > with your app. We are linking statically. In the libxul.so azakai linked to, it is not linked statically.

Pushed the "do not send crash reports without crashreporter enabled on android" bit, http://hg.mozilla.org/mozilla-central/rev/1f693cd5a613

Comment on attachment 523727 [details] [diff] [review] nspr patch Review of attachment 523727 [details] [diff] [review]: ----------------------------------------------------------------- ::: nsprpub/build/autoconf/config.sub @@ +1690,5 @@ > vendor=stratus > ;; > + *android*) > + vendor=linux- > + ;; We generally pull this whole file from upstream. I'm okay with taking local changes since we don't update it all that often, but can you try to submit these changes upstream? ::: nsprpub/configure.in @@ +164,5 @@ > + ;; > +esac > + > +case "$target" in > +*android*) Can this be -*android*) (does that match everything you care about?) @@ +1050,4 @@ > RESOLVE_LINK_SYMBOLS=1 > ;; > > +*android*) Same question here.

(In reply to comment #36) > Comment on attachment 523727 [details] [diff] [review] [review] > nspr patch > > Review of attachment 523727 [details] [diff] [review] [review]: > ----------------------------------------------------------------- > > ::: nsprpub/build/autoconf/config.sub > We generally pull this whole file from upstream. I'm okay with taking local > changes since we don't update it all that often, but can you try to submit > these changes upstream? do you know where exactly to submit the patch? > > +case "$target" in > > +*android*) > > Can this be -*android*) (does that match everything you care about?) *-*android* should work. another option would be *-android*|*-linuxandroid*

(In reply to comment #37) > > We generally pull this whole file from upstream. I'm okay with taking local > > changes since we don't update it all that often, but can you try to submit > > these changes upstream? > > do you know where exactly to submit the patch? Contact details are in the file header. > *-*android* should work. another option would be *-android*|*-linuxandroid* Either of those sound fine.

Comment on attachment 532773 [details] [diff] [review] Part 5: crashreporter fixes Review of attachment 532773 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/crashreporter/Makefile.in @@ +100,5 @@ > > ifeq ($(OS_TARGET),Android) > DIRS += fileid > +# NDK5 workarounds > +DEFINES += -D_STLP_CONST_CONSTRUCTOR_BUG -D_STLP_NO_MEMBER_TEMPLATES Can we just put these in configure instead of having to stick them in a couple of random Makefiles? ::: toolkit/crashreporter/google-breakpad/src/client/linux/handler/exception_handler.cc @@ +98,5 @@ > #include "client/linux/minidump_writer/minidump_writer.h" > #include "common/linux/guid_creator.h" > #include "common/linux/eintr_wrapper.h" > > +#include "linux/sched.h" These need to go upstream, I can land them for you. If you can provide me a patch against Breakpad SVN, that'd be even easier: http://code.google.com/p/google-breakpad/source/browse/ You have tested that these don't break the desktop Linux build, right?

Comment on attachment 533427 [details] [diff] [review] rolled up patch Review of attachment 533427 [details] [diff] [review]: ----------------------------------------------------------------- There are some things here that don't quite make sense to me. Why are these changes needed now with NDK r5 and not with previous versions? ::: configure.in @@ +281,5 @@ > + ;; > +esac > + > +case "$target" in > +*android*) Same comment as with the NSPR patch. @@ +1248,4 @@ > darwin*) OS_ARCH=Darwin OS_TARGET=Darwin ;; > esac > case "${target}" in > + *android*) OS_ARCH=Linux OS_TARGET=Android ;; And here, and all the following places in this file. ::: db/sqlite3/src/sqlite3.c @@ +25868,4 @@ > ** If you know that your system does support fdatasync() correctly, > ** then simply compile with -Dfdatasync=fdatasync > */ > +#if (!defined(fdatasync) && !defined(__linux__)) || defined(ANDROID) This needs to be upstreamed. ::: ipc/chromium/src/base/hash_tables.h @@ +19,4 @@ > > #include "base/string16.h" > > +#if defined(COMPILER_MSVC) || defined(ANDROID) && defined(_STLP_STD_NAME) I think this could use extra parentheses on the right-hand side of the ||. ::: js/src/assembler/wtf/Platform.h @@ +329,4 @@ > /* PLATFORM(LINUX) */ > /* Operating system level dependencies for Linux-like systems that */ > /* should be used regardless of operating environment */ > +#if defined(__linux__) && !defined(ANDROID) I don't understand this, what's the reasoning here? ::: js/src/configure.in @@ +272,5 @@ > + ;; > +esac > + > +case "$target" in > +*android*) Same comments about the pattern here as elsewhere. ::: js/src/ctypes/libffi/config.sub @@ +126,4 @@ > nto-qnx* | linux-gnu* | linux-dietlibc | linux-newlib* | linux-uclibc* | \ > uclinux-uclibc* | uclinux-gnu* | kfreebsd*-gnu* | knetbsd*-gnu* | netbsd*-gnu* | \ > kopensolaris*-gnu* | \ > + storm-chaos* | os2-emx* | rtmk-nova* | wince-winmo*) This has a bunch of extra changes, what's up with that? ::: netwerk/base/src/Makefile.in @@ +139,4 @@ > include $(topsrcdir)/ipc/chromium/chromium-config.mk > include $(topsrcdir)/config/rules.mk > > +nsURLParsers.$(OBJ_SUFFIX):MOZ_OPTIMIZE_FLAGS= What's the purpose of this? This needs an explanatory comment. ::: toolkit/xre/nsSigHandlers.cpp @@ +60,4 @@ > #include <sys/resource.h> > #include <unistd.h> > #include <stdlib.h> // atoi > +#if defined(__amd64__) || defined(__i386__) // no arm impl Surely this should just be #ifndef __arm__ ?

> > +#if (!defined(fdatasync) && !defined(__linux__)) || defined(ANDROID) > and > > /* Operating system level dependencies for Linux-like systems that */ > > /* should be used regardless of operating environment */ > > +#if defined(__linux__) && !defined(ANDROID) > > I don't understand this, what's the reasoning here? the new toolchain (arm-linuxandroid-eabi) defines __linux__ where the old toolchain didn't. In these two places we tested for __linux__ and assumed not android which isn't true with the new toolchain > ::: js/src/ctypes/libffi/config.sub > @@ +126,4 @@ > > nto-qnx* | linux-gnu* | linux-dietlibc | linux-newlib* | linux-uclibc* | \ > > uclinux-uclibc* | uclinux-gnu* | kfreebsd*-gnu* | knetbsd*-gnu* | netbsd*-gnu* | \ > > kopensolaris*-gnu* | \ > > + storm-chaos* | os2-emx* | rtmk-nova* | wince-winmo*) > > This has a bunch of extra changes, what's up with that? I simply copied the config.sub from elsewhere in the tree. This copy was apparently a few versions behind > > ::: netwerk/base/src/Makefile.in > @@ +139,4 @@ > > include $(topsrcdir)/ipc/chromium/chromium-config.mk > > include $(topsrcdir)/config/rules.mk > > > > +nsURLParsers.$(OBJ_SUFFIX):MOZ_OPTIMIZE_FLAGS= > > What's the purpose of this? This needs an explanatory comment. I believe gcc was crashing when compiling that file with opt flags. I'll retest to make sure its still necessary and add a comment if it is. > > ::: toolkit/xre/nsSigHandlers.cpp > @@ +60,4 @@ > > #include <sys/resource.h> > > #include <unistd.h> > > #include <stdlib.h> // atoi > > +#if defined(__amd64__) || defined(__i386__) // no arm impl > > Surely this should just be #ifndef __arm__ ? sure

updated nspr patch based on comments, carrying r+

updated based on review

pushed to try http://tbpl.mozilla.org/?tree=Try&rev=c7dc12714194

Comment on attachment 534985 [details] [diff] [review] patch Review of attachment 534985 [details] [diff] [review]: ----------------------------------------------------------------- ::: netwerk/base/src/Makefile.in @@ +141,5 @@ > include $(topsrcdir)/config/rules.mk > > +ifeq ($(OS_TARGET),Android) > +# this works around a "branch out of range" error when compiling this file opt > +nsURLParsers.$(OBJ_SUFFIX):MOZ_OPTIMIZE_FLAGS= Nit: can you stick a space after the colon here? Makes it slightly more readable. ::: toolkit/xre/nsSigHandlers.cpp @@ +190,4 @@ > *mxcsr &= ~SSE_STATUS_FLAGS; /* clear all pending SSE exceptions */ > #endif > #endif > +#if defined(LINUX) && (defined(__amd64__) || defined(__i386__)) This should probably be && !defined(__arm__) as well, then.

pushed http://hg.mozilla.org/mozilla-central/rev/b854ffeef0d1 Ted said he'll push the NSPR changes to NSPR's CVS. Wan-Teh, can we get a tag so we can update NSPR in mozilla-central to pick this up?

Landed in NSPR CVS: Checking in configure; /cvsroot/mozilla/nsprpub/configure,v <-- configure new revision: 1.304; previous revision: 1.303 done Checking in configure.in; /cvsroot/mozilla/nsprpub/configure.in,v <-- configure.in new revision: 1.306; previous revision: 1.305 done Checking in build/autoconf/config.sub; /cvsroot/mozilla/nsprpub/build/autoconf/config.sub,v <-- config.sub new revision: 1.19; previous revision: 1.18 done

Ted: you can create a NSPR_4_8_9_BETA2 CVS tag and push it to mozilla-central.

Okay, I have tagged NSPR tip with that. blassey: feel free to push that tag to mozilla-central.

blassey: FYI, the NSPR_4_8_9_BETA2 tag contains the fixes for the following bugs: - bug 617115 (this bug) - bug 647820 - bug 564851 - Bug 650474 (just the last patch in the bug)

pushed nspr patch as part of http://hg.mozilla.org/mozilla-central/rev/b20b6d500c05

Reopening since we still have the 'Part 5' patch (crashreporter stuff) to finish.

(In reply to comment #39) > Comment on attachment 532773 [details] [diff] [review] [review] > Part 5: crashreporter fixes > > Review of attachment 532773 [details] [diff] [review] [review]: > ----------------------------------------------------------------- > > ::: toolkit/crashreporter/Makefile.in > @@ +100,5 @@ > > > > ifeq ($(OS_TARGET),Android) > > DIRS += fileid > > +# NDK5 workarounds > > +DEFINES += -D_STLP_CONST_CONSTRUCTOR_BUG -D_STLP_NO_MEMBER_TEMPLATES > > Can we just put these in configure instead of having to stick them in a > couple of random Makefiles? > It looks like the build command for crashreporter succeeds when I put the defines in a -D, but if they appear inside mozilla-config.h, they have no effect, even though that file is -include'ed. Perhaps there is an order issue of some sort I can't figure out.

Ok, the problem is that having those defines globally will cause files in /ipc/chromium/ to fail. So, I'd like to go with the "Part 5: crashreporter fixes" patch as-is. ted, is that ok with you?

Pushed since we want this ASAP. I can push a followup if anything needs to be changed. http://hg.mozilla.org/mozilla-central/rev/02a5505b965b I'll prepare a patch for upstream crashreporter against their svn.

ted, here is a patch against upstream breakpad svn. I didn't test it since I'm not sure how, but it should work.

Marking closed since all done here, except for the upstream patch.

Okay, unfortunate that you need to do those defines that way, but that's fine. I pushed the Breakpad patch upstream, thanks for making that easy for me! http://code.google.com/p/google-breakpad/source/detail?r=790

(In reply to Ted Mielczarek [:ted, :luser] from comment #40) > ::: db/sqlite3/src/sqlite3.c > @@ +25868,4 @@ > > ** If you know that your system does support fdatasync() correctly, > > ** then simply compile with -Dfdatasync=fdatasync > > */ > > +#if (!defined(fdatasync) && !defined(__linux__)) || defined(ANDROID) > > This needs to be upstreamed. In the future, this needs to be upstreamed *BEFORE* we take it. Changing SQLite without telling the person who has to maintain the code that wraps it is *not* cool. You've already complicated the upgrade to a newer version, which is one of the reasons why we don't take changes to SQLite in our tree.