997992 - Bug 988619 broke non-ion platforms

Status: VERIFIED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Landry Breuil (:gaston)]

Sigh. https://hg.mozilla.org/mozilla-central/rev/21aca7217e7a introduces jit::GetIonScript without #ifdef goop:

http://buildbot.rhaalovely.net/builders/mozilla-central-sparc64/builds/772/steps/build/logs/stdio

/home/buildslave/mozilla-central-sparc64/build/js/src/jsinfer.cpp: In member function 'void js::types::TypeZone::sweep(js::FreeOp*, bool, bool*)':
/home/buildslave/mozilla-central-sparc64/build/js/src/jsinfer.cpp:4337:21: error: 'GetIonScript' is not a member of 'js::jit'
                     jit::GetIonScript(script, output.mode())->recompileInfoRef() = uint32_t(-1);

Comment 2

[Steve Singer (:stevensn)]

Attachment: 997992_fix_nonion.diff

This patch gets things building on my non-ion builds(with bug 1002041 applied) and firefox seems to work.  I can't say for certain if this code would ever get called on a non-ion build.

Comment 3

[Jan Beich]

Alternatively, you can try to remove #ifdef JS_ION from js/src/jit/ExecutionMode-inl.h if maybeIonScript() being available with --disable-ion is not a bug.

Comment 4

[Steve Singer (:stevensn)]

Removing the #ifdef from ExecutionMode-inl.h does not work

c++ -o RegExp.o -c  -fvisibility=hidden -DENABLE_PARALLEL_JS -DENABLE_BINARYDATA -DENABLE_SHARED_ARRAY_BUFFER -DEXPORT_JS_API -DJS_HAS_CTYPES -DDLL_PREFIX='"lib"' -DDLL_SUFFIX='".so"' -DUSE_SYSTEM_MALLOC=1 -DENABLE_ASSEMBLER=1 -DMOZ_GLUE_IN_PROGRAM -DNO_NSPR_10_SUPPORT -DUSE_ZLIB -I/media/nfs/usb_drive_src/firefox/mozilla-central-hg/src/js/src -I. -I/media/nfs/usb_drive_src/firefox/mozilla-central-hg/src/js/src/../../mfbt/double-conversion -Ictypes/libffi/include -I/media/nfs/usb_drive_src/firefox/mozilla-central-hg/src/intl/icu/source/common -I/media/nfs/usb_drive_src/firefox/mozilla-central-hg/src/intl/icu/source/i18n -I../../dist/include  -I/home/ssinger/build-out.ppc64/dist/include/nspr    -I/media/nfs/usb_drive_src/firefox/mozilla-central-hg/src/modules/zlib/src    -fPIC   -DMOZILLA_CLIENT -include ../../js/src/js-confdefs.h -MD -MP -MF .deps/RegExp.o.pp  -Wall -Wpointer-arith -Woverloaded-virtual -Werror=return-type -Werror=int-to-pointer-cast -Wtype-limits -Wempty-body -Werror=conversion-null -Wsign-compare -Wno-invalid-offsetof -Wcast-align -fno-rtti -fno-exceptions -fno-math-errno -std=gnu++0x -pthread -pipe  -DNDEBUG -DTRIMMED -g -O3 -freorder-blocks  -fomit-frame-pointer     /media/nfs/usb_drive_src/firefox/mozilla-central-hg/src/js/src/builtin/RegExp.cpp
In file included from /media/nfs/usb_drive_src/firefox/mozilla-central-hg/src/js/src/jit/CompileInfo.h:12:0,
                 from /media/nfs/usb_drive_src/firefox/mozilla-central-hg/src/js/src/jit/ExecutionMode-inl.h:12,
                 from /media/nfs/usb_drive_src/firefox/mozilla-central-hg/src/js/src/jsinferinlines.h:27,
                 from /media/nfs/usb_drive_src/firefox/mozilla-central-hg/src/js/src/jsobjinlines.h:22,
                 from /media/nfs/usb_drive_src/firefox/mozilla-central-hg/src/js/src/builtin/RegExp.cpp:14:
/media/nfs/usb_drive_src/firefox/mozilla-central-hg/src/js/src/jit/Registers.h:22:3: error: #error "Unknown architecture!"
/media/nfs/usb_drive_src/firefox/mozilla-central-hg/src/js/src/jit/Registers.h:29:13: error: ?Registers? does not name a type

Comment 5

[Jan Beich]

Attachment: Expose inlines to make non-ION build behave slightly more like ION disabled at runtime. (alternative fix)

CompileInfo.h (which pulls Registers.h) doesn't seem to be used in ExecutionMode-inl.h.

Comment 6

[Jan de Mooij [:jandem]]

Comment on attachment 8413415 [details] [diff] [review]
997992_fix_nonion.diff

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

::: js/src/jsinfer.cpp
@@ +4360,4 @@
>      /* Sweep and find compressed indexes for each compiler output. */
>      size_t newCompilerOutputCount = 0;
>      if (compilerOutputs) {
>          for (size_t i = 0; i < compilerOutputs->length(); i++) {

I think it'd be nicer to add #ifdef JS_ION above this line, and add the #else MOZ_CRASH(); #endif after the loop.

compilerOutputs is allocated in FinishCompilation but that should never be called without Ion.

Comment 7

[Steve Singer (:stevensn)]

The alternative fix in 8413576 compiles and runs on my system (with pending patches from a few other bugs applied)

Comment 8

[Steve Singer (:stevensn)]

 Jan de Mooij,
Did you want me to submit a modified patch for jsinfer.cpp with the #ifdef moved or are you going to go ahead and get your patch reviewed and committed?

Comment 9

[Jan de Mooij [:jandem]]

(In reply to Steve Singer (:stevensn) from comment #8)
>  Jan de Mooij,
> Did you want me to submit a modified patch for jsinfer.cpp with the #ifdef
> moved or are you going to go ahead and get your patch reviewed and committed?

Please post an updated patch and I'll review it :)

Comment 10

[Steve Singer (:stevensn)]

Attachment: 997992_nonion_fix.diff

Attached is the updated patch.
This patch is only needed on mozilla-beta
https://hg.mozilla.org/mozilla-central/rev/aa534ca9cea5  actually included this fix on m-c before the merge to m-a

Comment 11

[Landry Breuil (:gaston)]

Comment on attachment 8416557 [details] [diff] [review]
997992_nonion_fix.diff

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 988619
User impact if declined: failure to build on non-ion platforms
Testing completed (on m-c, etc.): yes
Risk to taking this patch (and alternatives if risky): #ifdef goop

So since that #ifdef is already on aurora/central, lets land it on beta directly and be done with it ? Maybe even a=NPOTB ?

Comment 12

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 8416557 [details] [diff] [review]
997992_nonion_fix.diff

Go ahead and land to beta - but won't this be an issue when 31 gets to beta if we don't land on all branches?

Comment 13

[Ryan VanderMeulen [:RyanVM]]

(In reply to Lukas Blakk [:lsblakk] from comment #12)
> Go ahead and land to beta - but won't this be an issue when 31 gets to beta
> if we don't land on all branches?

This was fixed (or rendered moot) on 31/32 by bug 995336 (per comment 10).

https://hg.mozilla.org/releases/mozilla-beta/rev/5958a9259d6a

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/5958a9259d6a

Comment 15

[Landry Breuil (:gaston)]

Firefox 30.0b2 builds _and_ runs fine on sparc64, so marking as VERIFIED.