Closed Bug 850576 Opened 7 years ago Closed 6 years ago

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

Categories

(Firefox OS Graveyard :: General, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: glandium, Assigned: glandium)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
Note this also means there are chances of allocator mismatch between operator new calls that are inlined and those from libstlport.so itself.
Depends on: 850332
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?
Attached patch Build STLport ourselves for b2g (obsolete) — Splinter Review
Testing this locally.
Assignee: nobody → mh+mozilla
(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 ?
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).
(In reply to Justin Lebar [:jlebar] from comment #5)
> or call malloc_usable_size on an stl allocation (this we probably do).

Telemetry does.
> 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.
(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.
> You never get full memory profiles?

Not in production...
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...
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.
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.
Oops, it seems that bionic isn't capable of handling circular dependencies (libstlport <-> libmozglue). Renaming symbols doesn't work in this case.
Attachment #737851 - Attachment is obsolete: true
Attachment #756159 - Flags: review?(ted) → review+
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.
Ted, thoughts on comment 15?
Flags: needinfo?(ted)
Importing stlport into our tree seems like the most straightforward way to fix that.
Flags: needinfo?(ted)
Depends on: 879792
Depends on: 892401
This is mostly the same as the patch you already reviewed, but using the in-tree stlport from bug 879792.
Attachment #774415 - Flags: review?(ted)
Attachment #756159 - Attachment is obsolete: true
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?
Attachment #774415 - Flags: review?(ted) → review+
(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++.
https://hg.mozilla.org/mozilla-central/rev/926141b660e5
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
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*)'
  ...
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).
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Depends on: 894538
Resolution: --- → FIXED
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++"
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++"
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 ?
You need to log in before you can comment on or make changes to this bug.