580407 - Link js statically via --disable-shared-js

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Comment 1

[(dormant account)]

Attachment: static mozjs

Comment 2

[Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)]

Why are we making this a configure option?

Comment 3

[(dormant account)]

Linux users might want separate js

Comment 4

[Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)]

/me sighs, ok, I'll try to review tomorrow.

Comment 5

[Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)]

Comment on attachment 458806 [details] [diff] [review]
static mozjs

I think this is ok, but it would be a lot easier to understand if --disable-shared-js defined JS_SHARED_LIBRARY so that my tiny mind didn't have to parse double negatives everywhere.

Somebody else should look over the xpcconnect changes even though they're simple.

Comment 6

[(dormant account)]

Attachment: staticjs v2

This gets rid of doublenegatives. The behavior is that mozjs is linked statically for mozilla builds unless --enable-shared-js is specified. If one configures a standalone spidermonkey --enable-shared-js is the default.

Comment 7

[Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)]

Comment on attachment 459097 [details] [diff] [review]
staticjs v2

># HG changeset patch
># Parent e2346fa5faaa957a3689c1192584bedb63924a32
>Bug 580407 - Link js statically via --disable-shared-js
>
>diff --git a/config/autoconf.mk.in b/config/autoconf.mk.in
>--- a/config/autoconf.mk.in
>+++ b/config/autoconf.mk.in
>@@ -669,3 +669,5 @@ ANDROID_TOOLCHAIN = @ANDROID_TOOLCHAIN@
> ANDROID_PLATFORM  = @ANDROID_PLATFORM@
> ANDROID_SDK       = @ANDROID_SDK@
> ANDROID_TOOLS     = @ANDROID_TOOLS@
>+
>+JS_SHARED_LIBRARY = @JS_SHARED_LIBRARY@
>diff --git a/configure.in b/configure.in
>--- a/configure.in
>+++ b/configure.in
>@@ -74,6 +74,16 @@ TARGET_OS="${target_os}"
> MOZ_DEB_TIMESTAMP=`date +"%a, %d  %b %Y %T %z"   2>&1` 
> AC_SUBST(MOZ_DEB_TIMESTAMP)
> 
>+MOZ_ARG_ENABLE_BOOL(shared-js,
>+[  --enable-shared-js
>+                          Create a shared JavaScript library.],
>+    ENABLE_SHARED_JS=1,
>+    ENABLE_SHARED_JS=0)
>+
>+if test "$ENABLE_SHARED_JS" = "1" ; then
>+  JS_SHARED_LIBRARY=1
>+fi
>+AC_SUBST(JS_SHARED_LIBRARY)
> 
> dnl ========================================================
> dnl =
>@@ -1216,7 +1226,11 @@ MOZ_BZ2_LIBS='$(call EXPAND_LIBNAME_PATH
> MOZ_PNG_CFLAGS=
> MOZ_PNG_LIBS='$(call EXPAND_LIBNAME_PATH,mozpng,$(DEPTH)/modules/libimg/png)'
> 
>+if test -z "$JS_SHARED_LIBRARY" ; then
>+MOZ_JS_LIBS='-L$(LIBXUL_DIST)/bin -ljs_static'
>+else
> MOZ_JS_LIBS='-L$(LIBXUL_DIST)/bin -lmozjs'
>+fi
> DYNAMIC_XPCOM_LIBS='-L$(LIBXUL_DIST)/bin -lxpcom -lxpcom_core -lmozalloc'
> MOZ_FIX_LINK_PATHS='-Wl,-rpath-link,$(LIBXUL_DIST)/bin -Wl,-rpath-link,$(prefix)/lib'
> XPCOM_FROZEN_LDOPTS='-L$(LIBXUL_DIST)/bin -lxpcom -lmozalloc'
>@@ -2269,7 +2283,11 @@ ia64*-hpux*)
>     MOZ_DEBUG_FLAGS='-Zi'
>     MOZ_DEBUG_LDFLAGS='-DEBUG -DEBUGTYPE:CV'
>     MOZ_FIX_LINK_PATHS=
>-    MOZ_JS_LIBS='$(LIBXUL_DIST)/lib/mozjs.lib'
>+    if test -z "$JS_SHARED_LIBRARY" ; then
>+       MOZ_JS_LIBS='$(LIBXUL_DIST)/lib/js_static.lib'
>+    else
>+       MOZ_JS_LIBS='$(LIBXUL_DIST)/lib/mozjs.lib'
>+    fi
>     OBJ_SUFFIX=obj
>     RANLIB='echo not_ranlib'
>     STRIP='echo not_strip'
>@@ -2329,7 +2347,11 @@ ia64*-hpux*)
>         RCFLAGS='-O coff --use-temp-file'
>         # mingw doesn't require kernel32, user32, and advapi32 explicitly
>         LIBS="$LIBS -luuid -lgdi32 -lwinmm -lwsock32"
>-        MOZ_JS_LIBS='-L$(LIBXUL_DIST)/lib -lmozjs'
>+        if test -z "$JS_SHARED_LIBRARY" ; then
>+           MOZ_JS_LIBS='-L$(LIBXUL_DIST)/bin -ljs_static'
>+        else
>+           MOZ_JS_LIBS='-L$(LIBXUL_DIST)/bin -lmozjs'
>+        fi
>         MOZ_FIX_LINK_PATHS=
>         DYNAMIC_XPCOM_LIBS='-L$(LIBXUL_DIST)/lib -lxpcom -lxpcom_core -lmozalloc'
>         XPCOM_FROZEN_LDOPTS='-L$(LIBXUL_DIST)/lib -lxpcom -lmozalloc'
>@@ -2375,7 +2397,11 @@ ia64*-hpux*)
>         MOZ_DEBUG_LDFLAGS='-DEBUG -DEBUGTYPE:CV'
>         WARNINGS_AS_ERRORS='-WX'
>     	MOZ_OPTIMIZE_FLAGS='-O1'
>-        MOZ_JS_LIBS='$(LIBXUL_DIST)/lib/mozjs.lib'
>+        if test -z "$JS_SHARED_LIBRARY" ; then
>+           MOZ_JS_LIBS='$(LIBXUL_DIST)/lib/js_static.lib'
>+        else
>+           MOZ_JS_LIBS='$(LIBXUL_DIST)/lib/mozjs.lib'
>+        fi
>         MOZ_FIX_LINK_PATHS=
>         DYNAMIC_XPCOM_LIBS='$(LIBXUL_DIST)/lib/xpcom.lib $(LIBXUL_DIST)/lib/xpcom_core.lib $(LIBXUL_DIST)/lib/mozalloc.lib'
>         XPCOM_FROZEN_LDOPTS='$(LIBXUL_DIST)/lib/xpcom.lib $(LIBXUL_DIST)/lib/mozalloc.lib'
>@@ -9198,6 +9224,9 @@ if test "$BUILD_CTYPES"; then
>     # Build js-ctypes on the platforms we can.
>     ac_configure_args="$ac_configure_args --enable-ctypes"
> fi
>+if test -z "$JS_SHARED_LIBRARY" ; then
>+    ac_configure_args="$ac_configure_args --disable-shared-js"
>+fi
> if test -z "$MOZ_NATIVE_NSPR"; then
>     ac_configure_args="$ac_configure_args --with-nspr-cflags='$NSPR_CFLAGS'"
>     ac_configure_args="$ac_configure_args --with-nspr-libs='$NSPR_LIBS'"
>diff --git a/js/src/Makefile.in b/js/src/Makefile.in
>--- a/js/src/Makefile.in
>+++ b/js/src/Makefile.in
>@@ -119,8 +119,11 @@ endif
> # In fact, we now build both a static and a shared library, as the
> # JS shell would like to link to the static library.
> 
>+ifdef JS_SHARED_LIBRARY
> FORCE_SHARED_LIB = 1
>+endif
> FORCE_STATIC_LIB = 1
>+DIST_INSTALL = 1
> 
> VPATH		= $(srcdir)
> 
>diff --git a/js/src/config/autoconf.mk.in b/js/src/config/autoconf.mk.in
>--- a/js/src/config/autoconf.mk.in
>+++ b/js/src/config/autoconf.mk.in
>@@ -338,3 +338,5 @@ WRAP_SYSTEM_INCLUDES = @WRAP_SYSTEM_INCL
> ENABLE_TRACEJIT = @ENABLE_TRACEJIT@
> NANOJIT_ARCH = @NANOJIT_ARCH@
> HAVE_ARM_SIMD= @HAVE_ARM_SIMD@
>+
>+JS_SHARED_LIBRARY = @JS_SHARED_LIBRARY@
>diff --git a/js/src/configure.in b/js/src/configure.in
>--- a/js/src/configure.in
>+++ b/js/src/configure.in
>@@ -206,6 +206,19 @@ MOZ_ARG_ENABLE_BOOL(compile-environment,
>     COMPILE_ENVIRONMENT=1,
>     COMPILE_ENVIRONMENT= )
> 
>+MOZ_ARG_ENABLE_BOOL(shared-js,
>+[  --disable-shared-js
>+                          Do not create a shared library.],
>+    DISABLE_SHARED_JS=0,
>+    DISABLE_SHARED_JS=1)
>+
>+if test "$DISABLE_SHARED_JS" = "1" ; then
>+  AC_DEFINE(STATIC_JS_API)
>+else
>+  JS_SHARED_LIBRARY=1
>+fi
>+AC_SUBST(JS_SHARED_LIBRARY)
>+
> dnl ========================================================
> dnl = Android uses a very custom (hacky) toolchain; we need to do this
> dnl = here, so that the compiler checks can succeed
>diff --git a/js/src/xpconnect/src/xpcwrappednativejsops.cpp b/js/src/xpconnect/src/xpcwrappednativejsops.cpp
>--- a/js/src/xpconnect/src/xpcwrappednativejsops.cpp
>+++ b/js/src/xpconnect/src/xpcwrappednativejsops.cpp
>@@ -1276,7 +1276,13 @@ XPC_WN_Helper_NewResolve(JSContext *cx, 
> 
> /***************************************************************************/
> 
>-extern "C" JS_IMPORT_DATA(JSObjectOps) js_ObjectOps;
>+extern "C"
>+#ifndef STATIC_JS_API
>+JS_IMPORT_DATA(JSObjectOps)
>+#else
>+JSObjectOps
>+#endif
>+js_ObjectOps;
> 
> static JSObjectOps XPC_WN_WithCall_JSOps;
> static JSObjectOps XPC_WN_NoCall_JSOps;

Comment 8

[Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)]

Gah I need to stop doing that.

Comment 9

[(dormant account)]

Attachment: Static js v2.5

Carrying over khuey's review

Comment 10

[(dormant account)]

Nominated this as a blocker because it's one of the incremental steps on the way to a keeping our code in a single dll in bug 561842. This improves startup by causing fewer library page faults.

Comment 11

[(dormant account)]

checkin-needed on mozilla-central.

Comment 12

[Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)]

Please avoid landing this until Bug 522770 is fixed.  Making libxul bigger is scary for the moment.

Comment 13

[Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)]

Bug 522770 has been fixed to the point where landing this no longer gives me nightmares.

Comment 14

[(dormant account)]

Attachment: 459588: Static js

Carrying over r+. Updated to apply cleanly on trunk. Got rid of the xpcwrappednativejsops.cpp as that code doesn't exist anymore.

Comment 15

[(dormant account)]

Attachment: Static js

Last patch omitted the installer manifest.

Comment 16

[Shawn Wilsher :sdwilsh]

Are we going to get a new patch before Friday?  This needs to land by then.  Also, when uploading the patch, please keep in mind the points raised in http://blog.bonardo.net/2010/06/22/so-youre-about-to-use-checkin-needed (specifically, you aren't storing your username in the patch which makes more work for the person being kind enough to land for you).

Comment 17

[(dormant account)]

(In reply to comment #16)
> Are we going to get a new patch before Friday?  This needs to land by then. 
> Also, when uploading the patch, please keep in mind the points raised in
> http://blog.bonardo.net/2010/06/22/so-youre-about-to-use-checkin-needed
> (specifically, you aren't storing your username in the patch which makes more
> work for the person being kind enough to land for you).

hopefully. I found a weird regression in (Components.classes is not a function) in xpcshell-tests, no idea what's causing it yet.

Comment 18

[(dormant account)]

Attachment: Fixed patch

Too bad I didn't catch this one earlier http://scotland.proximity.on.ca/dxr//mozilla-central/js/src/jstypes.h.html#l167 for some reason makes xpcshell unhappy. In retrospect, static js isn't going into a binary perhaps setting STATIC_JS_API wasn't the wisest choice. Instead I just added JS_SHARED_LIBRARY to guard Dllmain.

Comment 19

[(dormant account)]

Comment on attachment 465397 [details] [diff] [review]
Fixed patch

Nevermind, this wont work.

Comment 20

[(dormant account)]

Attachment: Fixed patch(force shared js when tests are enabled)

Sorry it took so long. I finally realized that the problem is that xpcshell links to xul AND mozjs. That clearly doesn't work with a staticjs lib. It's possible to have staticjs + tests if we fold xpcshell code into libxul(conditionally on --enable-tests).

Comment 21

[Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)]

Comment on attachment 465485 [details] [diff] [review]
Fixed patch(force shared js when tests are enabled)

I didn't actually look at the patch, but we ship releases with --enable-tests, and I don't think folding xpcshell into code we ship is something we want to do.

Comment 22

[Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)]

If mozjs is in xul why does xpcshell need to link to mozjs anymore?

Comment 23

[Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)]

So now that I read your patch it doesn't fold xpcshell into libxul at all.  It forces js to be shared when --enable-tests is on.  That still won't fly though because we ship releases with --enable-tests.

Comment 24

[(dormant account)]

(In reply to comment #22)
> If mozjs is in xul why does xpcshell need to link to mozjs anymore?

Because currently JS gets folded in by a static-like link. Ie the JS symbols aren't exported. Changing the build to export JS symbols via libxul would involve including jsheaders with JS_EXPORT_API from xul code. That's the right way to fix this but I don't know how to do that(yet).

(In reply to comment #23)
> So now that I read your patch it doesn't fold xpcshell into libxul at all.  It
> forces js to be shared when --enable-tests is on.  That still won't fly though
> because we ship releases with --enable-tests.

I didn't realize that's how we ship :( Thanks for the quick feedback.

Comment 25

[Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)]

(In reply to comment #24)
> (In reply to comment #22)
> > If mozjs is in xul why does xpcshell need to link to mozjs anymore?
> 
> Because currently JS gets folded in by a static-like link. Ie the JS symbols
> aren't exported. Changing the build to export JS symbols via libxul would
> involve including jsheaders with JS_EXPORT_API from xul code. That's the right
> way to fix this but I don't know how to do that(yet).

AIUI that's what http://mxr.mozilla.org/mozilla-central/source/toolkit/library/dlldeps-xul.cpp is for.  See the comments at the top of http://mxr.mozilla.org/mozilla-central/source/xpcom/build/dlldeps.cpp

Comment 26

[(dormant account)]

I was talking about defining EXPORT_JS_API from xul code http://scotland.proximity.on.ca/dxr//mozilla-central/js/src/jstypes.h.html#l167
so dll exports could be defined correctly.

Comment 27

[Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)]

The solution here is probably to AC_DEFINE(EXPORT_JS_API) instead of STATIC_JS_API then.

Comment 28

[Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)]

(In reply to comment #27)
> The solution here is probably to AC_DEFINE(EXPORT_JS_API) instead of
> STATIC_JS_API then.

Well, not quite.  I think what we really want is to have EXPORT_JS_API defined whenever MOZILLA_INTERNAL_API is defined and IMPORT_JS_API defined anywhere else.

Comment 29

[(not currently active) Ted Mielczarek]

Could you just link static js into xpcshell? (I'm not sure how the interaction between two static copies of JS would work.)

Comment 30

[Benjamin Smedberg]

Statically linking JS into xpcshell probably won't work. We really need to be exporting the JS symbols. I think this means:

#if defined(STATIC_JS_API) && defined(MOZILLA_INTERNAL_API)
# define JS_PUBLIC_API(t)   JS_EXPORT_API(t)
...

and keep the rest of the jstypes.h the same

Comment 31

[(dormant account)]

Attachment: Static js that exports symbols via libxul

This uses khuey's approach for exporting js symbols via libxul

Comment 32

[(dormant account)]

Attachment: Static js + checkin message + username

Carrying over r+. This patch is for checkin-ride-along https://wiki.mozilla.org/LandingQueue

Comment 33

[(dormant account)]

Attachment: Static js + checkin message + username

Minor omission in js/config

Comment 34

[[e-mail]]

http://hg.mozilla.org/projects/electrolysis/rev/b6c7ed5a2922

Comment 35

[Mats Palmgren (:mats)]

This made x86-64 Linux builds with --disable-libxul crash: bug 587375

Comment 36

[Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)]

We probably should document that all of the JS symbols are now exposed from libxul.

Comment 37

[Eric Shepherd [:sheppy]]

Looking at this patch, I'm slightly confused. Does this add --disable-shared-js or --enable-shared-js or both? It looks like it adds both, which seems odd. Need to get that sorted out to write this up properly for docs.

Comment 38

[Eric Shepherd [:sheppy]]

FWIW, assuming that --disable-shared-js is the correct flag, I've documented this here:

https://developer.mozilla.org/En/SpiderMonkey/Build_Documentation#Building_SpiderMonkey_as_a_static_library

Comment 39

[(dormant account)]

(In reply to comment #37)
> Looking at this patch, I'm slightly confused. Does this add --disable-shared-js
> or --enable-shared-js or both? It looks like it adds both, which seems odd.
> Need to get that sorted out to write this up properly for docs.

Your docs got it right. --disable-shared-js is a flag added to spidermonkey since by default it should build a shared library. Mozilla builds static js by default so it needed an opposite flag.