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

RESOLVED FIXED in mozilla19

Status

()

Core
Build Config
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: folecr, Assigned: folecr)

Tracking

unspecified
mozilla19
ARM
Android
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

5 years ago
Android build of SpiderMonkey should support GNU libstdc++. Currently the Android build is hardcoded to use STLPORT.
(Assignee)

Comment 1

5 years ago
Created attachment 677988 [details] [diff] [review]
Patch
(Assignee)

Comment 2

5 years ago
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.
(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.
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.
Assignee: nobody → folecr
Status: UNCONFIRMED → NEW
Ever confirmed: true
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.
(Assignee)

Comment 6

5 years ago
> 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.
(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.
(Assignee)

Comment 8

5 years ago
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.
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.
(Assignee)

Comment 10

5 years ago
Sounds good to me.
(Assignee)

Updated

5 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 11

5 years ago
Created attachment 678908 [details] [diff] [review]
Allows building JS with GNU libstdc++ on Android
(Assignee)

Comment 12

5 years ago
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
(Assignee)

Updated

5 years ago
Attachment #678908 - Flags: review?(ted)
(Assignee)

Comment 13

5 years ago
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)
Pushed to try:
https://tbpl.mozilla.org/?tree=Try&rev=6907a9c6680b
(Assignee)

Comment 15

5 years ago
Created attachment 681203 [details] [diff] [review]
Updated patch : changes to js/src/build/autoconf/android.m4 copied to build/autoconf/android.m4
Attachment #677988 - Attachment is obsolete: true
Attachment #678908 - Attachment is obsolete: true
Attachment #678908 - Flags: review?(ted)
Attachment #681203 - Flags: review?
(Assignee)

Updated

5 years ago
Attachment #681203 - Flags: review? → review?(ted)
(Assignee)

Comment 16

5 years ago
Could you please push the updated patch to try? Thanks.
https://tbpl.mozilla.org/?tree=Try&rev=2deb2f59857b
(Assignee)

Comment 18

5 years ago
All green!
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.
Attachment #681203 - Flags: review?(ted) → review+
You can add the checkin-needed keyword to this bug if you''d like someone to land it for you.
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/7f785ec19024
Keywords: checkin-needed
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.
Attachment #681203 - Flags: review-
https://hg.mozilla.org/mozilla-central/rev/7f785ec19024
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
> 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.
(Assignee)

Comment 25

5 years ago
Sounds good. I'll change the flag name and submit a patch.
You need to log in before you can comment on or make changes to this bug.