846986 - JSContext::updateMallocCounter undefined link failures

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Steve Singer (:stevensn)]

I am getting an undefined symbol error when my firefox build  tries to link programs against libmozjs.so when building test executables in xpcom/tests

JSContext::updateMallocCounter(unsigned int)


Executing: /usr/bin/ccache c++ -Wall -Wpointer-arith -Woverloaded-virtual -Werror=return-type -Wtype-limits -Wempty-body -Wno-invalid-offsetof -Wcast-align -fno-exceptions -fno-strict-aliasing -fno-rtti -ffunction-sections -fdata-sections -fno-exceptions -fshort-wchar -pthread -pipe -DNDEBUG -DTRIMMED -g -Os -freorder-blocks -fomit-frame-pointer -o nsIFileEnumerator /media/nfs/usb_drive_src/firefox/mozilla-central-hg/src/build-output/xpcom/tests/tmpFgtTt8.list -lpthread -Wl,-z,noexecstack -Wl,--build-id -Wl,-rpath-link,/media/nfs/usb_drive_src/firefox/mozilla-central-hg/src/build-output/dist/bin -Wl,-rpath-link,/usr/local/mozilla21/lib -L../../dist/bin -L../../dist/lib -ldl -L/media/nfs/usb_drive_src/firefox/mozilla-central-hg/src/build-output/dist/bin -lxpcom -lmozalloc -lxul ../../dist/lib/libxpcomglue_s.a -L/media/nfs/usb_drive_src/firefox/mozilla-central-hg/src/build-output/dist/bin -lxpcom -lmozalloc -L/media/nfs/usb_drive_src/firefox/mozilla-central-hg/src/build-output/dist/lib -lnspr4 -lplc4 -lplds4 ../../dist/lib/libmozjs.so -Wl,--whole-archive ../../dist/lib/libmozglue.a ../../dist/lib/libmemory.a -Wl,--no-whole-archive -rdynamic -ldl
/media/nfs/usb_drive_src/firefox/mozilla-central-hg/src/build-output/xpcom/tests/tmpFgtTt8.list:
    INPUT("nsIFileEnumerator.o")

../../dist/lib/libmozjs.so: undefined reference to `JSContext::updateMallocCounter(unsigned int)'

This is happening in mozilla-central I think this was caused by the patches in bug 836524 the implementation of JSContext::updateMallocCounter(size_t nbytes) was moved from within the class definition inside of jscntx.h to a function marked inline in jscntxtinlines.h

Comment 1

[Steve Singer (:stevensn)]

Attachment: This patch makes the problem go away but might not be the best fix

The attached patch will ensure that JSContext::updateMallocCounter(size_t) does get included in the shared library.   Without this patch nm on libmozjs.so shows updateMallocCounter(size_t) as being undefined.

I don't think this patch is the correct solution to the problem I am attaching it to show that the problem is that otherwise updateMallocCounter doesn't get included in libmozjs but the undefined symbol later causes issues in linking

Comment 2

[Steve Singer (:stevensn)]

Attachement 720222 lets me link with g++ 4.4.5 but didn't fix the issue on another machine that has a newer g++ (4.7.x ?)

Comment 3

[Steve Singer (:stevensn)]

Attachment: include headers with inline definitions in SPSProfiler

I've done some more investigating on this

I get the undefined symbols on my debian ppc32 machine with g++ 4.4.5-8.
I do not have the problems(even without the patches) on a fedora19 ppc64 machine with g++ 4.7.2 20121109 

The undefined reference is introduced in SPSProfiler.o

nm SPSProfiler.o |grep _ZN9JSContext19updateMallocCounterEm
                 U _ZN9JSContext19updateMallocCounterEm

This stays shows up as undefined on both of the platforms I mention.

nm libmozjs.so |grep _ZN9JSContext19updateMallocCounterEm
                 U _ZN9JSContext19updateMallocCounterEm

On one machine this prevents anything from linking with libmozjs but on the other I have no issues. 

SPSProfiler.cpp includes jscntxt.h but does not include jscntxtinlines.h 
Pulling in the inlines makes this issue go away.  The attached patch does this

Comment 4

[Steve Singer (:stevensn)]

https://hg.mozilla.org/mozilla-central/rev/489ab986ea69
Seems to fix this issue for me as well.  I can build more recent versions of mozilla-central without needing this patch.

Jory, can confirm that the above commit fixes this issue for you as well. If so we can mark this as resolved.

Comment 5

[Jeff Walden [:Waldo]]

Yeah, that would have fixed it.  For the triviality of it, it didn't seem worth bugspamming/reviewspamming people.  Sorry for not noticing this bug had already been filed, and for not associating the patch with it.  :-(

Comment 6

[Jory A. Pratt [:anarchy]]

(In reply to Steve Singer (:stevensn) from comment #4)
> https://hg.mozilla.org/mozilla-central/rev/489ab986ea69
> Seems to fix this issue for me as well.  I can build more recent versions of
> mozilla-central without needing this patch.
> 
> Jory, can confirm that the above commit fixes this issue for you as well. If
> so we can mark this as resolved.

Well this would fix the problem we need to get it back ported onto beta branch so it will be available in 21.0

Comment 7

[Jan de Mooij [:jandem]]

Comment on attachment 726388 [details] [diff] [review]
include headers with inline definitions in SPSProfiler

Resetting review flag per comment 4 (the problem has been fixed already).

Comment 8

[Steve Singer (:stevensn)]

Attachment: patch as committed

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: build failures on no-jit configurations
Testing completed (on m-c, etc.):  Patch has been fine on m-c for a while. 
Risk to taking this patch (and alternatives if risky):  low risk, #include file only change
String or IDL/UUID changes made by this patch:

Comment 9

[bhavana bajaj [:bajaj]]

(In reply to Steve Singer (:stevensn) from comment #8)
> Created attachment 741092 [details] [diff] [review]
> patch as committed
> 
> [Approval Request Comment]
> Bug caused by (feature/regressing bug #): 
> User impact if declined: build failures on no-jit configurations
> Testing completed (on m-c, etc.):  Patch has been fine on m-c for a while. 
> Risk to taking this patch (and alternatives if risky):  low risk, #include
> file only change
> String or IDL/UUID changes made by this patch:

Can you give more information on what versions are affected ? Has this always been broken or is it a recent regression & the regressing bug# ? Is Fx22(aurora) not affected here ? 

Also the review was cancelled in comment 7 as the issue is fixed . It will need a review to land on branches.

Comment 10

[Jory A. Pratt [:anarchy]]

Attachment: backported patch

Well we still need review in order to get permission to land on mozilla-beta, I have not checked aurora to see if it is effected as well.

Comment 11

[Jory A. Pratt [:anarchy]]

Comment on attachment 741092 [details] [diff] [review]
patch as committed

backported patch will need to be used, no point in review on m-c patch.

Comment 12

[Jory A. Pratt [:anarchy]]

Comment on attachment 741557 [details] [diff] [review]
backported patch

Aurora is effected as well.

Comment 13

[bhavana bajaj [:bajaj]]

(In reply to Jory A. Pratt from comment #12)
> Comment on attachment 741557 [details] [diff] [review]
> backported patch
> 
> Aurora is effected as well.

Is this a regression from some new feature or bug # ?

Comment 14

[bhavana bajaj [:bajaj]]

Comment on attachment 741557 [details] [diff] [review]
backported patch

Bug 836524 , is the regressing bug here.

The patch is very low risk,tested on m-c and looks good to land

Comment 16

[Landry Breuil (:gaston)]

Fwiw, it definitely fixed the build of 21.0b4 for me on OpenBSD/ppc.

Comment 17

[ricardoquesada]

Thanks for fixing it.
With the patch, it works OK on iOS Simulator.

What's strange is that it worked OK (without the patch) for iOS device (ARMv7 without JIT), but it didn't link on iOS simulator (i386 without JIT).

Comment 18

[Landry Breuil (:gaston)]

Seems this was never backported to aurora/beta at that time, which shifted to beta/release. So 21.0 might have shipped with that problem, gotta check it on a ppc before reasking for proper approval.

Comment 19

[Landry Breuil (:gaston)]

It seems 21.0 final builds fine on ppc at least under js/. strange. Will test beta, which was aurora at that time.

Comment 20

[Landry Breuil (:gaston)]

Nah, after a long & painful build, it failed to link libxul :

../../dist/bin/libxul.so.40.0: undefined reference to `JSContext::updateMallocCounter(unsigned long)'

So 21 is still affected, and likely 22...

Comment 21

[Landry Breuil (:gaston)]

For what's now in beta, the error looks different since the js shell itself doesnt link on ppc:

../libjs_static.a(SPSProfiler.o)(.text._ZN2js11SPSProfiler18allocProfileStringEP9JSContextP8JSScriptP10JSFunction+0x190): In fu
nction `js::SPSProfiler::allocProfileString(JSContext*, JSScript*, JSFunction*)':
./../../dist/include/js/Vector.h:620: undefined reference to `JSScript::filename() const'
../libjs_static.a(SPSProfiler.o)(.text._ZN2js11SPSProfiler18allocProfileStringEP9JSContextP8JSScriptP10JSFunction+0x1a0):./../.
./dist/include/js/Vector.h:167: undefined reference to `JSScript::filename() const'
../libjs_static.a(SPSProfiler.o)(.text._ZN2js11SPSProfiler18allocProfileStringEP9JSContextP8JSScriptP10JSFunction+0x1ac):./../.
./dist/include/js/Vector.h:166: undefined reference to `JSScript::filename() const'

Trying the same fix allows the js shell to link, i'll see in a few hours how it goes for libxul/firefox itself..

Comment 22

[Landry Breuil (:gaston)]

Comment on attachment 741557 [details] [diff] [review]
backported patch

[Approval Request Comment]
Regression caused by (bug #): 
User impact if declined: 
Testing completed (on m-c, etc.): 
Risk to taking this patch (and alternatives if risky):
String or IDL/UUID changes made by this patch:

Reasking for approval, this time for beta & release in case there's a 21 chemspill. Confirmed to fix the issue on OpenBSD/macppc for both branches.

Comment 23

[Alex Keybl [:akeybl]]

Has this landed anywhere or had a try build run of it? That's a prerequisite for landing to beta.

Comment 24

[Landry Breuil (:gaston)]

Oh sorry, yeah it's in central & aurora, since this landed on central at that time in 489ab986ea69 (which is now in aurora), got approval but i totally forgot to push it.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/24b0907a5df4

Comment 26

[Landry Breuil (:gaston)]

thanks !

Comment 27

[u279076]

Steve, this should now be fixed in Firefox 22 and 23 builds. Can you please confirm?

Comment 28

[Landry Breuil (:gaston)]

Fwiw, i'm running 23.0b1 on OpenBSD/macppc, and it built unpatched. Same case for 22.0.

Comment 29

[Steve Singer (:stevensn)]

I have verified this works fine on an unpatched ff 23 build.
I am just starting a new ff 22 build and will update the bug when it is finished (hopefully tonight)

Comment 30

[Steve Singer (:stevensn)]

This works on ff 22 as well

Comment 31

[u279076]

Verified fixed based on comment 29 and 30. Thanks for your help Steve.