Last Comment Bug 617115 - Fixes for building with Android NDK r5
: Fixes for building with Android NDK r5
Status: RESOLVED FIXED
: footprint, mobile, perf
Product: Core
Classification: Components
Component: Build Config (show other bugs)
: Trunk
: All Android
: -- normal with 1 vote (vote)
: ---
Assigned To: Alon Zakai (:azakai)
:
Mentors:
Depends on: 640494 658246 661463 663214 697709
Blocks: 658074 670315
  Show dependency treegraph
 
Reported: 2010-12-06 13:18 PST by Matt Brubeck (:mbrubeck)
Modified: 2011-11-02 11:21 PDT (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP (7.49 KB, patch)
2010-12-06 13:18 PST, Matt Brubeck (:mbrubeck)
no flags Details | Diff | Splinter Review
WIP 2 (73.50 KB, patch)
2010-12-08 13:41 PST, Matt Brubeck (:mbrubeck)
no flags Details | Diff | Splinter Review
WIP 3 (66.96 KB, patch)
2011-01-03 00:49 PST, Brad Lassey [:blassey] (use needinfo?)
no flags Details | Diff | Splinter Review
WIP patch v4 (18.98 KB, patch)
2011-04-01 16:54 PDT, Brad Lassey [:blassey] (use needinfo?)
no flags Details | Diff | Splinter Review
nspr patch (45.00 KB, patch)
2011-04-01 16:55 PDT, Brad Lassey [:blassey] (use needinfo?)
ted: review+
Details | Diff | Splinter Review
do not use TLS (483 bytes, patch)
2011-05-12 17:01 PDT, Alon Zakai (:azakai)
no flags Details | Diff | Splinter Review
do not send crash reports without crashreporter (901 bytes, patch)
2011-05-13 17:29 PDT, Alon Zakai (:azakai)
blassey.bugs: review+
Details | Diff | Splinter Review
Part 5: crashreporter fixes (2.98 KB, patch)
2011-05-16 15:52 PDT, Alon Zakai (:azakai)
ted: review+
blassey.bugs: feedback+
Details | Diff | Splinter Review
Part 3: configure.in fixes (1009 bytes, patch)
2011-05-17 17:29 PDT, Alon Zakai (:azakai)
no flags Details | Diff | Splinter Review
patch to turn on vfp and fix building with ndk4 (8.76 KB, patch)
2011-05-18 14:04 PDT, Brad Lassey [:blassey] (use needinfo?)
no flags Details | Diff | Splinter Review
rolled up patch (45.00 KB, patch)
2011-05-18 14:17 PDT, Brad Lassey [:blassey] (use needinfo?)
no flags Details | Diff | Splinter Review
rolled up patch (69.83 KB, patch)
2011-05-18 14:20 PDT, Brad Lassey [:blassey] (use needinfo?)
no flags Details | Diff | Splinter Review
rolled up patch (24.82 KB, patch)
2011-05-18 14:23 PDT, Brad Lassey [:blassey] (use needinfo?)
no flags Details | Diff | Splinter Review
rolled up patch (23.74 KB, patch)
2011-05-18 14:33 PDT, Brad Lassey [:blassey] (use needinfo?)
ted: review-
Details | Diff | Splinter Review
nspr patch (45.12 KB, patch)
2011-05-24 21:42 PDT, Brad Lassey [:blassey] (use needinfo?)
blassey.bugs: review+
Details | Diff | Splinter Review
patch (23.67 KB, patch)
2011-05-24 21:44 PDT, Brad Lassey [:blassey] (use needinfo?)
ted: review+
Details | Diff | Splinter Review
breakpad patch against upstream svn (892 bytes, patch)
2011-05-27 10:29 PDT, Alon Zakai (:azakai)
no flags Details | Diff | Splinter Review

Description Matt Brubeck (:mbrubeck) 2010-12-06 13:18:48 PST
Created attachment 495603 [details] [diff] [review]
WIP

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.
Comment 1 Matt Brubeck (:mbrubeck) 2010-12-08 13:41:07 PST
Created attachment 496262 [details] [diff] [review]
WIP 2

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.
Comment 2 Brad Lassey [:blassey] (use needinfo?) 2011-01-03 00:49:43 PST
Created attachment 500755 [details] [diff] [review]
WIP 3

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.
Comment 3 Mike Hommey [:glandium] 2011-03-10 01:56:03 PST
(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
Comment 4 Brad Lassey [:blassey] (use needinfo?) 2011-04-01 16:54:28 PDT
Created attachment 523726 [details] [diff] [review]
WIP patch v4

this patch gets us to build and run, but we crash 1-2s after about:home loads
Comment 5 Brad Lassey [:blassey] (use needinfo?) 2011-04-01 16:55:54 PDT
Created attachment 523727 [details] [diff] [review]
nspr patch

nspr patch split out because configure changes make it huge and ugly
Comment 6 Brad Lassey [:blassey] (use needinfo?) 2011-04-01 17:09:42 PDT
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
Comment 7 Alon Zakai (:azakai) 2011-05-10 10:06:38 PDT
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?
Comment 8 Alon Zakai (:azakai) 2011-05-10 12:19:29 PDT
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"
Comment 9 Alon Zakai (:azakai) 2011-05-11 15:17:28 PDT
(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.
Comment 10 Alon Zakai (:azakai) 2011-05-11 17:33:25 PDT
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.
Comment 11 Alon Zakai (:azakai) 2011-05-12 17:01:47 PDT
Created attachment 532081 [details] [diff] [review]
do not use TLS

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.
Comment 12 Alon Zakai (:azakai) 2011-05-12 18:32:24 PDT
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?
Comment 13 Alon Zakai (:azakai) 2011-05-13 10:48:55 PDT
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?
Comment 14 Alon Zakai (:azakai) 2011-05-13 13:54:34 PDT
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?
Comment 15 Alon Zakai (:azakai) 2011-05-13 15:41:39 PDT
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.
Comment 16 Alon Zakai (:azakai) 2011-05-13 17:29:23 PDT
Created attachment 532394 [details] [diff] [review]
do not send crash reports without crashreporter

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!
Comment 17 Alon Zakai (:azakai) 2011-05-16 15:52:35 PDT
Created attachment 532773 [details] [diff] [review]
Part 5: crashreporter fixes

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?)
Comment 18 Alon Zakai (:azakai) 2011-05-16 16:29:31 PDT
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|
Comment 19 Mark Finkle (:mfinkle) (use needinfo?) 2011-05-16 18:53:54 PDT
(In reply to comment #18)

> 2. ndk4 has |-mfpu=vfp|

IIRC, this made a big change in performance when Vlad switched to use it.
Comment 20 Alon Zakai (:azakai) 2011-05-17 13:57:15 PDT
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?
Comment 21 Michael Wu [:mwu] 2011-05-17 14:03:54 PDT
(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.
Comment 22 Mike Hommey [:glandium] 2011-05-17 14:08:47 PDT
(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?
Comment 23 Alon Zakai (:azakai) 2011-05-17 15:12:01 PDT
(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.
Comment 24 Alon Zakai (:azakai) 2011-05-17 17:29:47 PDT
Created attachment 533132 [details] [diff] [review]
Part 3: configure.in fixes

Updated configure.in patch. Fixes for both TLS and mfpu stuff.
Comment 25 Mike Hommey [:glandium] 2011-05-17 21:44:03 PDT
(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.
Comment 26 Alon Zakai (:azakai) 2011-05-18 10:14:06 PDT
(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
Comment 27 Brad Lassey [:blassey] (use needinfo?) 2011-05-18 14:04:03 PDT
Created attachment 533410 [details] [diff] [review]
patch to turn on vfp and fix building with ndk4

this patch fixes building with ndkr4 and turns vfp back on
Comment 28 Brad Lassey [:blassey] (use needinfo?) 2011-05-18 14:17:07 PDT
Created attachment 533417 [details] [diff] [review]
rolled up patch
Comment 29 Brad Lassey [:blassey] (use needinfo?) 2011-05-18 14:20:18 PDT
Created attachment 533420 [details] [diff] [review]
rolled up patch
Comment 30 Brad Lassey [:blassey] (use needinfo?) 2011-05-18 14:23:09 PDT
Created attachment 533422 [details] [diff] [review]
rolled up patch
Comment 31 Brad Lassey [:blassey] (use needinfo?) 2011-05-18 14:33:59 PDT
Created attachment 533427 [details] [diff] [review]
rolled up patch
Comment 32 Mike Hommey [:glandium] 2011-05-19 06:32:34 PDT
(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++ ?
Comment 33 Brad Lassey [:blassey] (use needinfo?) 2011-05-19 06:40:03 PDT
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.
Comment 34 Mike Hommey [:glandium] 2011-05-19 06:58:04 PDT
(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.
Comment 35 Alon Zakai (:azakai) 2011-05-20 10:14:58 PDT
Pushed the "do not send crash reports without crashreporter enabled on android" bit,

http://hg.mozilla.org/mozilla-central/rev/1f693cd5a613
Comment 36 Ted Mielczarek [:ted.mielczarek] 2011-05-24 09:02:11 PDT
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.
Comment 37 Brad Lassey [:blassey] (use needinfo?) 2011-05-24 09:47:26 PDT
(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*
Comment 38 Ted Mielczarek [:ted.mielczarek] 2011-05-24 09:58:03 PDT
(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 39 Ted Mielczarek [:ted.mielczarek] 2011-05-24 10:21:16 PDT
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 40 Ted Mielczarek [:ted.mielczarek] 2011-05-24 10:30:08 PDT
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__ ?
Comment 41 Brad Lassey [:blassey] (use needinfo?) 2011-05-24 10:37:57 PDT
> > +#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
Comment 42 Brad Lassey [:blassey] (use needinfo?) 2011-05-24 21:42:53 PDT
Created attachment 534984 [details] [diff] [review]
nspr patch

updated nspr patch based on comments, carrying r+
Comment 43 Brad Lassey [:blassey] (use needinfo?) 2011-05-24 21:44:20 PDT
Created attachment 534985 [details] [diff] [review]
patch

updated based on review
Comment 44 Brad Lassey [:blassey] (use needinfo?) 2011-05-24 21:45:17 PDT
pushed to try http://tbpl.mozilla.org/?tree=Try&rev=c7dc12714194
Comment 45 Ted Mielczarek [:ted.mielczarek] 2011-05-26 05:44:57 PDT
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.
Comment 46 Brad Lassey [:blassey] (use needinfo?) 2011-05-26 08:31:23 PDT
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?
Comment 47 Ted Mielczarek [:ted.mielczarek] 2011-05-26 10:07:45 PDT
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
Comment 48 Wan-Teh Chang 2011-05-26 10:27:59 PDT
Ted: you can create a NSPR_4_8_9_BETA2 CVS tag and push it to mozilla-central.
Comment 49 Ted Mielczarek [:ted.mielczarek] 2011-05-26 10:33:08 PDT
Okay, I have tagged NSPR tip with that. blassey: feel free to push that tag to mozilla-central.
Comment 50 Wan-Teh Chang 2011-05-26 10:44:56 PDT
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)
Comment 51 Brad Lassey [:blassey] (use needinfo?) 2011-05-26 12:58:01 PDT
pushed nspr patch as part of http://hg.mozilla.org/mozilla-central/rev/b20b6d500c05
Comment 52 Alon Zakai (:azakai) 2011-05-26 13:28:12 PDT
Reopening since we still have the 'Part 5' patch (crashreporter stuff) to finish.
Comment 53 Alon Zakai (:azakai) 2011-05-26 14:50:22 PDT
(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.
Comment 54 Alon Zakai (:azakai) 2011-05-26 16:07:58 PDT
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?
Comment 55 Alon Zakai (:azakai) 2011-05-27 10:04:54 PDT
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.
Comment 56 Alon Zakai (:azakai) 2011-05-27 10:29:07 PDT
Created attachment 535681 [details] [diff] [review]
breakpad patch against upstream 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.
Comment 57 Alon Zakai (:azakai) 2011-05-27 10:30:21 PDT
Marking closed since all done here, except for the upstream patch.
Comment 58 Ted Mielczarek [:ted.mielczarek] 2011-05-31 05:13:45 PDT
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
Comment 59 Shawn Wilsher :sdwilsh 2011-08-25 23:32:18 PDT
(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.

Note You need to log in before you can comment on or make changes to this bug.