Open Bug 1588340 Opened 6 years ago Updated 1 year ago

Spidermonkey does not build with --disable-shared-js

Categories

(Core :: JavaScript Engine, defect, P2)

Product:
Core
Component:
JavaScript Engine
Type:
defect

Tracking

()

Status:
ASSIGNED

People

(Reporter: sleik, Assigned: decoder)

References

Details

(Keywords: leave-open)

Attachments

(2 files, 1 obsolete file)

fix-static-builds-of-sm78-for-real.patch
719 bytes, patch
Details | Diff | Splinter Review
Bug 1588340 - Always define MOZ_HAS_MOZGLUE for JS. r?glandium
48 bytes, text/x-phabricator-request
Details | Review

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:70.0) Gecko/20100101 Firefox/70.0

Steps to reproduce:

When I tried to build Spidermonkey like so:
hg clone https://hg.mozilla.org/mozilla-central/
cd mozilla-central/js/src/
autoconf2.13
mkdir build_OPT.OBJ/
configure --disable-shared-js
make j

and got multiple building erors.

Actual results:

make[3]: *** [Unified_cpp_js_src17.o] Error 1
/out/mozilla-central/js/src/vm/StringType.cpp:57:16: error: no member named 'LossyConvertUtf16toLatin1' in namespace 'mozilla'
using mozilla::LossyConvertUtf16toLatin1;
      ~~~~~~~~~^
/out/mozilla-central/js/src/vm/StringType.cpp:179:20: error: no member named 'ConvertLatin1toUtf8Partial' in namespace 'mozilla'
          mozilla::ConvertLatin1toUtf8Partial(src, buffer);
          ~~~~~~~~~^
2 errors generated.
/out/mozilla-central/config/rules.mk:787: recipe for target 'Unified_cpp_js_src_debugger1.o' failed
make[3]: *** [Unified_cpp_js_src_debugger1.o] Error 1
make[3]: *** Waiting for unfinished jobs....
2 errors generated.
/out/mozilla-central/js/src/vm/StringType.cpp:234:20: error: no member named 'ConvertUtf16toUtf8Partial' in namespace 'mozilla'
          mozilla::ConvertUtf16toUtf8Partial(src, buffer);
          ~~~~~~~~~^
/out/mozilla-central/config/rules.mk:787: recipe for target 'Unified_cpp_js_src4.o' failed
make[3]: *** [Unified_cpp_js_src4.o] Error 1
2 errors generated.
/out/mozilla-central/config/rules.mk:787: recipe for target 'Interpreter.o' failed
make[3]: *** [Interpreter.o] Error 1
2 errors generated.
/out/mozilla-central/js/src/vm/StringType.cpp:591:5: error: use of undeclared identifier 'LossyConvertUtf16toLatin1'
    LossyConvertUtf16toLatin1(src, AsWritableChars(MakeSpan(dest, len)));
    ^
/out/mozilla-central/config/rules.mk:787: recipe for target 'Unified_cpp_js_src10.o' failed
make[3]: *** [Unified_cpp_js_src10.o] Error 1
2 errors generated.
/out/mozilla-central/config/rules.mk:787: recipe for target 'Unified_cpp_js_src_gc2.o' failed
make[3]: *** [Unified_cpp_js_src_gc2.o] Error 1
make[3]: Leaving directory '/out/mozilla-central/js/src/build_OPT.OBJ/js/src/gc'
/out/mozilla-central/config/recurse.mk:74: recipe for target 'js/src/gc/target-objects' failed
make[2]: *** [js/src/gc/target-objects] Error 2
make[2]: *** Waiting for unfinished jobs....
/out/mozilla-central/js/src/vm/StringType.cpp:962:3: error: use of undeclared identifier 'ConvertLatin1toUtf16'
  ConvertLatin1toUtf16(AsChars(MakeSpan(src, length)), MakeSpan(dest, length));
  ^
/out/mozilla-central/js/src/vm/StringType.cpp:1001:3: error: use of undeclared identifier 'LossyConvertUtf16toLatin1'
  LossyConvertUtf16toLatin1(MakeSpan(src, length),
  ^
2 errors generated.

Expected results:

It's unclear to me why this error occurs and especially why it only occurs when in "--disable-shared-js" mode. How do I get Spidermonkey to build correclty?

Bugbug thinks this bug should belong to this component, but please revert this change in case of error.

Component: Untriaged → JavaScript Engine
Product: Firefox → Core

:hsivonen, this looks related to the changes in bug 1490601. Can you take a look? Thanks!

Flags: needinfo?(hsivonen)

I mistook the dev-platform announcement of bug 1554056 as meaning that enable/disable-shared-js was no longer a thing. It looks like it was scoped to Firefox builds.

What is --disable-shared-js supposed to do when building standalone SpiderMonkey, why is what it does important, and does it work with --enable-cranelift?

Flags: needinfo?(hsivonen)

From my understanding, --disable-shared-js should compile SpiderMonkey ("libmozjs") as a static library instead of a shared one. For me, the build works neither with --disable-shared-js --enable-cranelift nor with --disable-shared-js --disable-cranelift

Did --disable-shared-js --enable-cranelift work before bug 1572364?

Considering that the error is a compiler error rather than a linker error, I take it that --disable-shared-js builds mfbt/ without defining MOZ_HAS_MOZGLUE. Was not defining MOZ_HAS_MOZGLUE technically correct previously? That is, is a libmozjs.a supposed to be linkable into an app that doesn't link mozglue?

I suppose we could mint a new #define to indicate that an app which doesn't link mozglue promises to have jsrust-shared somewhere in its Rust dependency graph (and promises to link a static library corresponding to that Rust dependency graph).

:hsivonen Just to clarify in case you are waiting on answer on those questions from me: Unfortunately, I am only a user of Spidermonkey, not a developer.

sfink, what's the intended relationship of mozglue and non-Firefox apps linking libmozjs.a? That is, is it correct or incorrect for the scenario here to have MOZ_HAS_MOZGLUE undefined?

(It's pretty clear that going forward apps that wish to link libmozjs.a also need to link jsrust.a or need to have their own "all Rust code goes here" .a that among other things depends on jsrust-shared.)

Flags: needinfo?(sphink)
See Also: → 1589769

I don't know the answer to these questions, unfortunately. All I can tell you is that spidermonkey does depend on mozglue symbols -- in particular, if you don't have mozglue available, you'll get missing mozglue symbols related to mozilla::TimeStamp.

mfbt/Latin1.h defines ConvertLatin1toUtf16 only if MOZ_HAS_JSRUST. js/src/util/Text.h uses ConvertLatin1toUtf16 unconditionally. I guess the JS engine now assumes MOZ_HAS_JSRUST?

Anyway, I suspect glandium is the only human on this planet who currently has any clue about this stuff.

Flags: needinfo?(sphink) → needinfo?(mh+mozilla)

(In reply to Steve Fink [:sfink] [:s:] from comment #8)

I don't know the answer to these questions, unfortunately. All I can tell you is that spidermonkey does depend on mozglue symbols -- in particular, if you don't have mozglue available, you'll get missing mozglue symbols related to mozilla::TimeStamp.

OK. Thanks. In that case, it seems the bug is that MOZ_HAS_MOZGLUE isn't defined in the situation reported here.

glandium, any ideas what mechanism should be defining MOZ_HAS_MOZGLUE but isn't doing so?

mfbt/Latin1.h defines ConvertLatin1toUtf16 only if MOZ_HAS_JSRUST. js/src/util/Text.h uses ConvertLatin1toUtf16 unconditionally. I guess the JS engine now assumes MOZ_HAS_JSRUST?

Yes, the engine depending on MOZ_HAS_JSRUST is expected.

(In reply to Henri Sivonen (:hsivonen) from comment #9)

(In reply to Steve Fink [:sfink] [:s:] from comment #8)

I don't know the answer to these questions, unfortunately. All I can tell you is that spidermonkey does depend on mozglue symbols -- in particular, if you don't have mozglue available, you'll get missing mozglue symbols related to mozilla::TimeStamp.

OK. Thanks. In that case, it seems the bug is that MOZ_HAS_MOZGLUE isn't defined in the situation reported here.

glandium, any ideas what mechanism should be defining MOZ_HAS_MOZGLUE but isn't doing so?

Part of it is https://searchfox.org/mozilla-central/source/js/src/build/moz.build#15 which says that Gecko builds get an automatic MOZ_HAS_MOZGLUE. But the question here is what happens with standalone builds. I wish I had a better understanding of the overall picture here. I suspect you (Henri) have a much better idea than I do at this point. I'm sure glandium has a comment somewhere with all this, but the pieces that come to mind are:

  • mozjs
  • libxul
  • jemalloc
  • mozglue
  • the rust shared libs
  • mfbt, since it seems to have some non-header stuff at the moment
  • ?

and the various ways that one of these could include one or more of the other, and how they avoid duplicate copies, etc. Do we have a single pile of rust objects? It seems like there should be separate ones corresponding to mozjs, mozglue, and libxul. Which then, I guess, would need to be separate only when necessary for the final target, so as to not inhibit optimization (including LTO inlining) in the common cases?

Mike, is this written down anywhere?

Do we have a single pile of rust objects? It seems like there should be separate ones corresponding to mozjs, mozglue, and libxul.

Anything that statically links SpiderMonkey needs to have jsrust-shared in its Cargo dependency graph. For libxul, this is accomplished by gkrust-shared depending on jsrust-shared and gkrust depending on gkrust-shared producing the libgkrust.a that satisfies the linker requirements. For the standalone SpiderMonkey as a shared library, jsrust depends on jsrust-shared and produces libjsrust.a that satisfies the linker requirements when linking the shared library.

A non-Firefox app that wants to statically link SpiderMonkey and has no other Rust code should be able to link libjsrust.a (but due to this bug doesn't get past the compilation phase). A non-Firefox app that wants to statically link SpiderMonkey and does have other Rust code should make its Cargo dependency graph that eventually produced some .a have jsrust-shared somewhere in the dependency graph.

There's no mozglue-shared Rust crate, because so far it has been the assumption that consumers that use mozglue but don't link SpiderMonkey don't want to link any Rust code. That is, jsrust-shared effectively has the role of the non-existent mozglue-shared.

Which then, I guess, would need to be separate only when necessary for the final target, so as to not inhibit optimization (including LTO inlining) in the common cases?

The LTO needs to happen when linking the .a representing all Rust code with the C++ .o files, so apps that want to link SpiderMonkey statically and want to benefit from cross-language LTO need to make their own final link step use LTO.

(In reply to Henri Sivonen (:hsivonen) from comment #11)

There's no mozglue-shared Rust crate, because so far it has been the assumption that consumers that use mozglue but don't link SpiderMonkey don't want to link any Rust code. That is, jsrust-shared effectively has the role of the non-existent mozglue-shared.

Specifically, both as a matter of footprint and as a matter of symbol collision, it would be a bad idea for the Firefox executable whose purpose is mainly to load libxul to get its own copy of the Rust code relevant to the bug at hand.

Priority: -- → P1

The rule is simple: anything that has access to mozglue functions somehow should have MOZ_HAS_MOZGLUE set. Access via either static or dynamic linking. Any kind of access. The build system is however not necessarily exhaustive about that, and removing the condition around LIBRARY_DEFINES['MOZ_HAS_MOZGLUE'] = True in js/src/build/moz.build should fix this issue.

However, this unveils the more general problem of static linking standalone js, which adding rust code to spidermonkey made harder, and that the recent addition of encoding-js made worse, because now you not only have to link the static js library, and the static js-rust library, but you now also have to link the static mozglue... which the build system actually doesn't even produce. So in fact static standalone js is now unusable. I'm not even sure how this should be dealt with...

Flags: needinfo?(mh+mozilla)

(In reply to Mike Hommey [:glandium] (high latency) from comment #13)

The rule is simple: anything that has access to mozglue functions somehow should have MOZ_HAS_MOZGLUE set. Access via either static or dynamic linking. Any kind of access. The build system is however not necessarily exhaustive about that, and removing the condition around LIBRARY_DEFINES['MOZ_HAS_MOZGLUE'] = True in js/src/build/moz.build should fix this issue.

Thanks.

However, this unveils the more general problem of static linking standalone js, which adding rust code to spidermonkey made harder, and that the recent addition of encoding-js made worse, because now you not only have to link the static js library, and the static js-rust library, but you now also have to link the static mozglue... which the build system actually doesn't even produce. So in fact static standalone js is now unusable. I'm not even sure how this should be dealt with...

How did mozilla::TimeStamp mentioned in comment 8 work in static-linkage standalone SpiderMonkey before the Rust changes?

Flags: needinfo?(mh+mozilla)

Well, I guess this means static-linkage Spidermonkey has been broken for longer than I thought.

Flags: needinfo?(mh+mozilla)

And now that I'm looking at the mozjs_sys crate for other reasons, I think it's broken too.

(In reply to Mike Hommey [:glandium] (high latency) from comment #16)

And now that I'm looking at the mozjs_sys crate for other reasons, I think it's broken too.

In what way? nox was looking at providing the Rust dependencies that SpiderMonkey depends on via the Cargo dependency graph of a larger Rust-using app.

I believe mozjs_sys has been relying on https://github.com/servo/mozjs/blob/master/etc/patches/mozglue-static-library.patch for a long time for successful static builds.

I am currently trying to update SpiderMonkey in Servo, and I'm not sure what you people are talking about, but in my local stuff I have the following patch:

--- js/src/build/moz.build
+++ js/src/build/moz.build
@@ -11,8 +11,7 @@ CONFIGURE_SUBST_FILES += [
 
 LIBRARY_DEFINES['EXPORT_JS_API'] = True
 
-if not CONFIG['JS_STANDALONE']:
-    LIBRARY_DEFINES['MOZ_HAS_MOZGLUE'] = True
+LIBRARY_DEFINES['MOZ_HAS_MOZGLUE'] = True
 
 # JavaScript must be built shared, even for static builds, as it is used by
 # other modules which are always built shared. Failure to do so results in

Having the same problem when configuring with JS_STANDALONE=1 ../configure --disable-tests --disable-js-shell --disable-shared-js in Windows trying to build a static and standalone SpiderMonkey library.

(In reply to Josh Matthews [:jdm] from comment #18)

I believe mozjs_sys has been relying on https://github.com/servo/mozjs/blob/master/etc/patches/mozglue-static-library.patch for a long time for successful static builds.

If I try that the following when configuring with JS_STANDALONE=1 ../configure --disable-tests --disable-js-shell --disable-shared-js

==============================
FATAL ERROR PROCESSING MOZBUILD FILE

The error occurred while processing the following file or one of the files it includes:

c:/Users/user/Documents/code/mozilla-central/js/src/build/moz.build

The error occurred when validating the result of the execution. The reported error is:

The static "js" library is not used in a shared library or a program, but USE_LIBS contains the following shared library names:
    mozglue

Maybe you can remove the static "js" library?
Priority: P1 → P2
Attached patch fix-static-builds-of-sm78.patch (obsolete) — — Splinter Review

(discussed on matrix today).
I confirm this bug happens on ESR78 and presumably master.
From what I can tell, the issue is that:

  1. Spidermonkey requires mozglue symbols
  2. In JS_STANDALONE builds, without MOZ_MEMORY, Mozglue is built as a static library (patch #1176787, which is very related). Note that I'm not sure what MOZ_MEMORY has to do with this.
  3. The 'js' library does not include mozglue in USE_LIBS (presumably because both mozglue and js get linked together in whatever target app it's building, i.e. Firefox?)
  4. Since mozglue is a static library, it may or may not be actually built. It isn't under SM68 and 78 from what I can tell.
    With all the above, you actually end up with the libjs_static.a not including mozglue symbols, but requiring them, and sometimes no way to even link mozglue separately since it isn't built (I'm not entirely sure why, tbh).

The patch I'm attaching adds USE_LIBS to js when in STANDALONE mode, which makes the 'js' lib actually include the mozglue symbols. This does still require mozglue to be built statically, so maybe a preferred fix would be to FINAL_LIBRARY('js') mozglue in that initial condition. Either works.

Relatedly, but on a separate note:
Spidermonkey does not set MOZ_HAS_MOZGLUE in JS_STANDALONE build (which is wrong according to glandium above - the OG patch is 1309573 - comment #37, still by glandium, suggests that the define being behind "not js_standalone" was wrong from the start and one occurence was missed in the review back then).

(sorry, attached the wrong patch -_-)

Attachment #9187603 - Attachment is obsolete: true

On secondary investigation, SDK_LIBRARY = True is also only set if CONFIG['JS_SHARED_LIBRARY']. I'm not sure that makes sense, but I'm fairly certain we at least want that true for JS_STANDALONE builds (probably in general).

Further issue on SM78: the rust library adds FINAL_LIBRARY = 'js' only in shared JS libraries. It should at least do so for JS_STANDALONE too.
The SDK_LIBRARY comment above appears outdated on SM78 too.

This is a blocker for building SpiderMonkey with Fuzzilli instrumentation. Currently, we locally just remove the condition in front of the LIBRARY_DEFINES['MOZ_HAS_MOZGLUE'] = True and that seems to work.

:glandium, is there any harm in this change (as in, making it more broken than it already is)? Is the second part in comment 23 required/advisable?

Flags: needinfo?(mh+mozilla)

The second half probably subtly breaks --enable-shared-js at runtime, so no, it's not advisable.

Flags: needinfo?(mh+mozilla)

Comment 13 still applies though from the perspective of third-parties relying on the standalone js library.

(In reply to Mike Hommey [:glandium] from comment #28)

Comment 13 still applies though from the perspective of third-parties relying on the standalone js library.

Okay, I'm going to go ahead and submit a patch for the first half of the patch which should be sufficient for us to build with --disable-shared-js for Fuzzilli and we should leave this bug open until we can fix this properly. I'll also leave a comment in there about the potentially broken static js library, pointing to this bug.

Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: leave-open
Version: 70 Branch → Trunk
Assignee: nobody → choller
Status: NEW → ASSIGNED
Blocks: 1689582

Bug 1589769 is already set as Blocked.

See Also: 1589769 →
Pushed by choller@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/4e8fe724a8f2 Always define MOZ_HAS_MOZGLUE for JS. r=glandium

The leave-open keyword is there and there is no activity for 6 months.
:decoder, maybe it's time to close this bug?

Flags: needinfo?(choller)

There appears to remain one bug in SM91: Without the JS shell, the rust libraries aren't linked.

The issue is here: https://github.com/mozilla/gecko-dev/blob/master/js/src/moz.build#L53
The 'rust' library is needed even if the JS shell is disabled. I believe this was changed in https://bugzilla.mozilla.org/show_bug.cgi?id=1576082, but I think rust become necessary anyways after that date, and this change probably just got lost.

The fix is trivial, rust just needs to be added unconditionally to the 'js' library DIRS.

The leave-open keyword is there and there is no activity for 6 months.
:decoder, maybe it's time to close this bug?
For more information, please visit auto_nag documentation.

Flags: needinfo?(choller)
Severity: normal normal → S3 S3
Flags: needinfo?(choller)

(In reply to wraitii from comment #35)

There appears to remain one bug in SM91: Without the JS shell, the rust libraries aren't linked.

The issue is here: https://github.com/mozilla/gecko-dev/blob/master/js/src/moz.build#L53
The 'rust' library is needed even if the JS shell is disabled. I believe this was changed in https://bugzilla.mozilla.org/show_bug.cgi?id=1576082, but I think rust become necessary anyways after that date, and this change probably just got lost.

The fix is trivial, rust just needs to be added unconditionally to the 'js' library DIRS.

The following issue is still present in SM135

You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: