library symbol conflicts with spidermonkey-1.8.5

RESOLVED FIXED in mozilla23

Status

()

Core
Build Config
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: Ian Stakenvicius, Assigned: Ian Stakenvicius)

Tracking

(Blocks: 1 bug)

Trunk
mozilla23
All
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 11 obsolete attachments)

1011 bytes, text/plain
Details
1.83 KB, patch
glandium
: review+
Details | Diff | Splinter Review
1.41 KB, patch
gps
: review+
glandium
: feedback-
Details | Diff | Splinter Review
(Assignee)

Description

5 years ago
Created attachment 679142 [details] [diff] [review]
firefox-js-symver.patch

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.4 (KHTML, like Gecko) Chrome/22.0.1229.94 Safari/537.4

Steps to reproduce:

Various mozilla products, such as the pre-built binary versions of Firefox 15,16 , Thunderbird 15,16 , Seamonkey 2.13.2 , etc. will crash (segfault) at some point (often on exit) after they load plugins or extensions that link to spidermonkey-1.8.5.

The root cause of this issue seems to be that either firefox uses symbols from the external libmozjs185.so or the plugin uses symbols from libxul.so, either one of which will cause a crash as the JS components within libxul are not ABI compatible with libmozjs185.

The solution seems to be that unique symbol versioning must be applied to both spidermonkey and the js symbols in libxul.  With the help of 'glandium' in #developers on irc.mozilla.org I was able to develop these two patches, which when tested on my system seem to prevent the crash of FF16.0.2 caused by libproxy-0.4.10 (linked to spidemronkey-1.8.5)


Actual results:

I don't know how to attach two files to the same bug at once, so here is the patch against spidermonkey-1.8.5.:

diff -Naur js/src/configure.in js.new/src/configure.in
--- a/js/src/configure.in       2012-11-07 09:36:16.000000000 -0500
+++ b/js/src/configure.in       2012-11-07 09:37:05.000000000 -0500
@@ -6018,6 +6018,11 @@
 rm conftest.sh
 
 echo $MAKEFILES > unallmakefiles
+cat <<SYMVEREOF >symverscript
+MOZJS185 {
+  global: *;
+};
+SYMVEREOF
 
 mv -f config/autoconf.mk config/autoconf.mk.orig 2> /dev/null
 
diff -Naur js/src/Makefile.in js.new/src/Makefile.in
--- a/js/src/Makefile.in        2012-11-07 09:36:16.000000000 -0500
+++ b/js/src/Makefile.in        2012-11-07 09:37:09.000000000 -0500
@@ -871,7 +871,7 @@
 SHLIB_ANY_VER   := $(DESTDIR)$(libdir)/$(SHARED_LIBRARY)
 SHLIB_ABI_VER   := $(DESTDIR)$(libdir)/$(SHARED_LIBRARY).$(SRCREL_ABI_VERSION)
 SHLIB_EXACT_VER := $(DESTDIR)$(libdir)/$(SHARED_LIBRARY).$(SRCREL_VERSION)
-$(SHARED_LIBRARY): EXTRA_DSO_LDOPTS += -Wl,-soname,$(notdir $(SHLIB_ABI_VER))
+$(SHARED_LIBRARY): EXTRA_DSO_LDOPTS += -Wl,-soname,$(notdir $(SHLIB_ABI_VER)) -Wl,-version-script,symverscript
 endif
 endif
(Assignee)

Comment 1

5 years ago
Created attachment 679143 [details] [diff] [review]
spidermonkey-1.8.5-symver.patch

second patch, attached as a file.
(Assignee)

Updated

5 years ago
Assignee: nobody → axs
Component: Untriaged → Build Config
Hardware: x86_64 → All
Version: 16 Branch → Trunk
Comment on attachment 679143 [details] [diff] [review]
spidermonkey-1.8.5-symver.patch

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

::: js/src/Makefile.in
@@ +871,4 @@
>  SHLIB_ANY_VER   := $(DESTDIR)$(libdir)/$(SHARED_LIBRARY)
>  SHLIB_ABI_VER   := $(DESTDIR)$(libdir)/$(SHARED_LIBRARY).$(SRCREL_ABI_VERSION)
>  SHLIB_EXACT_VER := $(DESTDIR)$(libdir)/$(SHARED_LIBRARY).$(SRCREL_VERSION)
> +$(SHARED_LIBRARY): EXTRA_DSO_LDOPTS += -Wl,-soname,$(notdir $(SHLIB_ABI_VER)) -Wl,-version-script,symverscript

soname should already be set by the build system, so you only really need -version-script.

Please don't generate the symbol version script in configure.in, but instead, create it in Makefile.in with Preprocessor.py (check the various places in the tree using that script), and add it as a dependency of the shared library target.
Attachment #679143 - Flags: review-
Comment on attachment 679142 [details] [diff] [review]
firefox-js-symver.patch

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

::: mozilla-release/toolkit/library/Makefile.in
@@ +617,4 @@
>  endif
>  endif
>  
> +EXTRA_DSO_LDOPTS += -Wl,-version-script,../../symverscript

Same comments as the other patch ; this also needs to be conditional to building for linux.
Attachment #679142 - Flags: review-
Note you can merge both patches.
(Assignee)

Comment 5

5 years ago
(In reply to Mike Hommey [:glandium] from comment #2)
> > +$(SHARED_LIBRARY): EXTRA_DSO_LDOPTS += -Wl,-soname,$(notdir $(SHLIB_ABI_VER)) -Wl,-version-script,symverscript
> 
> soname should already be set by the build system, so you only really need
> -version-script.
> 

That's all i did -- the '-Wl,-soname,$(notdir $(SHLIB_ABI_VER))' part already existed within Makefile.in, i just appended to it.


> Please don't generate the symbol version script in configure.in, but
> instead, create it in Makefile.in with Preprocessor.py (check the various
> places in the tree using that script), and add it as a dependency of the
> shared library target.

I did it within configure.in for Firefox as the symbol-version is created dynamically from ${MOZ_APP_NAME} and ${MOZ_APP_VERSION}  (which i thought would theoretically allow the exact same code to be used across all mozilla products).


(In reply to Mike Hommey [:glandium] from comment #4)
> Note you can merge both patches.

I was thinking that would be the case, but I haven't tested it.  I'm working from official source tarball distfiles rather than sources checked out from VCS.


Can the improvements necessary for integration be left to the mozilla dev's (who know what they're doing) or would it be quicker if I attempted to improve the patches and resubmit?
(Assignee)

Comment 6

5 years ago
Created attachment 679299 [details] [diff] [review]
Updated patch to FF (etc) acceptable for submission

This patch generates the symverscript and applies it appropriately, in better accordance with the mozilla build system
Attachment #679142 - Attachment is obsolete: true
(Assignee)

Comment 7

5 years ago
Created attachment 679305 [details]
updated patch against a distributable spidermonkey

This patch is of a similar format to the updated FF patch, but is meant to be applied against a spidermonkey release.  Mozilla-Central does not seem to hold 'js' sources appropriate for an actual spidermonkey release, and so this patch will not apply cleanly there.

The patch as-is applies to js185 , and will with minor modification apply to js187 as proposed in https://bugzilla.gnome.org/show_bug.cgi?id=678410 ..  there seems to be no way to nicely substitute the library soname via Preprocessor.py into symverscript (since there is no defined source of the JS_VERSION in configure, etc), so the symverscript.in file will need to be adjusted by hand.
Attachment #679143 - Attachment is obsolete: true
Component: Build Config → Build Config
Product: Firefox → Core
(Assignee)

Comment 8

5 years ago
oops, I meant the js187 as proposed in bug 735599
(Assignee)

Comment 9

5 years ago
Created attachment 679705 [details] [diff] [review]
Patch to merge to mozilla-central

I added a forward-port of the spidermonkey patch (as best i could do) to make it work with the mozilla-central codebase -- symbol version is set to MOZJS_EXT instead of something that would be version-specific to a particular spidermonkey release; this will suffice to keep from conflicting against libxul.

Updated

5 years ago
Attachment #679705 - Flags: review?(mh+mozilla)
Comment on attachment 679705 [details] [diff] [review]
Patch to merge to mozilla-central

>+#filter substitution
>+@MOZ_APP_BASENAME@_@MOZ_APP_VERSION@ {
>+	global: JS_*; js_*; _Z[NTVK0-9]*JS*; _Z[NTVK0-9]*js*;

Please put each on a separate line.

>--- a/mozilla/toolkit/library/Makefile.in	2012-11-07 11:08:34.000000000 -0500
>+++ b/mozilla/toolkit/library/Makefile.in	2012-11-07 11:08:15.000000000 -0500
>@@ -120,6 +120,13 @@
> ifeq (Linux,$(OS_ARCH))
> ifneq (Android,$(OS_TARGET))
> OS_LIBS += -lrt
>+EXTRA_DSO_LDOPTS += -Wl,-version-script,symverscript
>+
>+symverscript: symverscript.in
>+	$(PYTHON) $(topsrcdir)/config/Preprocessor.py -DMOZ_APP_BASENAME="$(MOZ_APP_BASENAME)" -DMOZ_APP_VERSION="$(MOZ_APP_VERSION)" $< > $@
>+
>+EXTRA_DEPS += symverscript
>+
> endif
> endif
> 
>--- a/mozilla/js/src/symverscript.in	2012-11-07 09:36:16.000000000 -0500
>+++ b/mozilla/js/src/symverscript.in	2012-11-07 09:37:05.000000000 -0500
>@@ -0,0 +1,5 @@
>+#filter substitution
>+MOZJS_EXT {

You're not making any substitution here, which effectively will literally use "MOZJS_EXT" as the soversion.
Also, MOZJS_EXT is not defined in js/src without the standalone tarball patches (i guess that's where it comes from), which would break building as standalone without them (and we do that on our buildbots).

>+# ensure symbol versions of shared library on linux do not conflict with those in libxul
>+ifeq (Linux,$(OS_ARCH))
>+ifneq (Android,$(OS_TARGET))

ifeq (Linux,$(OS_TARGET)) is enough.

>+OS_LIBS += -lrt

Why add this? This seems unrelated.
Attachment #679705 - Flags: review?(mh+mozilla) → review-
(Assignee)

Comment 11

5 years ago
(In reply to Mike Hommey [:glandium] from comment #10)
> Comment on attachment 679705 [details] [diff] [review]
> Patch to merge to mozilla-central
> 
> >+#filter substitution
> >+@MOZ_APP_BASENAME@_@MOZ_APP_VERSION@ {
> >+	global: JS_*; js_*; _Z[NTVK0-9]*JS*; _Z[NTVK0-9]*js*;
> 
> Please put each on a separate line.

*nod*


> >--- a/mozilla/js/src/symverscript.in	2012-11-07 09:36:16.000000000 -0500
> >+++ b/mozilla/js/src/symverscript.in	2012-11-07 09:37:05.000000000 -0500
> >@@ -0,0 +1,5 @@
> >+#filter substitution
> >+MOZJS_EXT {
> 
> You're not making any substitution here, which effectively will literally
> use "MOZJS_EXT" as the soversion.
> Also, MOZJS_EXT is not defined in js/src without the standalone tarball
> patches (i guess that's where it comes from), which would break building as
> standalone without them (and we do that on our buildbots).
> 

MOZJS_EXT is entirely arbitrary, i chose it because it seemed an appropriate string for the standalone library.  As long as it is different from "@MOZ_APP_BASENAME@_@MOZ_APP_VERSION@" for every mozilla product this is all that matters.  It has no relation to the sources.  As mentioned in the comments when i posted this patch, there is currently no appropriate spidermonkey version identifiers within configure.in in js/src , so there is nothing to use for an appropriate substutition.  I added the #filter substitution directive just to supress the WARNING from Preprocessor.py


> >+# ensure symbol versions of shared library on linux do not conflict with those in libxul
> >+ifeq (Linux,$(OS_ARCH))
> >+ifneq (Android,$(OS_TARGET))
> 
> ifeq (Linux,$(OS_TARGET)) is enough.
> 

Sure.

> >+OS_LIBS += -lrt
> 
> Why add this? This seems unrelated.

..this is probably cruft I accidentally copied from the patch against the JS185 distfile.  I'll remove it.
(In reply to Ian Stakenvicius from comment #11)
> MOZJS_EXT is entirely arbitrary, i chose it because it seemed an appropriate
> string for the standalone library.  As long as it is different from
> "@MOZ_APP_BASENAME@_@MOZ_APP_VERSION@" for every mozilla product this is all
> that matters.  It has no relation to the sources.  As mentioned in the
> comments when i posted this patch, there is currently no appropriate
> spidermonkey version identifiers within configure.in in js/src , so there is
> nothing to use for an appropriate substutition.

But then, if two system libraries pull a different version of libmozjs, they will both have the same soversion, and we're back to square one (of if firefox is built with libmozjs as a shared library, which is what i do in debian, for instance)

> I added the #filter substitution directive just to supress the WARNING from Preprocessor.py

Note that if you don't need substitution, then you can just use the file without preprocessing it. But you need substitution anyways, to use some kind of version for the soversion.
(Assignee)

Comment 13

5 years ago
(In reply to Mike Hommey [:glandium] from comment #12)
> (In reply to Ian Stakenvicius from comment #11)
> > MOZJS_EXT is entirely arbitrary, i chose it because it seemed an appropriate
> > string for the standalone library.  As long as it is different from
> > "@MOZ_APP_BASENAME@_@MOZ_APP_VERSION@" for every mozilla product this is all
> > that matters.  It has no relation to the sources.  As mentioned in the
> > comments when i posted this patch, there is currently no appropriate
> > spidermonkey version identifiers within configure.in in js/src , so there is
> > nothing to use for an appropriate substutition.
> 
> But then, if two system libraries pull a different version of libmozjs, they
> will both have the same soversion, and we're back to square one (of if
> firefox is built with libmozjs as a shared library, which is what i do in
> debian, for instance)
> 

Yep, that's a problem.  And I don't think that this case (two copies of libmozjs*.so) is supportable at all -- you are bound to only having one copy of the libmozjs###.so shared lib and all must link to that.  This bug and patch only provides the solution for collision between libmozjs###.so and libxul.so (mainly in terms of how it's built and distributed in binary form direct from mozilla).

To fix this, there needs to be some versioning defined and implemented/added to configure.in so that it can be substituted appropriately in the first place.  Proper SONAME based versioning wouldn't hurt, either.  That's beyond the scope of this bug/patch though as its something the dev team in charge of js/src need to sort out.
(In reply to Ian Stakenvicius from comment #13)
> Yep, that's a problem.  And I don't think that this case (two copies of
> libmozjs*.so) is supportable at all -- you are bound to only having one copy
> of the libmozjs###.so shared lib and all must link to that.

It's bound to happen when you transition from libmozjs version x to libmozjs version x+1. You won't necessarily have updated all packages that depend on version x.
(Assignee)

Comment 15

5 years ago
(In reply to Mike Hommey [:glandium] from comment #14)
> (In reply to Ian Stakenvicius from comment #13)
> > Yep, that's a problem.  And I don't think that this case (two copies of
> > libmozjs*.so) is supportable at all -- you are bound to only having one copy
> > of the libmozjs###.so shared lib and all must link to that.
> 
> It's bound to happen when you transition from libmozjs version x to libmozjs
> version x+1. You won't necessarily have updated all packages that depend on
> version x.

I'm not saying this wouldn't be nice and good; I'm saying that, based on how things are established within the codebase in mozilla-central, that this isn't a supported option.  

As there hasn't been any releases of spidermonkey since 1.8.5 (libmozjs185), there isn't any established history to fall back on (previous versions don't count as they had proper soname and were also at least API-compatible with one-another).  I can say that Gentoo doesn't support having multiple spidermonkeys installed concurrently, and new versions of spidermonkey are masked (excluded from the package manager) until everything that depends on it can build against the new version; too much breakage occurs otherwise.  I would expect with a binary distro the issue would be the same -- that is, everything for a current "version" would need to be built homogeneously against the same libmozjs and mixing between "versions" of the distro would not be supported.

I very much agree it SHOULD be supported.  But as I said, supporting this requires the dev's in charge to make a design decision on how they're going to implement versioning in js/src, that imo is beyond the scope of this bug.  Along similar lines, since there doesn't seem to be a release of spidermonkey on the horizon (bug 735599 has had no movement in some time), getting the patch into place as it stands and as soon as possible (ie for FF17) would still be a significant improvement over the status-quo.  

If you prefer, though, I can make something up and propose it in the next patch??
(Assignee)

Comment 16

5 years ago
Created attachment 680384 [details] [diff] [review]
Patch to merge to mozilla-central

Update, incorporating requested changes.

I changed js/src/symverscript.in so that it substitutes LIBRARY_NAME -- after substitution the result will be identical to the previous patch, but when diff'ing Makefile.in between mozilla-central and the js185 distfile, LIBRARY_VERSION does differ (it is "mozjs185" in the distfile); so theoretically this variable is what will be updated to differentiate the library version for every release.
Attachment #679705 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #680384 - Flags: review?(mh+mozilla)

Updated

5 years ago
Duplicate of this bug: 809150
(Assignee)

Comment 18

5 years ago
Created attachment 681995 [details] [diff] [review]
Patch to merge to mozilla-central

fixed the syntax of the symbol version script for libxul (other one worked but was not the same form that i since found in other packages)
Attachment #680384 - Attachment is obsolete: true
Attachment #680384 - Flags: review?(mh+mozilla)
Attachment #681995 - Flags: review?(mh+mozilla)
Comment on attachment 681995 [details] [diff] [review]
Patch to merge to mozilla-central

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

I'd like someone from the js team to look at the list of symbols from mozjs we'd export with versions, whether that's enough or if there are other patterns to add.

::: mozilla/toolkit/library/symverscript.in
@@ +1,2 @@
> +#filter substitution
> +@MOZ_APP_BASENAME@_@MOZ_APP_VERSION@ {

It would probably be better to make that XUL_@MOZ_APP_VERSION@ or GECKO_@MOZ_APP_VERSION@.

@@ +1,3 @@
> +#filter substitution
> +@MOZ_APP_BASENAME@_@MOZ_APP_VERSION@ {
> +global: JS_*;

Can you put "JS_*" on a separate line, and indent the remaining items?

@@ +2,5 @@
> +@MOZ_APP_BASENAME@_@MOZ_APP_VERSION@ {
> +global: JS_*;
> +js_*;
> +_Z[NTVK0-9]*JS*;
> +_Z[NTVK0-9]*js*;

While looking in ld documentation what characters are legal for versions, I found out that we could use something like:
extern "C++" {
  js::*;
  JS::*;
};

which would make things much clearer. Can you test this works?
Attachment #681995 - Flags: review?(mh+mozilla)
Attachment #681995 - Flags: review?(dmandelin)
Attachment #681995 - Flags: review+
(Assignee)

Comment 20

5 years ago
(In reply to Mike Hommey [:glandium] from comment #19)
> 
> I'd like someone from the js team to look at the list of symbols from mozjs
> we'd export with versions, whether that's enough or if there are other
> patterns to add.
> [ Snip! ]
> @@ +2,5 @@
> > +@MOZ_APP_BASENAME@_@MOZ_APP_VERSION@ {
> > +global: JS_*;
> > +js_*;
> > +_Z[NTVK0-9]*JS*;
> > +_Z[NTVK0-9]*js*;
> 
> While looking in ld documentation what characters are legal for versions, I
> found out that we could use something like:
> extern "C++" {
>   js::*;
>   JS::*;
> };
> 
> which would make things much clearer. Can you test this works?

In theory it does work, sort of.  A symverscript in the spidermonkey-1.8.5 codebase will get all but four symbols via ' global: js_*; JS_*; extern "C++" { JS*; js*; }; '   , plus those symbols it doesn't get also does not exist in libxul from FF16.0.2 codebase.

However, using that match for js/src in the FF16.0.2 codebase leaves 93 symbols that aren't caught.  The original match isn't perfect either though as it leaves 24 symbols unmatching.

Also something to consider is that ' extern "C++" { JS*; js*; }; ' for libxul.so seems to match most (maybe all) of the symbols from js/jsd , which are unnecessary.

In short, I can't find a nice simple way to match these symbols.  I'm now looking into finding a way to generate symverscript from code so that it contains the full list of symbols from js/src; barring that I recommend versioning all symbols in both libxul and libmozjs with a 'global: *' , despite the additional file size it will add to libxul (which seems to be close to 2MB).


> ::: mozilla/toolkit/library/symverscript.in
> @@ +1,2 @@
> > +#filter substitution
> > +@MOZ_APP_BASENAME@_@MOZ_APP_VERSION@ {
> 
> It would probably be better to make that XUL_@MOZ_APP_VERSION@ or
> GECKO_@MOZ_APP_VERSION@.
> 

Probably substituting @LIBRARY_NAME@ for both js/src/symverscript.in and toolkit/library/symverscript.in would be a good fit.
(Assignee)

Comment 21

5 years ago
Given the issues with trying to match all mozjs symbols; and 

Given that whether there is symbol versioning on one symbol or all of them, the stripped libxul.so is identical in size (ELF,linux,gcc-4.5 toolchain);

Request that toolkit/library/symverscript.in include "global: *;" and be done with it.  

Friendly?
(Assignee)

Comment 22

5 years ago
Created attachment 687085 [details] [diff] [review]
add symbol-versions to libxul and libmozjs for linux (m-c and tagged releases)

per above comments:

 - substitute LIBRARY_NAME instead of MOZ_APP_NAME

 - version all symbols because matching just the JS ones are proving to be impossible in the generic sense and there seems to be no drawback to versioning all of them versus just a few.

Did not add dmandelin to review because all symbols are versioned so not much need to evaluate if it's a complete list or not
Attachment #681995 - Attachment is obsolete: true
Attachment #681995 - Flags: review?(dmandelin)
Attachment #687085 - Flags: review?(mh+mozilla)
(Assignee)

Comment 23

5 years ago
Comment on attachment 679299 [details] [diff] [review]
Updated patch to FF (etc) acceptable for submission

main patch obsoletes this one
Attachment #679299 - Attachment is obsolete: true
(Assignee)

Comment 24

5 years ago
Created attachment 687267 [details] [diff] [review]
add symbol-versions to libxul and libmozjs for linux (m-c and tagged releases)

minor update to symbol version definitions to fix a potential syntax error (it didn't play nice with the proposed patch for bug 812265, now it'll apply clean and permanent)
Attachment #687085 - Attachment is obsolete: true
Attachment #687085 - Flags: review?(mh+mozilla)
Attachment #687267 - Flags: review?(mh+mozilla)
Comment on attachment 687267 [details] [diff] [review]
add symbol-versions to libxul and libmozjs for linux (m-c and tagged releases)

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

::: mozilla/toolkit/library/symverscript.in
@@ +123,5 @@
> +EXTRA_DSO_LDOPTS += -Wl,-version-script,symverscript
> +
> +symverscript: symverscript.in
> +	$(PYTHON) $(topsrcdir)/config/Preprocessor.py \
> +		-DLIBRARY_NAME="$(subst -,_,$(LIBRARY_NAME))" \

It would be better with something like -DVERSION="$(LIBRARY_NAME)$(MOZILLA_VERSION)" here and -DVERSION="$(subst -,_,$(LIBRARY_NAME))" in js/src.

(no need for the subst in toolkit/library, LIBRARY_NAME is either xul or XUL)

Other than this nit, this is good for me, after carefully testing the effects of such a version script.
Attachment #687267 - Flags: review?(mh+mozilla) → review+
(Assignee)

Comment 26

5 years ago
Created attachment 687787 [details] [diff] [review]
add symbol-versions to libxul and libmozjs for linux (m-c and tagged releases) nit free

same patch with the versioning adjusted to address the nit.
Attachment #687267 - Attachment is obsolete: true
Attachment #687787 - Flags: review+
Attachment #687787 - Flags: feedback?(anarchy)

Comment 27

5 years ago
Comment on attachment 687787 [details] [diff] [review]
add symbol-versions to libxul and libmozjs for linux (m-c and tagged releases) nit free

--- a/mozilla/toolkit/library/Makefile.in	2012-11-07 11:08:34.000000000 -0500
+++ b/mozilla/toolkit/library/Makefile.in	2012-11-07 11:08:15.000000000 -0500
@@ -120,6 +120,14 @@
 ifeq (Linux,$(OS_ARCH))
 ifneq (Android,$(OS_TARGET))
 OS_LIBS += -lrt
+EXTRA_DSO_LDOPTS += -Wl,-version-script,symverscript
+
+symverscript: symverscript.in
+	$(PYTHON) $(topsrcdir)/config/Preprocessor.py \
+		-DVERSION="$(LIBRARY_NAME)$(MOZILLA_VERSION)" $< > $@
+
+EXTRA_DEPS += symverscript
+
 endif
 endif

Try builds have been run for all everything green other then reftest failure not related to patch.
Attachment #687787 - Flags: feedback?(anarchy) → feedback+

Updated

5 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 28

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/75e0d521bc2b

Updated

5 years ago
Depends on: 819172
I backed out https://hg.mozilla.org/integration/mozilla-inbound/rev/75e0d521bc2b due to the bug 819172 regression.  Clearly none of us there had any idea what the right fix is, best to punt back to the original author.  :-)

FYI, the steps to reproduce the issue reported there go like this.  Have a clean tree with that rev in it, then:

cd js/src
autoconf-2.13
mkdir objdir
cd objdir
../configure --enable-debug --disable-optimize
make
(Assignee)

Comment 30

5 years ago
Yeah I don't know why this happens.  'make all' works as expected, but 'make' on it's own doesn't.  Glandium mentioned that this was a bug that was already fixed, but as far as I can tell, the default target needs to be set in the makefile.

Note that this works fine in 17ESR but not in HEAD..
(Assignee)

Comment 31

5 years ago
Created attachment 690106 [details] [diff] [review]
add symbol versions and have correct target location in Makefile.in to support default 'make'

Same patch, but doesn't override the default target for js/src's Makefile in HEAD; allows patch to be applied again without causing bug 819172 to re-occur
Attachment #687787 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Whiteboard: commit-needed
(Assignee)

Comment 32

5 years ago
ACK.  No, this doesn't work -- symverscript target currently fails to build when patch is used against latest m-c, so all builds will fail.

Hold off pls, i'll figure it out.
Whiteboard: commit-needed

Comment 33

5 years ago
In the future, you probably want to "checkin-needed" keyword.
(Assignee)

Comment 34

5 years ago
Created attachment 690166 [details] [diff] [review]
add symbol versions and have correct target location in Makefile.in to support default 'make'

OK, the solution seems to be to make the symverscript target always be present at the end of the file, while placing the LDOPTS addition and EXTRA_DEPS inclusion where it originally was in the first patch.

It worked locally, but if glandium or another dev can confirm before checking it in again I would appreciate it.  I'd rather not generate another bug if possible.
Attachment #690106 - Attachment is obsolete: true
Attachment #690166 - Flags: review?(mh+mozilla)
(Assignee)

Comment 35

5 years ago
Created attachment 690168 [details] [diff] [review]
add symbol versions and have correct target location in Makefile.in to support default 'make'

(swapped XUL_ for @LIBRARY_VERSION@ -- accidental regression)
Attachment #690166 - Attachment is obsolete: true
Attachment #690166 - Flags: review?(mh+mozilla)
Attachment #690168 - Flags: review?(mh+mozilla)
Comment on attachment 690168 [details] [diff] [review]
add symbol versions and have correct target location in Makefile.in to support default 'make'

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

If you put the rule after the default rule, and before including rules.mk, you can group the rule and the flags.
Attachment #690168 - Flags: review?(mh+mozilla) → review+
Blocks: 837921
https://hg.mozilla.org/integration/mozilla-inbound/rev/7c536499e157
https://hg.mozilla.org/mozilla-central/rev/7c536499e157
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
I think this has broken |make -C toolkit/library| for me.

With this patch,

> $ rm -rf toolkit/library/libxul.so
> $ make -C toolkit/library
> make: Entering directory `/home/jlebar/code/moz/ff-git/debug/toolkit/library'
> make: `symverscript' is up to date.
> make: Leaving directory `/home/jlebar/code/moz/ff-git/debug/toolkit/library'

without this patch, that works fine.

We should probably back this out, unless there's a new way to build toolkit/library.

Comment 40

4 years ago
'make libs -C toolkit/library' works.  (It would still be nice to fix 'make -C toolkit/library', though.)
> 'make libs -C toolkit/library' works.

That's good enough for me!  Thanks.
I just wasted a ton of time because my normal workflow was not remaking libxul :-(.

This has also broken the new smartmake functionality of "mach build <dir>".
I mailed dev-platform so that hopefully nobody else ends up wasting time because of this.
Created attachment 745374 [details] [diff] [review]
fix building in toolkit/library

Or we could just fix it.
Attachment #745374 - Flags: review?(mbrubeck)
Comment on attachment 745374 [details] [diff] [review]
fix building in toolkit/library

I'm going to pass rubber-stamping duties off to gps here.
Attachment #745374 - Flags: review?(mbrubeck) → review?(gps)
Comment on attachment 745374 [details] [diff] [review]
fix building in toolkit/library

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

Gotta love ordering of rules in make files.

::: toolkit/library/Makefile.in
@@ +695,5 @@
>  libs:: $(FINAL_TARGET)/dependentlibs.list
>  
>  $(FINAL_TARGET)/dependentlibs.list: dependentlibs.py $(SHARED_LIBRARY) $(wildcard $(if $(wildcard $(FINAL_TARGET)/dependentlibs.list),$(addprefix $(FINAL_TARGET)/,$(shell cat $(FINAL_TARGET)/dependentlibs.list))))
>  	$(PYTHON) $< $(SHARED_LIBRARY) -L $(FINAL_TARGET) $(if $(TOOLCHAIN_PREFIX),$(addprefix -p ,$(TOOLCHAIN_PREFIX))) > $@
> +	

Nit: Whitespace
Attachment #745374 - Flags: review?(gps) → review+
Comment on attachment 745374 [details] [diff] [review]
fix building in toolkit/library

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

Please don't land this. Please instead file a new bug, and figure out why .DEFAULT_GOAL is not working in toolkit/library.
Attachment #745374 - Flags: feedback-
I reopened bug 777379 because our .DEFAULT_GOAL usage appears to be wrong.

The way I see it, this regression is high developer impact and a fix is available in this bug. I'm not sure we'll fix bug 777379 quickly.

Therefore, you have my permission (as build module owner) to land this trivial fix.
(In reply to Gregory Szorc [:gps] from comment #48)
> The way I see it, this regression is high developer impact and a fix is
> available in this bug. I'm not sure we'll fix bug 777379 quickly.

A patch is up already.
Depends on: 927073
You need to log in before you can comment on or make changes to this bug.