850576 - Some (all?) STL allocations use libc's malloc instead of jemalloc

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Mike Hommey [:glandium]]

I found out about this when I noticed there should be a dead-lock in pthread_atfork from BionicGlue.cpp, but that it doesn't happen. Breaking in __node_alloc_impl::_S_chunk_alloc in external/stlport/src/allocators.cpp and tracing code execution shows that calls to operator new within libstlport end up in libstdc++'s operator new, which, in turn, dispatches to libc.so's malloc.

And in fact, that's totally understandable: libmozglue's dependencies include libstlport and libstdc++, which means the linker can't resolve libstlport's operator new relocations with libmozglue symbol, since libmozglue is not linked yet when it does so. I think the only way out of this would be to statically link stlport in libmozglue.

Comment 1

[Mike Hommey [:glandium]]

Note this also means there are chances of allocator mismatch between operator new calls that are inlined and those from libstlport.so itself.

Comment 2

[Mike Hommey [:glandium]]

As bug 861973 showed, more than statically linking stlport, we'd also need to build it. For the android port, we're using the stlport source available in the NDK. There is no such thing for b2g, isn't it?

Comment 3

[Mike Hommey [:glandium]]

Attachment: Build STLport ourselves for b2g

Testing this locally.

Comment 4

[Mike Hommey [:glandium]]

(In reply to Mike Hommey [:glandium] from comment #3)
> Created attachment 737851 [details] [diff] [review]
> Build STLport ourselves for b2g
> 
> Testing this locally.

And this doesn't work. The Android counterpart relies on the fact that there is a stlport static build in the NDK tree to fallback to until we build stlport. That can't really work for B2G, except if we're allowed to use one from under prebuilt/ndk/android-ndk-r*, which would also require choosing one of the three existing (and making sure it stays there).

Thoughts ?

Comment 5

[Justin Lebar (not reading bugmail)]

Do we need to block b2g on this?  I guess this is only problematic if we mismatch allocators (I think we shouldn't) or call malloc_usable_size on an stl allocation (this we probably do).

Comment 6

[Mike Hommey [:glandium]]

(In reply to Justin Lebar [:jlebar] from comment #5)
> or call malloc_usable_size on an stl allocation (this we probably do).

Telemetry does.

Comment 7

[Justin Lebar (not reading bugmail)]

> Telemetry does.

The telemetry memory reporter does, but we make an effort not to run most memory reporters during telemetry pings.  When we measure explicit, we do by running only the non-heap single reporters and calling GetExplicitNonHeap on the multi-reporters.  I don't think either of those calls into the telemetry reporter.

Yoric and dietrich are telling me on IRC that they think we don't run telemetry on B2G.  So maybe we get off easy on this one.

Comment 8

[Mike Hommey [:glandium]]

(In reply to Justin Lebar [:jlebar] from comment #7)
> > Telemetry does.
> 
> The telemetry memory reporter does, but we make an effort not to run most
> memory reporters during telemetry pings.

This has sadly nothing to do with telemetry pings.

> When we measure explicit, we do by running only the non-heap single reporters and calling GetExplicitNonHeap on
> the multi-reporters.  I don't think either of those calls into the telemetry
> reporter.

You never get full memory profiles?

> Yoric and dietrich are telling me on IRC that they think we don't run
> telemetry on B2G.  So maybe we get off easy on this one.

AFAIK, the telemetry code is always on.

Comment 9

[Justin Lebar (not reading bugmail)]

> You never get full memory profiles?

Not in production...

Comment 10

[Ben Turner (not reading bugmail, use the needinfo flag!)]

By the way, does this STL library use C++ exceptions (like most do)? We don't have any support for that in gecko and we rely on the STL calling moz_abort() instead...

Comment 11

[Mike Hommey [:glandium]]

Note the hack we use on android does disable exceptions in the stlport used for gecko. The patch here uses the same hack, it just needs some more fiddling to work with gonk.

Comment 12

[Ting-Yuan Huang]

To force stlport to use jemalloc, can we just override the symbols in it? More specifically,
objcopy --redefine-sym malloc=jemalloc libstlport.so
that makes all references[1] of malloc to jemalloc.

[1] and definitions, but fortunately stlport doesn't have it.

Comment 13

[Ting-Yuan Huang]

Oops, it seems that bionic isn't capable of handling circular dependencies (libstlport <-> libmozglue). Renaming symbols doesn't work in this case.

Comment 14

[Mike Hommey [:glandium]]

Attachment: Statically link stlport on b2g and android, and always use a custom built stlport for that

This should do it.

Comment 15

[Mike Hommey [:glandium]]

So, this patch works, but it fails to build on B2G Arm (but works on the other B2G builds) because it uses an old gonk that doesn't contain the stlport sources. So, either we import the stlport sources, or we wait for those builds to use a more proper gonk.
The former, actually, might be worth considering, as it would guarantee the stlport content we build, instead of relying on whatever comes with the ndk.

Comment 16

[Mike Hommey [:glandium]]

Ted, thoughts on comment 15?

Comment 17

[(not currently active) Ted Mielczarek]

Importing stlport into our tree seems like the most straightforward way to fix that.

Comment 18

[Mike Hommey [:glandium]]

Attachment: Statically link stlport on b2g and android, and always use a custom built stlport for that

This is mostly the same as the patch you already reviewed, but using the in-tree stlport from bug 879792.

Comment 19

[(not currently active) Ted Mielczarek]

Comment on attachment 774415 [details] [diff] [review]
Statically link stlport on b2g and android, and always use a custom built stlport for that

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

::: build/autoconf/android.m4
@@ +244,5 @@
>              else
>                  AC_MSG_ERROR([Couldn't find path to gnu-libstdc++ in the android ndk])
>              fi
> +        else
> +            STLPORT_CPPFLAGS="-I$_topsrcdir/build/stlport/stlport -I$android_ndk/sources/cxx-stl/system/include"

Why do we still have a reference to $android_ndk/sources/cxx-stl/system/include here?

Comment 20

[Mike Hommey [:glandium]]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #19)
> Why do we still have a reference to
> $android_ndk/sources/cxx-stl/system/include here?

That's where the system headers like <cstdio>, <cstdint>, etc., and <new> are. They are not part of stlport but still required for C++.

Comment 21

[Mike Hommey [:glandium]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/926141b660e5

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/926141b660e5

Comment 23

[Vicamo Yang [:vicamo][:vyang]]

When building Gecko with either arm or x86 ICS Gonk, I have warnings hidden in millions of lines:

  arm-linux-androideabi-g++: unrecognized option '-static-libstdc++'

or

  i686-android-linux-g++: unrecognized option '-static-libstdc++'

So we're actually always building Gecko with libstdc++ dynamically linked.  This can be verified by `readelf -d out/target/product/unagi/system/b2g/b2g | grep libstdc`.

But when it comes to build with JellyBean Gonk, the new toolchain seems to support it now and will try to locate libstdc++.a but in vain.  Several unreferenced symbol errors generated:

  arm-linux-androideabi/bin/ld: error: cannot find -lstdc++
  arm-linux-androideabi/bin/ld: objdir-gecko/mfbt/tests/TestWeakPtr.o: in function mozilla::detail::RefCounted<mozilla::detail::WeakReference<C, (mozilla::detail::RefCountAtomicity)1>, (mozilla::detail::RefCountAtomicity)1>::Release():../../dist/include/mozilla/RefPtr.h:81: error: undefined reference to 'operator delete(void*)'
  ...

Comment 24

[Vicamo Yang [:vicamo][:vyang]]

Re-produced with completely synced repo.  Tested so far: arm-ics (dyn-linked), arm-jb (build failed), x86-ics (dyn-linked), x86-jb (build failed).

Comment 25

[Mike Hommey [:glandium]]

With stlport statically linked into libmozglue, it should be using operator new/delete from libmozglue, which should solve most concerns for this bug. Looking more closely, there's nothing really exported from libstdc++.so on b2g besides operator new and operator delete so it seems fine to actually not statically link against it. And libmozglue is LD_PRELOADed, which means other libraries are going to use its operator new/delete. So all in all, I think this is fine for b2g. The build issues with --static-libstdc++ can be dealt with in bug 894538.

Comment 26

[Broton]

I have seen the change about b2g, but I don't know why libstdc++.a is linked to libxul.so for firefox android, could someone help me? 

code in build/autoconf/android.m4

STLPORT_CPPFLAGS="-isystem $_topsrcdir/build/stlport/stlport -isystem $android_ndk/sources/cxx-stl/system/include"          
STLPORT_LIBS="$_objdir/build/stlport/libstlport_static.a -static-libstdc++"

Comment 27

[Broton]

I have seen the change about b2g, but I don't know why libstdc++.a is linked to libxul.so for firefox android, could someone help me? 

code in build/autoconf/android.m4

STLPORT_CPPFLAGS="-isystem $_topsrcdir/build/stlport/stlport -isystem $android_ndk/sources/cxx-stl/system/include"          
STLPORT_LIBS="$_objdir/build/stlport/libstlport_static.a -static-libstdc++"

Comment 28

[Broton]

Anyone here?
I find libstdc++ will be moved to libc.so
https://github.com/android/platform_bionic/commit/15b641a26731a7e42455c3ed22e1e9bdf31ea79c
Firefox android will link static libc in the future ?