Last Comment Bug 807423 - Android SpiderMonkey should support using GNU libstdc++ for providing C++ support
: Android SpiderMonkey should support using GNU libstdc++ for providing C++ sup...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Build Config (show other bugs)
: unspecified
: ARM Android
: -- normal (vote)
: mozilla19
Assigned To: folecr
:
: Gregory Szorc [:gps]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-10-31 11:47 PDT by folecr
Modified: 2012-11-20 10:18 PST (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (2.63 KB, patch)
2012-11-03 01:23 PDT, folecr
no flags Details | Diff | Splinter Review
Allows building JS with GNU libstdc++ on Android (4.54 KB, patch)
2012-11-06 14:02 PST, folecr
no flags Details | Diff | Splinter Review
Updated patch : changes to js/src/build/autoconf/android.m4 copied to build/autoconf/android.m4 (8.61 KB, patch)
2012-11-13 14:03 PST, folecr
ted: review+
mh+mozilla: review-
Details | Diff | Splinter Review

Description folecr 2012-10-31 11:47:23 PDT
Android build of SpiderMonkey should support GNU libstdc++. Currently the Android build is hardcoded to use STLPORT.
Comment 1 folecr 2012-11-03 01:23:40 PDT
Created attachment 677988 [details] [diff] [review]
Patch
Comment 2 folecr 2012-11-03 01:24:41 PDT
References
1) old mozilla configure.in
   http://hg.mozilla.org/mozilla-central/file/a535ed0dc49f/configure.in#l1617
2) Current mozilla android.m4
   http://hg.mozilla.org/mozilla-central/file/5a29e32cc48b/build/autoconf/android.m4#l162
3) My patch to build SpiderMonkey with GNU libstdc++ on Android
   https://github.com/folecr/spidermonkey.riq/commit/012a932166315276cc8689a669bbe4c0c15bd023

This patch
 * Tests for MOZ_ANDROID_LIBSTDCXX, like in (1), and if set selects GNU libstdc++
 * Sets STLPORT_... variables as in (1) even when GNU libstdc++ is used
 * Uses the same flags for GNU libstdc++ as in (1).
 * Uses a new variable : android_gnu_compiler_version which can either be "4.6" (default) or "4.4.3"
 * Uses location of GNU libstdc++ in Android NDK, depending on the compiler version used

Tested
 * By building and running SpiderMonkey JS on Android

Questions
 * Is STLPORT_SOURCES really used?
 * What are Mozilla's currently supported NDK versions?
 * Is -D_GLIBCXX_PERMIT_BACKWARD_HASH required?

Future
 * The name STLPORT_... for variables is misleading.
 * Should be renamed to something more generic so that it refers to both options (STLPORT and GNULIBSTDC++)
 * Perhaps to ANDROIDCXX_... in a separate patch.
Comment 3 Ted Mielczarek [:ted.mielczarek] 2012-11-03 10:03:01 PDT
(In reply to folecr from comment #2)
> Questions
>  * Is STLPORT_SOURCES really used?

Yes, we build our own copy of STLport as part of the build so that it matches the compiler flags we use to build the rest of Firefox. (We had issues with this on ARMv6 builds.)

>  * What are Mozilla's currently supported NDK versions?

Our existing builds are done with NDK r5c, so that's the oldest we need to support. I think we have some issues building with NDK r8, but we'd like to support that as well.

>  * Is -D_GLIBCXX_PERMIT_BACKWARD_HASH required?

I have no idea.
Comment 4 Ted Mielczarek [:ted.mielczarek] 2012-11-03 10:04:02 PDT
Once you're happy with the patch you have here, you can ask for review by selecting review [?] on the attachment details page and then putting my email address in the text field.
Comment 5 Mike Hommey [:glandium] 2012-11-03 12:30:46 PDT
One thing that is bothering is that modifying android.m4 in js/src requires synchronizing the one at toplevel in m-c. Which means the flag will also be available when building firefox, and i'm pretty sure it won't work... so it would be better to avoid exposing it when building something else than js/src.
Comment 6 folecr 2012-11-04 07:06:14 PST
> so it would be better to avoid exposing it when building something else than js/src.

What is the recommended way to do this? Is there an example in the current build file?
Longer term - perhaps libstdc++ should be supported for the entire Firefox build?

> >  * Is -D_GLIBCXX_PERMIT_BACKWARD_HASH required?
> I have no idea.

How do I find out? I would hate to cause a hard-to-track-down bug by including/omitting this flag.
Comment 7 Mike Hommey [:glandium] 2012-11-04 10:12:23 PST
(In reply to folecr from comment #6)
> > so it would be better to avoid exposing it when building something else than js/src.
> 
> What is the recommended way to do this? Is there an example in the current
> build file?

I don't think there is. I'd add a m4 macro in js/src/configure.in and check if it's defined in android.m4.

> Longer term - perhaps libstdc++ should be supported for the entire Firefox
> build?

I don't think it should, mainly for the same reason we are (re)building stlport on our own.
Comment 8 folecr 2012-11-05 15:24:25 PST
ComputeIsJITBroken() in jscntxt.cpp http://hg.mozilla.org/mozilla-cent...355501/js/src/jscntxt.cpp#l1464

That one function introduces a dependency on <fstream> and <string> to the js build. Without it js builds fine on all versions of the NDK.
Comment 9 Ted Mielczarek [:ted.mielczarek] 2012-11-06 06:50:56 PST
FWIW, I'm okay if the libstdc++ support doesn't work on older versions of the NDK. We need to continue to support at least NDK r5 for our official builds, but they'll continue to use stlport. As long as your patch works in your configuration that's fine with me.
Comment 10 folecr 2012-11-06 13:58:17 PST
Sounds good to me.
Comment 11 folecr 2012-11-06 14:02:02 PST
Created attachment 678908 [details] [diff] [review]
Allows building JS with GNU libstdc++ on Android
Comment 12 folecr 2012-11-06 14:04:51 PST
Notes about this patch

 - Can build JS with GNU libstdc++ when using NDK r7 and above
 - I can modify ComputeIsJITBroken() so it only uses C code but it doesn't seem like this is necessary (See Ted's comment above)
 - I've removed -D_GLIBCXX_PERMIT_BACKWARD_HASH since I haven't been able to find out why it was added
Comment 13 folecr 2012-11-13 08:34:31 PST
Is it possible to get this patch to a try server to ensure that it doesn't break any of the current android builds? (it shouldn't)
Comment 14 Ted Mielczarek [:ted.mielczarek] 2012-11-13 12:01:42 PST
Pushed to try:
https://tbpl.mozilla.org/?tree=Try&rev=6907a9c6680b
Comment 15 folecr 2012-11-13 14:03:39 PST
Created attachment 681203 [details] [diff] [review]
Updated patch : changes to js/src/build/autoconf/android.m4 copied to build/autoconf/android.m4
Comment 16 folecr 2012-11-13 14:05:17 PST
Could you please push the updated patch to try? Thanks.
Comment 17 Ted Mielczarek [:ted.mielczarek] 2012-11-14 07:32:04 PST
https://tbpl.mozilla.org/?tree=Try&rev=2deb2f59857b
Comment 18 folecr 2012-11-14 10:11:54 PST
All green!
Comment 19 Ted Mielczarek [:ted.mielczarek] 2012-11-15 11:46:52 PST
Comment on attachment 681203 [details] [diff] [review]
Updated patch : changes to js/src/build/autoconf/android.m4 copied to build/autoconf/android.m4

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

Looks fine and passed Try, so sounds good.

::: build/autoconf/android.m4
@@ +18,5 @@
> +dnl default gnu compiler version is 4.4.3
> +android_gnu_compiler_version=4.4.3
> +
> +MOZ_ARG_WITH_STRING(android-gnu-compiler-version,
> +[  --with-android-gnu-compiler-version=VER

I would probably call this "--with-android-gcc-version" or "--with-android-toolchain-version", but I don't think I care enough to make you change it.
Comment 20 Ted Mielczarek [:ted.mielczarek] 2012-11-15 12:06:07 PST
You can add the checkin-needed keyword to this bug if you''d like someone to land it for you.
Comment 21 Ryan VanderMeulen [:RyanVM] 2012-11-15 17:41:08 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/7f785ec19024
Comment 22 Mike Hommey [:glandium] 2012-11-15 23:02:06 PST
Comment on attachment 681203 [details] [diff] [review]
Updated patch : changes to js/src/build/autoconf/android.m4 copied to build/autoconf/android.m4

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

::: build/autoconf/android.m4
@@ +18,5 @@
> +dnl default gnu compiler version is 4.4.3
> +android_gnu_compiler_version=4.4.3
> +
> +MOZ_ARG_WITH_STRING(android-gnu-compiler-version,
> +[  --with-android-gnu-compiler-version=VER

Actually, please change it. NDK r8c comes with new flavours with clang, and giving --with-android-gnu-compiler-version=clang3.1 is completely weird.
Comment 23 Ed Morley [:emorley] 2012-11-16 09:48:07 PST
https://hg.mozilla.org/mozilla-central/rev/7f785ec19024
Comment 24 Mike Hommey [:glandium] 2012-11-16 12:17:33 PST
> 20:54  <folecr> glandium: about your comment on bug id 807423 … 
> clang comes with it's own c++ implementation right? so it shouldn't
> really be using gnu libstdc++ but it's own implementation. Using
> clang probably needs a separate flag/configure option.

The --with-android-gnu-compiler-version option you added is only half related to libstdc++. You're also making it change the toolchain being used. Which means that to use clang, you'd need to use that option.
Comment 25 folecr 2012-11-20 10:18:54 PST
Sounds good. I'll change the flag name and submit a patch.

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