1176787 - MOZ_GLUE_IN_PROGRAM breaks static linking of SM into executables

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Michael Wu [:mwu]]

Servo normally statically links SM into the servo executable, but MOZ_GLUE_IN_PROGRAM breaks this. We have a hack that disables MOZ_GLUE_IN_PROGRAM, but it might make sense to disable it when building SM standalone or add some sort of option for disabling it.

Comment 1

[Till Schneidereit [:till]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=abab858f3420

Comment 2

[Till Schneidereit [:till]]

Attachment: Disable MOZ_GLUE_IN_PROGRAM in stand-alone builds on all platforms

AFAICT, this should fix the breakage. I don't have a Linux build environment set up right now, though, so I can't test.

Comment 3

[Michael Wu [:mwu]]

Comment on attachment 8668374 [details] [diff] [review]
Disable MOZ_GLUE_IN_PROGRAM in stand-alone builds on all platforms

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

This assumes that there are no embedders who want to stick mozglue in their executable and have SM in a separate DSO. Seems reasonable enough.

Comment 4

[Mike Hommey [:glandium]]

> Servo normally statically links SM into the servo executable, but MOZ_GLUE_IN_PROGRAM breaks this

Can you elaborate?

Comment 5

[Mike Hommey [:glandium]]

Comment on attachment 8668374 [details] [diff] [review]
Disable MOZ_GLUE_IN_PROGRAM in stand-alone builds on all platforms

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

All this patch will manage to do is to remove jemalloc from linux builds and possibly break linking the js shell if it ever uses functions from the MFBT .cpp files.

Comment 6

[Michael Wu [:mwu]]

(In reply to Mike Hommey [:glandium] from comment #4)
> > Servo normally statically links SM into the servo executable, but MOZ_GLUE_IN_PROGRAM breaks this
> 
> Can you elaborate?

HashBytes from HashFunctions.cpp is marked as weak. When servo runs, it can't find HashBytes and crashes. This doesn't happen when HashBytes isn't marked as weak.

Comment 7

[Till Schneidereit [:till]]

Unassigning myself: my linker-foo is way too weak.

Comment 8

[Anthony Ramine [:nox]]

I confirm that this is still a problem for my Rust bindings.

Comment 9

[(no longer active)]

Brad, this bug fixes the crash on Spidernode that you had seen.  Can you please see if you can pull the necessary managerial strings for a build system peer to drive this to the finish line?  Thanks!

Comment 10

[(no longer active)]

(In reply to :Ehsan Akhgari (busy, don't ask for review please) from comment #9)
> Brad, this bug fixes the crash on Spidernode that you had seen.  Can you
> please see if you can pull the necessary managerial strings for a build
> system peer to drive this to the finish line?  Thanks!

Looks like Blassey is on PTO for two weeks.  Andrew, any chance you can help with this, please?

Comment 11

[Andrew Overholt [:overholt]]

Jonathan told me he'd be able to find someone on his team.

Comment 12

[Mike Hommey [:glandium]]

Okay, so I have an angle to attack the whole problem from, but I need some clarifications as to which embedder needs what.
- Servo wants static mfbt/mozglue. What does it want for nspr?
- I presume Spidernode wants static mfbt/mozglue. What does it want for nspr?

Comment 13

[(no longer active)]

SpiderNode currently uses static mozglue on Linux but dynamic mozglue on OSX.  It would be nice if we could link to the same version on both platforms, but it's not the end of the world if that's not easy to do.  (I haven't tested the port to Windows at all yet.)

As far as NSPR goes, it seems like the parts of it that SpiderMonkey uses end up in libjs_static.a and we just link against that (we don't link against NSPR specifically.)

Comment 14

[Mike Hommey [:glandium]]

(In reply to :Ehsan Akhgari (busy, don't ask for review please) from comment #13)
> SpiderNode currently uses static mozglue on Linux but dynamic mozglue on
> OSX.  It would be nice if we could link to the same version on both
> platforms, but it's not the end of the world if that's not easy to do.  (I
> haven't tested the port to Windows at all yet.)
> 
> As far as NSPR goes, it seems like the parts of it that SpiderMonkey uses
> end up in libjs_static.a and we just link against that (we don't link
> against NSPR specifically.)

How do you link against libjs_static.a? So you use the Mozilla build system to link a program against it, or are you linking, like Servo, from an external build system?

Comment 15

[(no longer active)]

(In reply to Mike Hommey [:glandium] from comment #14)
> (In reply to :Ehsan Akhgari (busy, don't ask for review please) from comment
> #13)
> > SpiderNode currently uses static mozglue on Linux but dynamic mozglue on
> > OSX.  It would be nice if we could link to the same version on both
> > platforms, but it's not the end of the world if that's not easy to do.  (I
> > haven't tested the port to Windows at all yet.)
> > 
> > As far as NSPR goes, it seems like the parts of it that SpiderMonkey uses
> > end up in libjs_static.a and we just link against that (we don't link
> > against NSPR specifically.)
> 
> How do you link against libjs_static.a? So you use the Mozilla build system
> to link a program against it, or are you linking, like Servo, from an
> external build system?

From an external build system.

See <https://github.com/mozilla/spidernode/blob/75558b4b1852a4a80d2fd0b894e911874493c687/deps/spidershim/spidershim.gyp#L50> and <https://github.com/mozilla/spidernode/blob/75558b4b1852a4a80d2fd0b894e911874493c687/deps/spidershim/spidermonkey.gyp#L24>.

Comment 16

[(no longer active)]

nox mentioned on IRC that servo can get away with only linking against libjs_static.a and not against mozglue at all because of https://github.com/servo/mozjs/commit/e4385a1570884b8b7f71561428ef3bba7f1f90d8.  If this were possible in spidernode, I'd switch to using it too!

Mike, is this something that we can do here?

Comment 18

[Anthony Ramine [:nox]]

An ETA for this?

Comment 19

[Anthony Ramine [:nox]]

Oops, cleared NI. I don't understand what you mean by "- Servo wants static mfbt/mozglue. What does it want for nspr?". I don't want any of this, I just want libjs_static and to link against that and for it to work.

Comment 20

[Mike Hommey [:glandium]]

(In reply to nox from comment #19)
> Oops, cleared NI. I don't understand what you mean by "- Servo wants static
> mfbt/mozglue. What does it want for nspr?". I don't want any of this, I just
> want libjs_static and to link against that and for it to work.

For that to work, you need nspr, at least on Windows (the default on Linux and OSX is to use the posix nspr emulation, which doesn't require nspr). So the question remains: do you want nspr as a shared library in that case? Presumably, you do, if you are using NSS for SSL in servo (or maybe are you still on openssl? If you are, maybe you'd use nss at some point in the future? or maybe you use something else that requires nspr? if you do, you surely don't want two copies of nspr in the same address space).

Comment 21

[Anthony Ramine [:nox]]

I just want a static library to link against to use SpiderMonkey in Servo. Why can't nspr be in libjs_static exactly? I just want to land my SpiderMonkey update now, not worry about NSS or whatever for "some point in the future". :'(

Comment 22

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Feel free to assume we're not going to ever get another dependency on nspr.

Comment 23

[Steve Fink [:sfink] [:s:]]

Servo probably doesn't care, but at the moment nspr is still necessary even on Linux if --enable-ctypes is used, and I believe we do have embedders using that. (Heck, I use it, but my embedding is the standard test JS shell binary compiled with --enable-ctypes, and perhaps that's easier to accommodate for some reason?)

I suspect we'll want to remove that source of nspr dependency at some point, but we haven't done the work yet.

Comment 24

[(no longer active)]

If it's possible at all for embedders to *always* have to link against libjs_static.a and just pull in everything that SpiderMonkey needs in that static library, I think that will save embedders a lot of effort in terms of figuring out when they should use what library on what platform, etc.

With my m-c hat off for a sec, I wouldn't even know what NSPR is, so how am I supposed to know that I should link to it on some platforms and/or with some options passed to configure?!

Comment 25

[Mike Hommey [:glandium]]

Status update: this is taking time because I'm in that unstable state where I can have things working for servo and spidernode, or I can have things working for gecko, or I can have things working for normal standalone js builds but not all at the same time. I'll get to the bottom of it, but building spidermonkey these days takes some time, and this also needs to work on all platforms... so, please be patient.

Comment 26

[(no longer active)]

No problem, thanks for your help here!  Much appreciated.  :-)

Comment 27

[(no longer active)]

Mike, is there any recent progress on this?  We're going to be blocked on this bug fairly soon.

Comment 29

[Philip Chimento [:ptomato]]

Attachment: Disable MOZ_GLUE_IN_PROGRAM in stand-alone builds on all platforms

I ported the existing patch forward to ESR52. This definitely works for my embedding use case (using --enable-posix-nspr-emulation), but I assume it's still r- because of not working on other platforms?

Comment 30

[Chih-Hsuan Yen [:yan12125] (UTC+8)]

Attachment: Adding linker flags for mozglue in js.pc

Here I'd like to propose an alternative approach - adding appropriate linker flags to js.pc as described in https://bugzilla.mozilla.org/show_bug.cgi?id=1236085#c4. This patch is tested with gjs, which requires changes in its build scripts. [1]

Note that this is only a proof-of-concept. First, it may not work outside Linux as it hard-codes libmozglue.a. Second, this patch targets esr52 branch instead of hg-default. I'll update it if this sounds a good way on fixing crashes.

[1] https://github.com/yan12125/gjs/commit/d2b083cf3ee74952802f3d8457ee7e24173ba303

Comment 31

[Philip Chimento [:ptomato]]

That patch makes things that don't use JSAPI directly have to link against SpiderMonkey, I don't think it's the way to go.

Comment 32

[Mike Hommey [:glandium]]

Bug 1277338 kinda sorta solved the problem... if you build with --disable-jemalloc.

Comment 34

[Philip Chimento [:ptomato]]

Attachment: Always link mozglue into the shared library when building standalone

Regardless of which platform we are building on.

Comment 35

[Philip Chimento [:ptomato]]

This patch fixes things for me on mozjs-59a1, with --disable-jemalloc. A different fix would still be needed for ESR52.

Comment 36

[Mike Hommey [:glandium]]

Comment on attachment 8942756 [details] [diff] [review]
Always link mozglue into the shared library when building standalone

This might work with --disable-jemalloc, but it also explicitly breaks Windows without it.

Comment 37

[Philip Chimento [:ptomato]]

So my assumption that JS_STANDALONE is not used within Firefox is incorrect?

What would be the correct assumption? The combination of JS_STANDALONE plus --disable-jemalloc is not used within Firefox? I had hoped to write a patch that would only affect builds for SpiderMonkey embedders.

Comment 38

[Mike Hommey [:glandium]]

JS_STANDALONE is used without --disable-jemalloc.

Comment 39

[Philip Chimento [:ptomato]]

Attachment: Always link mozglue into the shared library when building standalone

Regardless of which platform we are building on.

Comment 40

[Philip Chimento [:ptomato]]

This one seems to build OK on Windows: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ac5bac1e046a7cc58ce60a686e730acf74a7f978

Comment 41

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b58c4adf52cf
Always link mozglue into the shared library when building standalone. r=glandium

Comment 42

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/b58c4adf52cf