On Windows, when building JS_STANDALONE with STATIC_JS_API, we still end up with stuff linked to mozglue with DLL linkage, even if mozglue is built as a static library. This is because while in JS, dllexport/dllimport stuff is behind STATIC_JS_API, there's a few things that get used from mfbt that are explicitly MOZ_EXPORT. This patch adds a MOZ_NO_IMPORT_EXPORT define that just kills off any/all dllimport/dllexport goop, for when we really, really just want everything to be in a static library.

Comment on attachment 8705710 [details] [diff] [review] add MOZ_NO_IMPORT_EXPORT Review of attachment 8705710 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for the explanation of what's going on. What are these mozjs/ paths in your patch? r=me with the dropping of the configure.in bit and the Types.h changes below. ::: mozjs/js/src/configure.in @@ +155,4 @@ > if test "$DISABLE_SHARED_JS" = "1" ; then > if test "$DISABLE_EXPORT_JS" = "1"; then > AC_DEFINE(STATIC_JS_API) > + AC_DEFINE(MOZ_NO_IMPORT_EXPORT) This seems a little cryptic, and has the downside of adding yet another thing to configure, which is not great. How about we drop this and... ::: mozjs/mfbt/Types.h @@ +37,4 @@ > * These macros are designed for use by library interfaces -- not for normal > * methods or data used cross-file. > */ > +#if defined(MOZ_NO_IMPORT_EXPORT) ...we instead do something like: // Explanatory comment (the first paragraph of comment 0 would be // a good start to this comment) #if defined(JS_STANDALONE) && defined(JS_STATIC_API) #define MOZ_NO_IMPORT_EXPORT #endif right about here: http://mxr.mozilla.org/mozilla-central/source/mfbt/Types.h#23 and then it's more obvious what MOZ_NO_IMPORT_EXPORT is doing, IMHO. It's not great to have these JS_* preprocessor symbols referenced in MFBT, but it seems clearer than the alternative.

Comment on attachment 8705710 [details] [diff] [review] add MOZ_NO_IMPORT_EXPORT Review of attachment 8705710 [details] [diff] [review]: ----------------------------------------------------------------- This seems wrong to me. You seem to want to change the meaning of MFBT_API and MFBT_DATA, not MOZ_EXPORT.

Also I'm not sure about the premise. (In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #0) > On Windows, when building JS_STANDALONE with STATIC_JS_API, we still end up > with stuff linked to mozglue with DLL linkage, even if mozglue is built as a > static library. I don't think mozglue is built as a static library when building js standalone on windows.

(In reply to Mike Hommey (VAC: 31 Dec - 11 Jan) [:glandium] from comment #3) > I don't think mozglue is built as a static library when building js > standalone on windows. Not yet it isn't ;) Sorry, that's in a separate patch that I haven't uploaded yet.

(In reply to Nathan Froyd [:froydnj] from comment #1) > What are these mozjs/ paths in your patch? The patch is against Servo's copy of mozjs; the root tree is in mozjs. I should have cleaned that up first, didn't notice it. > r=me with the dropping of the configure.in bit and the Types.h changes below. > > ::: mozjs/js/src/configure.in > @@ +155,4 @@ > > if test "$DISABLE_SHARED_JS" = "1" ; then > > if test "$DISABLE_EXPORT_JS" = "1"; then > > AC_DEFINE(STATIC_JS_API) > > + AC_DEFINE(MOZ_NO_IMPORT_EXPORT) > > This seems a little cryptic, and has the downside of adding yet another > thing to configure, which is not great. How about we drop this and... > > ::: mozjs/mfbt/Types.h > @@ +37,4 @@ > > * These macros are designed for use by library interfaces -- not for normal > > * methods or data used cross-file. > > */ > > +#if defined(MOZ_NO_IMPORT_EXPORT) > > ...we instead do something like: > > // Explanatory comment (the first paragraph of comment 0 would be > // a good start to this comment) > #if defined(JS_STANDALONE) && defined(JS_STATIC_API) > #define MOZ_NO_IMPORT_EXPORT > #endif > > right about here: > > http://mxr.mozilla.org/mozilla-central/source/mfbt/Types.h#23 > > and then it's more obvious what MOZ_NO_IMPORT_EXPORT is doing, IMHO. Works for me. I'll put up a new clean patch, as well as the static mozglue bit in a separate patch. > It's not great to have these JS_* preprocessor symbols referenced in MFBT, > but it seems clearer than the alternative. Yeah, we already do anyway in mozglue.. JS_STANDALONE gets spread a little throughout the code wherever necessary unfortunately.

(In reply to Mike Hommey (VAC: 31 Dec - 11 Jan) [:glandium] from comment #2) > This seems wrong to me. You seem to want to change the meaning of MFBT_API > and MFBT_DATA, not MOZ_EXPORT. That's a very good point. Would you guys prefer if I changed the various MFBT_DATA/MFBT_API/MOZ_IMPORT_DATA/MOZ_IMPORT_API defines instead? Unfortunately, comments in mfbt/Types.h still mention using MOZ_EXPORT directly.. there aren't many places that do, but there are a small handful.

So what you really want is to define IMPL_MFBT when building js, which is what was done when mfbt was statically linked to it before bug 1134039. Now, if you could detail what you actually want to do, that would be helpful, because statically linking mozglue is *not* going to do what you think it should do, and will, in practice, revert bug 1134039, because jemalloc will stop being used (iirc how things end up being linked).

Let's try this.. this works equally well for Servo's purposes.

What I want is a single libjs_static.a (js_static.lib), and perhaps a libmozglue.a (mozglue.lib) that I can statically link in to servo.exe and get a working JS engine for servo. I don't think servo uses jemalloc, but these changes shouldn't regress shell builds and the like. Hrm. With any dllimport/dllexport/etc. bits, either we end up with referencing __imp__ symbols that don't exist (due to dllimport) or end up depending on a DLL that we don't want.

s/MOZ_FORCE_STATIC_VISIBILITY/MOZ_FULLY_STATIC_MFBT/ ?

(In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #9) > I don't think servo uses jemalloc, but > these changes shouldn't regress shell builds and the like. Hrm. Servo uses jemalloc internally because that's Rust's default allocator. Servo's SpiderMonkey doesn't use jemalloc right now. However, we'll eventually want to make SpiderMonkey use the Rust allocator, which is an entirely separate task from all this, so I think we don't need to worry about jemalloc at all here.

Comment on attachment 8705910 [details] [diff] [review] JS Static linkage, v2 Review of attachment 8705910 [details] [diff] [review]: ----------------------------------------------------------------- r=me, but you need to get a second opinion on the JS engine bits. (I see you have an empty f? request open...) ::: js/src/jstypes.h @@ -35,5 @@ > #include "jsversion.h" > > -/*********************************************************************** > -** MACROS: JS_EXTERN_API > -** JS_EXPORT_API I am a fan of this deletion, but you should ask a JS engine person (jorendorff? sstangl?) about removing this stuff. ::: mfbt/Types.h @@ +23,5 @@ > +/* When building standalone JS, with the static API only, we don't want > + * to use any visibility attributes (especially not dllimport/dllexport). > + */ > +#if defined(JS_STANDALONE) && defined(STATIC_JS_API) > +#define MOZ_FORCE_STATIC_VISIBILITY 1 Mike's suggestion seems a little better here.

Comment on attachment 8705910 [details] [diff] [review] JS Static linkage, v2 Review of attachment 8705910 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jstypes.h @@ -35,5 @@ > #include "jsversion.h" > > -/*********************************************************************** > -** MACROS: JS_EXTERN_API > -** JS_EXPORT_API If those are really the only uses, go ahead. The less of the API macro gunk we have, the better, IMO. (That said, if js.cpp is really just self-declaring methods defined in editline.c with no API macro wrapper, that really *should* be defined in editline.h, it seems like it'd be better to declare those in editline.h, define them in editline.c, and make the #ifdef in js.cpp include editline.h. rather than the current approach. IMO, and if I'm understanding correctly. Maybe for a separate bug.)

(In reply to Nathan Froyd [:froydnj] from comment #12) > Comment on attachment 8705910 [details] [diff] [review] > JS Static linkage, v2 > > Review of attachment 8705910 [details] [diff] [review]: > ----------------------------------------------------------------- > > r=me, but you need to get a second opinion on the JS engine bits. (I see > you have an empty f? request open...) The f? was supposed to be for :glandium, but he's marked as not accepting r/f requests :/ > ::: mfbt/Types.h > @@ +23,5 @@ > > +/* When building standalone JS, with the static API only, we don't want > > + * to use any visibility attributes (especially not dllimport/dllexport). > > + */ > > +#if defined(JS_STANDALONE) && defined(STATIC_JS_API) > > +#define MOZ_FORCE_STATIC_VISIBILITY 1 > > Mike's suggestion seems a little better here. Ah yep, will change to MOZ_FULLY_STATIC_MFBT. (In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #13) > > -/*********************************************************************** > > -** MACROS: JS_EXTERN_API > > -** JS_EXPORT_API > > If those are really the only uses, go ahead. The less of the API macro gunk > we have, the better, IMO. Yeah, I was surprised; I'm guessing they were left over since the split into PUBLIC/FRIEND API. > (That said, if js.cpp is really just self-declaring methods defined in > editline.c with no API macro wrapper, that really *should* be defined in > editline.h, it seems like it'd be better to declare those in editline.h, > define them in editline.c, and make the #ifdef in js.cpp include editline.h. > rather than the current approach. IMO, and if I'm understanding correctly. > Maybe for a separate bug.) I confess I didn't quite understand what the editline stuff was trying to do. Like, I don't see why it needed the EXPORT_API stuff at all.

(In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #14) > > (That said, if js.cpp is really just self-declaring methods defined in > > editline.c with no API macro wrapper, that really *should* be defined in > > editline.h, it seems like it'd be better to declare those in editline.h, > > define them in editline.c, and make the #ifdef in js.cpp include editline.h. > > rather than the current approach. IMO, and if I'm understanding correctly. > > Maybe for a separate bug.) > > I confess I didn't quite understand what the editline stuff was trying to > do. Like, I don't see why it needed the EXPORT_API stuff at all. Because while there's a copy of editline in the tree, it's also possible to build against libreadline. And for that one, the symbols need to be marked public, since they're foreign.

(In reply to Mike Hommey [:glandium] from comment #15) > (In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #14) > > I confess I didn't quite understand what the editline stuff was trying to > > do. Like, I don't see why it needed the EXPORT_API stuff at all. > > Because while there's a copy of editline in the tree, it's also possible to > build against libreadline. And for that one, the symbols need to be marked > public, since they're foreign. Hmm -- EDITLINE is defined if we're using editline or libreadline. If we're using the built-in editline, then these symbols don't really need decoration. We link to the static editline lib. If we're linking with libreadline, then we're pulling readline()/add_history() in from libreadline.. in which case marking these symbols as JS_PUBLIC_API seems wrong, because that will mark them as symbols for export from JS ("symbols need to be marked public, since they're foreign" doesn't make sense to me -- symbols are marked public if you intend to export them from the currently being built dynamic object, or import them if you're using a header for a foreign object; right?). ... and this seems to only "accidentally work" because Windows builds define NO_EDITLINE, which means we don't end up with the bogus dllimport stuff we'd see otherwise. I'm going to just leave this as PUBLIC_API and not touch it :)

(In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #16) > Hmm -- EDITLINE is defined if we're using editline or libreadline. If we're > using the built-in editline, then these symbols don't really need > decoration. We link to the static editline lib. If we're linking with > libreadline, then we're pulling readline()/add_history() in from > libreadline.. in which case marking these symbols as JS_PUBLIC_API seems > wrong It is necessary, otherwise the symbols will be marked with visibility hidden on linux, and linking against libreadline would fail.