Closed Bug 1164921 Opened 5 years ago Closed 5 years ago

make Fennec buildable with something other than stlport

Categories

(Firefox Build System :: General, defect)

ARM
All
defect
Not set

Tracking

(firefox44 fixed)

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: froydnj, Assigned: froydnj)

References

Details

Attachments

(3 files, 4 obsolete files)

Our in-tree copy of stlport is holding us back from using many useful C++11 STL features.  It's time to fix that.

We have support for building with the libstdc++ that comes in the NDK already (--enable-android-libstdcxx); I'd like to modify that so we can support building with libstdc++, libc++, stlport, or the in-tree stlport.  (The stlport and in-tree stlport options would probably go away very shortly after we switch over to building with libstdc++/libc++, as people start using features that stlport doesn't support.)

Building with the NDK's libstdc++ (statically linked) adds ~20K of text to libxul.so.  I assume the numbers are similar for libc++, though I haven't tried that yet.

The NDK's libraries *are* built with -fexceptions -frtti; I don't know if we can live with that, or whether we'd have to do something like compile the sources afresh ourselves.

mwu, are there any obstacles to making the B2G build use something other than stlport?  Partner considerations, or anything like that?
Flags: needinfo?(mwu)
Assuming I haven't screwed up too badly, this bit adds a new configure option,
--enable-android-cxx-stl, so that we can pick what STL we want to use from the
NDK (all statically linked for the moment).  There's also a mozstlport option
(the default) for building with our own in-tree stlport.

Nothing enables this right now, just like the existing
--enable-android-libstdcxx.  I expect that very few people will ever use this
in their mozconfigs, except people doing some sort of perf/size testing.

One known problem with this is it selects the ARM-mode versions of the
libraries, whereas we want the thumb versions (assuming we're compiling for
thumb).  That will need to get addressed somehow, so just asking for feedback
at the moment.
Attachment #8605906 - Flags: feedback?(nalexander)
This should be reasonably non-controversial, assuming the previous part works
out.
Attachment #8605907 - Flags: review?(nalexander)
Attachment #8605906 - Flags: feedback?(nalexander) → feedback?(mh+mozilla)
Attachment #8605907 - Flags: review?(nalexander) → review?(mh+mozilla)
froydnj: sorry, these are out of my area :(  glandium is the keeper of the keys.
(In reply to Nick Alexander :nalexander from comment #3)
> froydnj: sorry, these are out of my area :(  glandium is the keeper of the
> keys.

No worries, thanks for redirecting them!
Comment on attachment 8605906 [details] [diff] [review]
part 1 - add --enable-android-cxx-stl to select the C++ STL to use

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

::: build/autoconf/android.m4
@@ +230,5 @@
> +
> +            STLPORT_LIBS="-L$ndk_libs -lgnustl_static"
> +            STLPORT_CPPFLAGS="-I$ndk_include -I$ndk_include/backward -I$ndk_libs/include"
> +            ;;
> +        libc++)

you mean stlport here

@@ +243,5 @@
> +
> +            STLPORT_LIBS="-L$ndk_libs -lc++_static
> +            STLPORT_CPPFLAGS="-I$ndk_include -I$ndk_libs/include"
> +            ;;
> +        stlport)

and libc++ here ;)

That said, I'm not convinced adding ndk stlport support is worth it, especially if it's meant to go away soon after.
Attachment #8605906 - Flags: feedback?(mh+mozilla) → feedback+
Attachment #8605907 - Flags: review?(mh+mozilla) → review+
I'm not sure if I can usefully comment without seeing how you're doing to do it on Gonk. We don't use the NDK to build Gecko on Gonk.
Flags: needinfo?(mwu)
(In reply to Michael Wu [:mwu] from comment #6)
> I'm not sure if I can usefully comment without seeing how you're doing to do
> it on Gonk. We don't use the NDK to build Gecko on Gonk.

Bleh, that makes life much more difficult.  I guess we'll have to import libc++ or something.
OK, now with correct support for thumb, and tested with both libstdc++ and libc++.
Attachment #8605906 - Attachment is obsolete: true
Attachment #8611454 - Flags: review?(mh+mozilla)
Comment on attachment 8611454 [details] [diff] [review]
part 1 - add --with-android-cxx-stl to select the C++ STL to use

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

As a followup, it would be good to rename all the STLPORT_* variables and related things in the build system to reflect that, well, this is not necessarily about stlport.

::: build/autoconf/android.m4
@@ +252,5 @@
> +
> +            STLPORT_LIBS="-L$cxx_libs -lc++_static"
> +            # Add android/support/include/ for prototyping long double math
> +            # functions, locale-specific C library functions, multibyte support,
> +            # etc.  

trailing whitespace.
Attachment #8611454 - Flags: review?(mh+mozilla) → review+
(In reply to Michael Wu [:mwu] from comment #6)
> I'm not sure if I can usefully comment without seeing how you're doing to do
> it on Gonk. We don't use the NDK to build Gecko on Gonk.

Gonk should have a copy of libc++.
(In reply to Mike Hommey [:glandium] from comment #10)
> (In reply to Michael Wu [:mwu] from comment #6)
> > I'm not sure if I can usefully comment without seeing how you're doing to do
> > it on Gonk. We don't use the NDK to build Gecko on Gonk.
> 
> Gonk should have a copy of libc++.

Do you know offhand where that copy of libc++ (not libstdc++?) lives in the gonk build world?  Is it easily accessible via |gcc -print-file-name=|?
Flags: needinfo?(mh+mozilla)
I don't know. I presume android has libc++ somewhere now, so it should be importable in gonk.
Flags: needinfo?(mh+mozilla)
My B2G tree is still (!) downloading, but if I understand the git repos correctly, some (all?) trees will have the .h/.cpp sources from the NDK available, but not the .a/.so files (boo!).
It's not necessarily about the NDK. And gonk is being built, too, so having source only is not really a problem, as long as what you want is part of what's built.
I would like to avoid having to build source, if possible.  We certainly can, but since we know Fennec already works with precompiled things from the NDK, it'd be nice if we could make B2G work the same way.

I still don't have a B2G tree available, but from browsing through codeaurora.org, it looks like only the sources for libc++/libstdc++ are there (even in the same directory hierarchy from the NDK), but not the binaries from the NDK.  Michael, do you know why those .a/.so files have been stripped?

Would it be feasible to have those .a/.so files put back, so we can use them in the B2G build?

Would it be feasible to have those .a/.so files put in a different repository on codeaurora.org, which we can then add to the B2G manifests?

Would it be feasiable to host those .a/.so files in our own repository on git.mozilla.org, which we can then add to the B2G manifests?
Flags: needinfo?(mwu)
Or, if you don't know the answers to those questions, do you know who would?
I think you're looking at this wrong.

When B2G is built, *all* of the OS is built. There exists a partial NDK and some binaries, but in general, everything is built from source. If it's Apache/MIT/BSD licensed, it's built from source. If you look at the full Android source checkout, they even build LLVM/Clang. (we remove that to save an hour of build time)

There is no putting .a/.so files back. They were never there to begin with.

Distributing binaries is generally not an option. SoC vendors expect access to source. In general, you want to use what's available out of a normal Android source repo as much as possible.
Flags: needinfo?(mwu)
This is just a tiny cleanup; these things will be going away or getting changed
in subsequent patches, but I think it's worth fixing them now.
Attachment #8613033 - Flags: review?(mh+mozilla)
This is somewhat of a hack; I'm not sure of the right way to accomplish this.

The problem is that if we configure --with-android-ndk=/some/absolute/path and
then want to refer to certain directories inside the NDK in LOCAL_INCLUDES, we
have no way of doing so, since everything in LOCAL_INCLUDES is assumed to be
srcdir- or topsrcdir-relative.  To deal with this, we declare that paths
starting with '//' should be interpreted as absolute paths.

This patch needs cleanup if it's going to go in, including documentation, maybe
some test cases, and possibly porting LOCAL_INCLUDES to use the new path
machinery you recently added (?).  Nevertheless, worth getting some feedback
and/or having alternate suggestions.
Attachment #8613035 - Flags: feedback?(mh+mozilla)
HAVE_LOG2 is never set by the build system, so we were always including
it.  Having this definition causes multiple definition errors on Android
when we compile a log2 implementation as part of the STL.

This patch is required to make things link without errors on my local Fennec
build.  No idea whether it's safe to remove this function entirely on all
platforms.
This is the most controversial part.

I elected to build things straight from the NDK, rather than importing yet
another third party library.  This way, we can rely on the NDK to make things
buildable, rather than maintaining patches ourselves, constantly importing new
versions, etc. etc.

My emulator B2G tree just finished downloading, though, and I sadly see that
its NDK--and possibly the NDKs for other emulator builds?--don't include libc++
sources.  So maybe we will have to import the NDK after all...

I think the moz.build files can survive more-or-less unchanged with in-tree
source files, though, so I think it's still worth getting feedback on those.
The only real thing that should change are the assignments to SOURCES.

My local Fennec build compiles and links with this (haven't tried running it),
so it's at least not totally broken.  It's possible I missed setting some
DEFINES or similar that configuring libc++ would have taken care of; I'll have
to look through that.  I'm sure getting all the Android and B2G platforms to be
happy with this will be a ton of fun, too.
Attachment #8613039 - Flags: feedback?(mh+mozilla)
Comment on attachment 8613036 [details] [diff] [review]
part 5 - remove definition of log2 from jsmath.cpp

HAVE_LOG2 is set by AC_CHECK_FUNCS([log2 log1p expm1 sqrt1pm1 acosh asinh atanh trunc cbrt])
(In reply to Mike Hommey [:glandium] from comment #22)
> HAVE_LOG2 is set by AC_CHECK_FUNCS([log2 log1p expm1 sqrt1pm1 acosh asinh
> atanh trunc cbrt])

I swear I knew this. ;)
Comment on attachment 8613033 [details] [diff] [review]
part 3 - make more things depend on CONFIG['MOZ_ANDROID_CXX_STL']

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

::: toolkit/content/moz.build
@@ +19,1 @@
>      DEFINES['USE_STLPORT'] = True

license.html will need an update for the other cases.
Attachment #8613033 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8613035 [details] [diff] [review]
part 4 - permit absolute paths in LOCAL_INCLUDES with a special sigil

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

::: python/mozbuild/mozbuild/backend/recursivemake.py
@@ +1165,5 @@
>  
>      def _process_local_include(self, local_include, backend_file):
> +        if local_include.startswith('//'):
> +            path = ''
> +        elif local_include.startswith('/'):

I think the right thing to do here would be add another kind of Path class in context.py to handle this situation, and switch LOCAL_INCLUDES to use it. We can also remove GENERATED_INCLUDES and use '!'-starting paths instead.

That being said, I'm not sure // is the right thing to use for absolute paths, because it's not clear how it's supposed to work with Windows paths. How about using %? (closest to a slash, and different enough from an exclamation mark)
Attachment #8613035 - Flags: feedback?(mh+mozilla)
Comment on attachment 8613039 [details] [diff] [review]
part 6 - support building libc++ from the Android NDK

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

I'd rather avoid doing this if we can. I don't see a compelling reason why this shouldn't be made part of the gonk build.
Attachment #8613039 - Flags: feedback?(mh+mozilla)
AFAICT, we're already pulling the ndk source as part of gonk, so it should be about ensuring we pull the right version for old android release based gonk, and that we build the right bits of it when building gonk. Although, I think that by default, everything that's present is built, so maybe we're already building the ndk.
(In reply to Mike Hommey [:glandium] from comment #27)
> AFAICT, we're already pulling the ndk source as part of gonk, so it should
> be about ensuring we pull the right version for old android release based
> gonk, and that we build the right bits of it when building gonk. Although, I
> think that by default, everything that's present is built, so maybe we're
> already building the ndk.

Assuming I understand B2G correctly, we'd need to upgrade the version (branch?) of the NDK that we use for ICS and JB, and maybe the device build for Aries (?).  How daunting of a task is that, and who do I have to try to convince that this is worth it?
Flags: needinfo?(mwu)
Much of the NDK involves GPL parts, and they aren't built from source.

ICS should be considered separately from everything newer. No vendor is looking at ICS based builds for new devices or devices they might want to update. All the ICS based devices that matter are under our control.

You might want to look at the work done in upgrading GCC on ICS for some hints on what's involved there - https://bugzilla.mozilla.org/show_bug.cgi?id=1056337

For everything afterwards, 4.4 is the main supported version. 5.1 is also being worked on. There are devices based on 4.3 and 4.2, but our community might be the only ones who care about those.

5.0 was the first version that started shipping libcxx https://android.googlesource.com/platform/external/libcxx/ . So start with 5.1 support. You might need to add libcxx to the manifest. Then backport to 4.4. And then try to figure out what to do with ICS.

Clang is available on 5.1 and 4.4 if you want to switch to clang. We disabled it, but it can be reenabled if it helps you.
Flags: needinfo?(mwu)
You should not think of the Android NDK when you look at B2G/Gonk. It is a very different platform. The only real similarity that you can rely on is they both use bionic.
Blocks: 864843
Repurposing this bug for the Fennec work, since Mike has indicated his interest in landing just that part.  The B2G work will have to take place in a different bug.
Summary: make Fennec/B2G buildable with something other than stlport → make Fennec buildable with something other than stlport
Attachment #8613035 - Attachment is obsolete: true
Attachment #8613036 - Attachment is obsolete: true
Attachment #8613039 - Attachment is obsolete: true
Gerv, we want to switch from STLport to libc++ as the C++ standard library on Fennec.  libc++'s licensing terms are listed here:

http://llvm.org/svn/llvm-project/libcxx/trunk/LICENSE.TXT

Do we need to add one of those blocks of text to /toolkit/content/license.html, or can we get by with one of the existing licenses?  And if we do add a separate block of text to license.html, I assume we'll also want it conditioned on whether we're using libc++, correct?
Flags: needinfo?(gerv)
Additional data point: we wouldn't be building libc++ ourselves, just linking against the one provided in the Android NDK.
Apparently either I got something wrong when I was building things or something in the tree changed out from underneath me, because compiling with libc++ throws weird errors in the protobuf code with a more recent m-c checkout.  I think this is gcc's inability to deal correctly with inline namespaces, even though it looks like libc++ makes allowances for that (?).

We could use libstdc++ instead:

1) just ship the .so and use that (assuming that's OK under the LGPL); or
2) link it in with the rest of the LGPL'd code that we now have a place for.

Or we could change Fennec to require clang.  I'm not sure which is easier.  glandium, do you have an opinion?
Flags: needinfo?(mh+mozilla)
It's also possible I was using the results from compiling our own libc++ (which might contain fixes that aren't in the NDK?) instead of linking to the one in the NDK.  I'll have to run some more experiments to confirm.
> 1) just ship the .so and use that (assuming that's OK under the LGPL); or

it's too big, iirc.

> 2) link it in with the rest of the LGPL'd code that we now have a place for.

likely has the same problem.
Flags: needinfo?(mh+mozilla)
(In reply to Nathan Froyd [:froydnj] [away 10 July through 19 July] from comment #32)
> Gerv, we want to switch from STLport to libc++ as the C++ standard library
> on Fennec.  libc++'s licensing terms are listed here:
> 
> http://llvm.org/svn/llvm-project/libcxx/trunk/LICENSE.TXT

Let's use the MIT license - easier.

But are we shipping this code or using a copy of the library already present on Android?

> Do we need to add one of those blocks of text to
> /toolkit/content/license.html, 

Hmm. I've never really pondered how that file applies to Fennec! It seems to show it... If we are shipping the libc++ code, then we should add the MIT license from the above URL to about:license, ideally conditioned on whether we are on Fennec and whether we are using libc++.

Gerv
Flags: needinfo?(gerv)
(In reply to Mike Hommey [:glandium] from comment #36)
> > 1) just ship the .so and use that (assuming that's OK under the LGPL); or
> 
> it's too big, iirc.
> 
> > 2) link it in with the rest of the LGPL'd code that we now have a place for.
> 
> likely has the same problem.

I don't understand either of these objections.  If we're OK with statically linking libc++, surely we'd be OK with statically linking libstdc++.  libc++ even looks fatter (at least in .so text size, according to size(1)) than libstdc++:

 927166	  16376	   6932	 950474	  e80ca	/home/froydnj/android-ndk-r10d/sources/cxx-stl/llvm-libc++/libs/armeabi-v7a/thumb/libc++_shared.so
 628049	  14364	  26497	 668910	  a34ee	/home/froydnj/android-ndk-r10d/sources/cxx-stl/gnu-libstdc++/4.6/libs/armeabi-v7a/thumb/libgnustl_shared.so
 644500	  14628	  26912	 686040	  a77d8	/home/froydnj/android-ndk-r10d/sources/cxx-stl/gnu-libstdc++/4.8/libs/armeabi-v7a/thumb/libgnustl_shared.so
 695166	  15104	  26900	 737170	  b3f92	/home/froydnj/android-ndk-r10d/sources/cxx-stl/gnu-libstdc++/4.9/libs/armeabi-v7a/thumb/libgnustl_shared.so

Presumably we'd ship the .a, statically linked into whatever, so the linker can strip out dead code, too, which probably makes any size differences a wash as well?
Flags: needinfo?(mh+mozilla)
(In reply to Gervase Markham [:gerv] from comment #37)
> Let's use the MIT license - easier.

OK, great.

> But are we shipping this code or using a copy of the library already present
> on Android?

We would be shipping the precompiled library that comes with the NDK.  I don't think we can use the version on Android because it might not be there (older phones) and/or might not have symbols we need.

Does that make a difference?

> > Do we need to add one of those blocks of text to
> > /toolkit/content/license.html, 
> 
> Hmm. I've never really pondered how that file applies to Fennec! It seems to
> show it... If we are shipping the libc++ code, then we should add the MIT
> license from the above URL to about:license, ideally conditioned on whether
> we are on Fennec and whether we are using libc++.

OK, great.  What about shipping libstdc++ in the same fashion as the proposed libc++ (statically linking in the binary that comes in the NDK)?  I ask because libc++ may present some insurmountable problems (see comment 34).  I see that libstdc++ is covered by the GPL, not the LGPL, though it has the runtime library exception:

https://gcc.gnu.org/onlinedocs/gcc-5.2.0/libstdc++/manual/manual/license.html#manual.intro.status.license.gpl

Are we able to link a static copy of libstdc++ into liblgpllibs.so or into libxul itself?  Or is libstdc++ a non-starter from a licensing perspective?
Flags: needinfo?(gerv)
Depends on: 1186561
If you link statically to liblgpllibs and rely on libc++/libstdc++ being there, you'll just end up with the same thing as using the .so from the NDK, except it'll be merged in liblgpllibs. Linking statically to each binary (like what -static-libstdc++ does) would help dead code elimination, but /can/ also have a worse effect on size. That could be made better with hidden visibility, but then there's the problem of global state used by the STL code, which needs to be shared between all statically linked parts.

Now, what amount of the libmozglue.so size is to be attributed to stlport? How much does libgabi++ (required for icu, bug 864843 has patches) add to that? libc++/libstdc++ would replace both. Without those numbers, it's also hard to tell to what degree the libc++/libstdc++ sizes are bad.
Flags: needinfo?(mh+mozilla)
(In reply to Nathan Froyd [:froydnj][:nfroyd] from comment #39) 
> Are we able to link a static copy of libstdc++ into liblgpllibs.so or into
> libxul itself?  Or is libstdc++ a non-starter from a licensing perspective?

My understanding is that the position is that GPLv3 imposes conditions (such as anti-Tivoization) that make partners nervous. Firefox currently contains no GPLv3 code. GCC's libstdc++ is GPLv3, with a linking exception. The linking exception makes it legal to use, but it would still come with those other obligations. So I'd say you need to try really hard to make other options work before we explore this further.

Also, on a non-licensing point, aren't we trying to shrink or at least hold down the size of the Fennec APK? How does shipping an entire C library help with that?

Gerv
Flags: needinfo?(gerv)
(In reply to Gervase Markham [:gerv] from comment #41)
> (In reply to Nathan Froyd [:froydnj][:nfroyd] from comment #39) 
> > Are we able to link a static copy of libstdc++ into liblgpllibs.so or into
> > libxul itself?  Or is libstdc++ a non-starter from a licensing perspective?
> 
> My understanding is that the position is that GPLv3 imposes conditions (such
> as anti-Tivoization) that make partners nervous. Firefox currently contains
> no GPLv3 code. GCC's libstdc++ is GPLv3, with a linking exception. The
> linking exception makes it legal to use, but it would still come with those
> other obligations. So I'd say you need to try really hard to make other
> options work before we explore this further.

OK, thanks for the explanation; that makes sense.

> Also, on a non-licensing point, aren't we trying to shrink or at least hold
> down the size of the Fennec APK? How does shipping an entire C library help
> with that?

We already more-or-less do that by shipping stlport on Fennec and B2G.  The question then becomes how much space is added by switching between stlport and libc++.

Numbers for libxul size, taken with size(1):

          text	   data	    bss	    dec	    hex	filename
stlport   30032096	2668156	 581196	33281448	1fbd5a8	toolkit/library/libxul.so
libc++    30160670	2671952	 583052	33415674	1fde1fa	toolkit/library/libxul.so
libstdc++ 29940022	2668780	 584836	33193638	1fa7ea6	toolkit/library/libxul.so

So libc++ adds ~130K to libxul over stlport, while libstdc++ *drops* ~90k.  I ran the numbers for stlport + gabi++, as we're going to add that in bug 864843, but the sizes are negligible: around 2k of text for the .o files.

I'm a little surprised by how heavyweight libc++ and how lightweight libstdc++ is.  I'm not entirely sure where the size increase is coming from...
It seems impossible. Are you sure it's not linked dynamically?
(In reply to Mike Hommey [:glandium] from comment #43)
> It seems impossible. Are you sure it's not linked dynamically?

Well, libstdc++ appears in the DT_NEEDED of libxul.so, but it does that in a normal build, too...

readelf -sW libxul.so | grep _ZNSt shows that all the std:: symbols are indeed linked locally (except for a few things related to exceptions, which I suppose requires something else from the NDK?)

readelf -sW libxul.so | grep _ZNSt | wc -l for libc++ is ~2400
readelf -sW libxul.so | grep _ZNSt | wc -l for libstdc++ is ~1600

That's not a great metric, as symbols get duplicated in the output, but I do notice that things like std::to_{w,}string are dragged in with libc++, whereas they're not with libstdc++...
Nathan, are we still blocked here? We need bug 864843 for b2gdroid so that would be great to land this.
Flags: needinfo?(nfroyd)
(In reply to [:fabrice] Fabrice Desré from comment #45)
> Nathan, are we still blocked here? We need bug 864843 for b2gdroid so that
> would be great to land this.

We are currently blocked because the libc++ distributed with the NDK we use (r10e) has a broken unwinder, which causes numerous test failures.

Conversations with glandium indicates that he doesn't want to import (a fixed version) of libc++ into our tree, so we have to build custom toolchain packages with a fixed libc++ to be able to land this.
Flags: needinfo?(nfroyd)
(In reply to Nathan Froyd [:froydnj] (gone through 2 September) from comment #46)

> Conversations with glandium indicates that he doesn't want to import (a
> fixed version) of libc++ into our tree, so we have to build custom toolchain
> packages with a fixed libc++ to be able to land this.

ok. Is that custom toolchain work tracked somewhere?
(In reply to [:fabrice] Fabrice Desré from comment #47)
> (In reply to Nathan Froyd [:froydnj] (gone through 2 September) from comment
> #46)
> 
> > Conversations with glandium indicates that he doesn't want to import (a
> > fixed version) of libc++ into our tree, so we have to build custom toolchain
> > packages with a fixed libc++ to be able to land this.
> 
> ok. Is that custom toolchain work tracked somewhere?

No.  Feel free to file a bug or even pick it up yourself. ;)
Depends on: 1201310
Assignee: nobody → nfroyd
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.