Closed Bug 846986 Opened 8 years ago Closed 7 years ago

JSContext::updateMallocCounter undefined link failures

Categories

(Core :: JavaScript Engine, defect)

PowerPC
Linux
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla23
Tracking Status
firefox21 --- wontfix
firefox22 --- verified
firefox23 --- verified

People

(Reporter: stevensn, Unassigned)

References

Details

Attachments

(2 files, 2 obsolete files)

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
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
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 ?)
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
Attachment #720222 - Attachment is obsolete: true
Attachment #726388 - Flags: review?(jdemooij)
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.
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.  :-(
Status: UNCONFIRMED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
(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 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).
Attachment #726388 - Flags: review?(jdemooij)
[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:
Attachment #726388 - Attachment is obsolete: true
Attachment #741092 - Flags: approval-mozilla-beta?
(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.
Attached patch backported patchSplinter Review
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.
Attachment #741557 - Flags: review?(jdemooij)
Attachment #741557 - Flags: approval-mozilla-beta?
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.
Attachment #741092 - Flags: approval-mozilla-beta?
Comment on attachment 741557 [details] [diff] [review]
backported patch

Aurora is effected as well.
Attachment #741557 - Flags: approval-mozilla-aurora?
Attachment #741557 - Flags: review?(jdemooij) → review+
(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 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
Attachment #741557 - Flags: approval-mozilla-beta?
Attachment #741557 - Flags: approval-mozilla-beta+
Attachment #741557 - Flags: approval-mozilla-aurora?
Attachment #741557 - Flags: approval-mozilla-aurora+
Duplicate of this bug: 866493
Fwiw, it definitely fixed the build of 21.0b4 for me on OpenBSD/ppc.
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).
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.
It seems 21.0 final builds fine on ppc at least under js/. strange. Will test beta, which was aurora at that time.
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...
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 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.
Attachment #741557 - Flags: approval-mozilla-release?
Attachment #741557 - Flags: approval-mozilla-beta?
Attachment #741557 - Flags: approval-mozilla-beta+
Attachment #741557 - Flags: approval-mozilla-aurora+
Has this landed anywhere or had a try build run of it? That's a prerequisite for landing to beta.
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.
Attachment #741557 - Flags: approval-mozilla-release?
Attachment #741557 - Flags: approval-mozilla-release-
Attachment #741557 - Flags: approval-mozilla-beta?
Attachment #741557 - Flags: approval-mozilla-beta+
thanks !
Steve, this should now be fixed in Firefox 22 and 23 builds. Can you please confirm?
Flags: needinfo?(steve)
Fwiw, i'm running 23.0b1 on OpenBSD/macppc, and it built unpatched. Same case for 22.0.
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)
This works on ff 22 as well
Flags: needinfo?(steve)
Verified fixed based on comment 29 and 30. Thanks for your help Steve.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.