539188 - Fix jemalloc linkage for Solaris builds

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Ginn Chen]

The problem was introduced since Bug 534848.
Firefox failed to build on Solaris for a certain situation, --enable-jemalloc --disable-libxul
It does't happen for --enable-libxul, which is default, because libxul.so depends on libmozjs.so, libmozjs.so links libjemalloc.a.
Currenly, on Solaris, libjemalloc.a is linked into libmozjs.so and firefox-bin.
I think it is not good.

Actually, libmozjs no longer uses posix_memalign on Solaris now, so it doesn't need to link with jemalloc.

To clean up the mess of jemalloc on Solaris.
I think the best solution is to link libjemalloc.a in libxpcom_core (if --disable-libxul) / libxul (if --enable-libxul) with some special flags.

In Bug 515354 comment #14, Benjamin Smedberg concerns link jemalloc into libxul would totally screw over all the embedders.
That's true if direct binding is used within the shared object.
Linking direct binding within shared object is off by default on Solaris, user may has "-B direct" in his LDFLAGS.
To avoid that from happening, we can use
1) -B nodirect, which would generate an error if "-B direct" is used.
2) use mapfile to disable direct binding for malloc functions.
I'd like take option 2), so that user can still have the benefits of "-B direct" for other functions.

Also we should use "-z interpose" for the shared object that has malloc functions. It can avoid some troubles.
And I just learned with this options we don't need to link libjemalloc into *-bin, we don't need to put libjemalloc ahead of other libraries while linking.

Another benefit of linking libjemalloc to libxpcom_core/libxul not browser-app or xulrunner-stub is we can switch to other memory allocator library by LD_PRELOAD at runtime.

IMO, it is a cleaner solution.

Comment 1

[Ginn Chen]

Attachment: patch

Comment 2

[Ginn Chen]

Comment on attachment 421227 [details] [diff] [review]
patch

Missed jemalloc_solaris.map in this patch.

Also I think we'd put jemalloc_solaris.map at $topsrc/config, because in js/src $topsrc/memory/jemalloc doesn't exist.

Comment 3

[Ginn Chen]

Attachment: patch

This also works fine with GRE_HOME situation, then malloc in jemalloc wouldn't be used by any library.

Comment 4

[Benjamin Smedberg]

Comment on attachment 423331 [details] [diff] [review]
patch

I really do not think we should support a different mechanism for Solaris than for Linux, but Ted's the owner and would have to bear the maintenance costs.

Comment 5

[Ginn Chen]

Either way, this is a bug we need to fix for Solaris. Move forward, or go back to what Linux does.

I would be more than thankful if this patch can be accepted, it would make my life much easier.

Comment 6

[(not currently active) Ted Mielczarek]

I'm not really comfortable reviewing this, since I don't understand linker mechanics quite that well.

Comment 7

[(not currently active) Ted Mielczarek]

Comment on attachment 423331 [details] [diff] [review]
patch

Yeah, I don't want this. If you have to, error in --disable-libxul --enable-jemalloc builds, that's fine.

Comment 8

[Ginn Chen]

The patch here is not just to fix "--disable-libxul --enable-jemalloc" buids.
The essential of the patch is to link libjemalloc.a into libxul.so/libxpcom_core.so with proper link map and flag.
Currently, libjemalloc.a is linked into both firefox-bin and libmozjs.so, it is very fragile.

Well, if you don't want that, I've no choice but back out all the tricks of jemalloc on Solaris, and disable jemalloc by default on Solaris.
Then there's no different of using jemalloc between Linux and Solaris.
BTW: I think Linux distro doesn't really use jemalloc in their ship of Firefox.

Of course, I have to revert Bug 543848 and cross my fingers waiting Bug 520422 be fixed and the fix is portable to Solaris.

Comment 9

[Ginn Chen]

Attachment: revert jemalloc fixes and revert Bug 534848

Comment 10

[Ginn Chen]

http://hg.mozilla.org/mozilla-central/rev/43ff69a3cd7e

Comment 11

[Ginn Chen]

Attachment: for comm-central

Comment 12

[Justin Wood (:Callek)]

Comment on attachment 443815 [details] [diff] [review]
for comm-central

r+ by simple compare vs what ted approved for m-c.

Comment 13

[Ginn Chen]

http://hg.mozilla.org/comm-central/rev/80c384b47532