Bug 988619 broke non-ion platforms

VERIFIED FIXED in Firefox 30

Status

()

defect
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: gaston, Assigned: stevensn)

Tracking

unspecified
mozilla30
Sun
OpenBSD
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox29 unaffected, firefox30+ verified, firefox31+ unaffected, firefox32+ unaffected, b2g-v1.4 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

Reporter

Description

5 years ago
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);
Reporter

Updated

5 years ago
Blocks: 988619

Updated

5 years ago
Duplicate of this bug: 999402
Assignee

Comment 2

5 years ago
Posted patch 997992_fix_nonion.diff (obsolete) — Splinter Review
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.
Attachment #8413415 - Flags: review?(jdemooij)

Comment 3

5 years ago
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.
Assignee

Comment 4

5 years ago
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

5 years ago
CompileInfo.h (which pulls Registers.h) doesn't seem to be used in ExecutionMode-inl.h.
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.
Attachment #8413415 - Flags: review?(jdemooij)
Assignee

Comment 7

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

Comment 8

5 years ago
 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?
Flags: needinfo?(jdemooij)
(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 :)
Flags: needinfo?(jdemooij)
Assignee

Comment 10

5 years ago
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
Attachment #8413415 - Attachment is obsolete: true
Attachment #8416557 - Flags: review?(jdemooij)
Attachment #8416557 - Flags: review?(jdemooij) → review+
Reporter

Comment 11

5 years ago
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 ?
Attachment #8416557 - Flags: approval-mozilla-beta?
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?
Attachment #8416557 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(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
Assignee: nobody → steve
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Reporter

Comment 15

5 years ago
Firefox 30.0b2 builds _and_ runs fine on sparc64, so marking as VERIFIED.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.