990154 - Bug 981693 broke archs without ENABLE_ASSEMBLER

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Landry Breuil (:gaston)]

While building js shell on sparc64.... probably affects aurora too since that landed when 30 was in m-c. Couldnt find if already reported.

eg++ -o Unified_cpp_js_src_shell0.o -c  -fvisibility=hidden -DEXPORT_JS_API -DIMPL_MFBT -DMOZ_GLUE_IN_PROGRAM -DNO_NSPR_10_SUPPORT -I/home/buildslave/mozilla-central-sparc64/build/js/src/shell -I. -I/home/buildslave/mozilla-central-sparc64/build/js/src/shell/.. -I.. -I../../../dist/include  -I/data/obj/buildslave/m-c/dist/include/nspr    -I/home/buildslave/mozilla-central-sparc64/build/modules/zlib/src    -fPIC   -DMOZILLA_CLIENT -include ../../../js/src/js-confdefs.h -MD -MP -MF .deps/Unified_cpp_js_src_shell0.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 -fno-rtti -fno-exceptions -fno-math-errno -std=gnu++0x -pthread -pipe  -DNDEBUG -DTRIMMED -g -O -fomit-frame-pointer     /data/obj/buildslave/m-c/js/src/shell/Unified_cpp_js_src_shell0.cpp
In file included from /home/buildslave/mozilla-central-sparc64/build/js/src/shell/../jsscript.h:25:0,
                 from /home/buildslave/mozilla-central-sparc64/build/js/src/shell/../vm/Runtime.h:26,
                 from /home/buildslave/mozilla-central-sparc64/build/js/src/shell/../jscntxt.h:15,
                 from /home/buildslave/mozilla-central-sparc64/build/js/src/shell/js.cpp:41,
                 from /data/obj/buildslave/m-c/js/src/shell/Unified_cpp_js_src_shell0.cpp:2:
/home/buildslave/mozilla-central-sparc64/build/js/src/shell/../jit/IonCode.h:62:18: error: 'JSC::CodeKind' has not been declared
             JSC::CodeKind kind)
                  ^
/home/buildslave/mozilla-central-sparc64/build/js/src/shell/../jit/IonCode.h:139:57: error: 'JSC::CodeKind' has not been declared
                         JSC::ExecutablePool *pool, JSC::CodeKind kind);

Comment 1

[Steve Singer (:stevensn)]

Attachment: 990154_noassembler.diff

Does this patch let you build?

Can anyone see a reason why we can't include the ExecutableAllocator header for non assembler builds?

Comment 2

[Landry Breuil (:gaston)]

Testing it along #991966 and #980891 in http://buildbot.rhaalovely.net/builders/mozilla-central-sparc64/builds/761

Comment 3

[Landry Breuil (:gaston)]

It fails much later on a libcubeb test :
../../../dist/bin/libxul.so.1.0: undefined reference to `I422ToI420'
../../../dist/bin/libxul.so.1.0: undefined reference to `I444ToI420'
../../../dist/bin/libxul.so.1.0: undefined reference to `NV21ToI420'
../../../dist/bin/libxul.so.1.0: undefined reference to `NV12ToI420'

but thats probably a new unrelated failure :)

Comment 4

[Nicholas Nethercote [inactive]]

Comment on attachment 8402064 [details] [diff] [review]
990154_noassembler.diff

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

This doesn't feel right to me -- I think you only need CodeKind, but this change exposes ExecutablePool and ExecutableAllocator? I'm not sure... I think jandem would know better than me.

Comment 5

[Jan de Mooij [:jandem]]

Comment on attachment 8402064 [details] [diff] [review]
990154_noassembler.diff

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

I agree with njn. Can you move the CodeKind enum in ExecutableAllocator.h before the #if ENABLE_ASSEMBLER and see if that works?

namespace JS {
  enum CodeKind { ION_CODE = 0, BASELINE_CODE, REGEXP_CODE, OTHER_CODE };
}

That seems a bit simpler and less likely to introduce new problems.

Comment 6

[Landry Breuil (:gaston)]

Attachment: bug-990154-codekind

Will be tested in the next build:
http://buildbot.rhaalovely.net/builders/mozilla-central-sparc64/builds/764

(that one will probably fail during linking libxul with the message in comment 3 but at least we'll know if it fixes js)

Comment 7

[Steve Singer (:stevensn)]

I am able to build fine  with attachment 8403460 [details] [diff] [review] on linux ppc64 (also with the skia patches in bug 980891 applied).
I am not seeing the I422ToI420 issue.

Comment 8

[Jan Beich]

I422ToI420 is bug 981780. sparc, mips, aarch64, etc. and Solaris still default to --disable-webrtc.

Comment 9

[Landry Breuil (:gaston)]

Comment on attachment 8403460 [details] [diff] [review]
bug-990154-codekind

Lets try to get this to aurora through inbound and central first then...

Comment 10

[Jan de Mooij [:jandem]]

Comment on attachment 8403460 [details] [diff] [review]
bug-990154-codekind

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

Looks good, thanks.

Comment 11

[Landry Breuil (:gaston)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4d46754ac133

given that this is NPOTB (all tier1 platforms should have ENABLE_ASSEMBLER, no ?) i wonder if i should ask for approval-aurora or land it directly there...

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4d46754ac133

Comment 13

[Landry Breuil (:gaston)]

Comment on attachment 8403460 [details] [diff] [review]
bug-990154-codekind

Just in case, formal approval request even if NPOTB-tier1..

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 981693  
User impact if declined: Fx broken on non-asm archs
Testing completed (on m-c, etc.): Fixes build on sparc64
Risk to taking this patch (and alternatives if risky): Puts more stuff outside ENABLE_ASSEMBLER scope, but that's needed now non non-ENABLE_ASSEMBLER archs
String or IDL/UUID changes made by this patch: non

Comment 14

[Landry Breuil (:gaston)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/09f4010d6f77