807423 - Android SpiderMonkey should support using GNU libstdc++ for providing C++ support

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[folecr]

Android build of SpiderMonkey should support GNU libstdc++. Currently the Android build is hardcoded to use STLPORT.

Comment 1

[folecr]

Attachment: Patch

Comment 2

[folecr]

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

[(not currently active) Ted Mielczarek]

(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

[(not currently active) Ted Mielczarek]

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

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]

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

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

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

[(not currently active) Ted Mielczarek]

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]

Sounds good to me.

Comment 11

[folecr]

Attachment: Allows building JS with GNU libstdc++ on Android

Comment 12

[folecr]

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]

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

[(not currently active) Ted Mielczarek]

Pushed to try:
https://tbpl.mozilla.org/?tree=Try&rev=6907a9c6680b

Comment 15

[folecr]

Attachment: Updated patch : changes to js/src/build/autoconf/android.m4 copied to build/autoconf/android.m4

Comment 16

[folecr]

Could you please push the updated patch to try? Thanks.

Comment 17

[(not currently active) Ted Mielczarek]

https://tbpl.mozilla.org/?tree=Try&rev=2deb2f59857b

Comment 18

[folecr]

All green!

Comment 19

[(not currently active) Ted Mielczarek]

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

[(not currently active) Ted Mielczarek]

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/7f785ec19024

Comment 22

[Mike Hommey [:glandium]]

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

https://hg.mozilla.org/mozilla-central/rev/7f785ec19024

Comment 24

[Mike Hommey [:glandium]]

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

Sounds good. I'll change the flag name and submit a patch.