1046452 - Unsupported architecture in MacroAssembler.h

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Steve Singer (:stevensn)]

Bug 1042833 is removed #ifdef JS_ION  in favour of a 'none' architecture

On ppc we now get 


 8:33.98 In file included from /home/buildbot/buildbot/slave/runtests/build/js/src/jit/none/Architecture-none.h:10:0,
 8:33.98                  from /home/buildbot/buildbot/slave/runtests/build/js/src/jit/Registers.h:22,
 8:33.98                  from /home/buildbot/buildbot/slave/runtests/build/js/src/jit/Snapshots.h:17,
 8:33.98                  from /home/buildbot/buildbot/slave/runtests/build/js/src/jit/JitFrameIterator.h:15,
 8:33.98                  from /home/buildbot/buildbot/slave/runtests/build/js/src/vm/Stack.h:16,
 8:33.98                  from /home/buildbot/buildbot/slave/runtests/build/js/src/vm/Runtime.h:42,
 8:33.98                  from /home/buildbot/buildbot/slave/runtests/build/js/src/jscntxt.h:15,
 8:33.98                  from /home/buildbot/buildbot/slave/runtests/build/js/src/jit/IonAllocPolicy.h:13,
 8:33.98                  from /home/buildbot/buildbot/slave/runtests/build/js/src/jit/BytecodeAnalysis.h:11,
 8:33.98                  from /home/buildbot/buildbot/slave/runtests/build/js/src/jit/IonBuilder.h:13,
 8:33.98                  from /home/buildbot/buildbot/slave/runtests/build/js/src/jit/IonBuilder.cpp:7,
 8:33.98                  from /home/buildbot/buildbot/slave/runtests/build/obj-powerpc64-unknown-linux-gnu/js/src/Unified_cpp_js_src4.cpp:2:
 8:33.98 /home/buildbot/buildbot/slave/runtests/build/js/src/assembler/assembler/MacroAssembler.h:62:2: error: #error "The MacroAssembler is not supported on this platform."
 8:33.98  #error "The MacroAssembler is not supported on this platform."

Comment 1

[Brian Hackett [Laid off!]]

Attachment: patch

Does this patch fix the bustage for you?

I guess we still have two separate sets of macro assemblers, and two different mechanisms for deciding which assembler to use.  This patch makes the JSC assembler files avoid including an assembler implementation in JS_CODEGEN_NONE builds.  It also fixes some --disable-ion breakage resulting from some asm.js related changes yesterday.

Comment 2

[Jan de Mooij [:jandem]]

Comment on attachment 8465185 [details] [diff] [review]
patch

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

Filed bug 1046585 to merge assembler/ with jit/ and get rid of these JSC defines.

Comment 3

[Steve Singer (:stevensn)]

Comment on attachment 8465185 [details] [diff] [review]
patch

This patch won't be sufficient, 
I tried something similar (adding a new BaseMacroAsssembler) and it got further but I also had to fix an assert(IS_LITTLE_ENDIAN) in IonMacroAssembler.h

and now I'm getting

/js/src/jit/AsmJSSignalHandlers.cpp:362:55: error: ?PC_sig? was not declared in this scope

I won't be able to actually try your patch till tonight.

Comment 4

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dcf11265df87

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/dcf11265df87

Comment 6

[Steve Singer (:stevensn)]

js/src/jit/none/BaseMacroAssembler-none.h:20:17: error: ?static bool JSC::MacroAssemblerNone::supportsFloatingPoint()? is private

I think supportFloatingPoint needs to be public.

Comment 7

[Steve Singer (:stevensn)]

Attachment: 1046452_WIP.diff

Here are the changes so far.
I still don't yet compile (dealing with errors in MIR.h) 


I also doubt my change in AsmJSSignalHandlers.cpp is what we actually want

Comment 8

[Steve Singer (:stevensn)]

Attachment: 1046452.diff

An updated patch.
With this patch I am able to build and run firefox on ppc64.  My ppc32 build segfaults on startup (that might be unrelated to this bug).

The value I put in 
js/src/asmjs/AsmJSSignalHandlers.cpp
for PC_sig is probably wrong but I'm not yet sure what the correct thing is (and what will compile on the BSD's).  Other non-tier 1 archs will probably need more lines here as well (SPARC and what else?)

Comment 9

[Jan Beich]

Attachment: PC_sig for mips, ppc, sparc on BSDs

Mostly a guess after reading the source.

Comment 10

[Steve Singer (:stevensn)]

Attachment: 1046452.diff

I have merged in Jan's PC_sig patch into my larger patch.

Comment 11

[Brian Hackett [Laid off!]]

Comment on attachment 8469007 [details] [diff] [review]
1046452.diff

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

::: js/src/asmjs/AsmJSSignalHandlers.cpp
@@ +63,5 @@
> +#  define R15_sig(p) ((p)->sc_r15)
> +# endif
> +# define EPC_sig(p) ((p)->sc_pc)
> +# define TPC_sig(p) ((p)->sc_pc)
> +# define SRR0_sig(p) ((p)->sc_frame.srr0)

Are these _sig() changes just to get ContextToPC to compile?  If that's the case you can just change the body of that function from:

    return reinterpret_cast<uint8_t**>(&PC_sig(context));

to:

#ifdef JS_CODEGEN_NONE
    MOZ_CRASH();
#else
    return reinterpret_cast<uint8_t**>(&PC_sig(context));
#endif

We don't install the asm.js signal handlers when the JIT isn't supported, so it doesn't matter in that case what this method does.  Non trivial changes made to this file need to be testable and ppc or any other --disable-ion stuff in this file will never execute.

::: js/src/jit/IonMacroAssembler.cpp
@@ +1944,5 @@
>      // 16-bit loads are slow and unaligned 32-bit loads may be too so
>      // perform an aligned 32-bit load and adjust the bitmask accordingly.
>      JS_ASSERT(JSFunction::offsetOfNargs() % sizeof(uint32_t) == 0);
>      JS_ASSERT(JSFunction::offsetOfFlags() == JSFunction::offsetOfNargs() + 2);
> +#if IS_LITTLE_ENDIAN

Just change:

JS_STATIC_ASSERT(IS_LITTLE_ENDIAN);

to:

#if !IS_LITTLE_ENDIAN
MOZ_CRASH()
#endif

And avoid trying to make sure this code works right on little endian platforms, since (as above) the #if IS_LITTLE_ENDIAN code will never execute here.

You could also just remove the JS_STATIC_ASSERT entirely: there are plenty of other places in the JIT which will not work right on little endian platforms.

::: js/src/jit/IonMacroAssembler.h
@@ +519,4 @@
>          uint32_t bit = JSFunction::INTERPRETED << 16;
> +#else
> +        uint32_t bit = JSFunction::INTERPRETED ;        
> +#endif

ditto

@@ +532,4 @@
>          uint32_t bit = JSFunction::INTERPRETED << 16;
> +#else
> +        uint32_t bit = JSFunction::INTERPRETED ;
> +#endif

ditto

Comment 12

[Brian Hackett [Laid off!]]

(In reply to Brian Hackett (:bhackett) from comment #11)
> And avoid trying to make sure this code works right on little endian
> platforms, since (as above) the #if IS_LITTLE_ENDIAN code will never execute
> here.

Oops, got little and big endian confused here (another argument for making the code simple and avoiding complicated #ifdefs).

Comment 13

[Steve Singer (:stevensn)]

Cameron, 

I think you've mentioned in the past that you were looking into adding ION support for ppc.  Do you have any objection to brian's suggestions?

Comment 14

[Cameron Kaiser [:spectre]]

Assuming you mean https://bugzilla.mozilla.org/attachment.cgi?id=8465185 -- I don't even know if OS X/ppc's JIT still works after irregexp landed (I'm working on the merge right now), but since we set JS_CODEGEN_PPC and not JS_CODEGEN_NONE in our local changesets, I don't see a problem.

Comment 15

[Cameron Kaiser [:spectre]]

Also, looking at https://bug1046452.bugzilla.mozilla.org/attachment.cgi?id=8469007, that would take one of my endian patches I have already and haven't upstreamed, so that's good. I don't care about the AsmJS changes because I have it really hacked up in our build to get around certain problems (we remain Baseline only right now).

Comment 16

[Steve Singer (:stevensn)]

Attachment: 1046452.diff

I have updated the patch as requested by Brian in AsmJSSignalHandlers.cpp: 
to have the MOZ_CRASH in ContextToPC,

I have changed the IonMacroAssembler changes so they still should work on big endian (it sounds like that part Cameron will need) but I've created a new macro so the #ifdef's can be isolated.

Comment 17

[Steve Singer (:stevensn)]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=952d0aba28f5

Comment 18

[Brian Hackett [Laid off!]]

Comment on attachment 8470491 [details] [diff] [review]
1046452.diff

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

Thanks!  r=me with the _sig() changes removed per the notes below.

::: js/src/asmjs/AsmJSSignalHandlers.cpp
@@ +75,5 @@
> +#  define R15_sig(p) ((p)->sc_r15)
> +# endif
> +# define EPC_sig(p) ((p)->sc_pc)
> +# define TPC_sig(p) ((p)->sc_pc)
> +# define SRR0_sig(p) ((p)->sc_frame.srr0)

With the change to ContextToPC, this change should be unnecessary, right?  I suggested the change to ContextToPC so that you wouldn't need to touch these _sig() macros.

@@ +100,5 @@
>  # define R11_sig(p) ((p)->uc_mcontext.gregs[REG_R11])
>  # define R12_sig(p) ((p)->uc_mcontext.gregs[REG_R12])
>  # define R13_sig(p) ((p)->uc_mcontext.gregs[REG_R13])
>  # define R14_sig(p) ((p)->uc_mcontext.gregs[REG_R14])
> +# define TPC_sig(p) ((p)->uc_mcontext.gregs[REG_PC])

Ditto.

@@ +113,5 @@
>  #  define RFP_sig(p) ((p)->uc_mcontext.gregs[30])
>  # endif
> +# if defined(__linux__) && defined(__powerpc__)
> +#  define SRR0_sig(p) ((p)->uc_mcontext.regs[15])
> +# endif

Ditto.

@@ +136,5 @@
>  # define R14_sig(p) ((p)->uc_mcontext.__gregs[_REG_R14])
>  # define R15_sig(p) ((p)->uc_mcontext.__gregs[_REG_R15])
> +# define EPC_sig(p) ((p)->uc_mcontext.__gregs[_REG_EPC])
> +# define TPC_sig(p) ((p)->uc_mcontext.__gregs[_REG_PC])
> +# define SRR0_sig(p) ((p)->uc_mcontext.__gregs[_REG_PC])

Ditto.

@@ +167,5 @@
>  #  define R15_sig(p) ((p)->uc_mcontext.mc_r15)
>  # endif
> +# define EPC_sig(p) ((p)->uc_mcontext.mc_pc)
> +# define TPC_sig(p) ((p)->uc_mcontext._mc_tpc)
> +# define SRR0_sig(p) ((p)->uc_mcontext.mc_srr0)

Ditto.

Comment 19

[Steve Singer (:stevensn)]

Attachment: 1046452.diff

Patch updated to remove the PC_sig changes.

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=8b58574186ef

Comment 20

[Ryan VanderMeulen [:RyanVM]]

There's a whole lot of jittest failures in that Try run and the run prior.

Comment 21

[Steve Singer (:stevensn)]

Attachment: 1046452.diff

Sorry about not catching the JIT failures in the try run.

It looks like I need a second set of brackets in
MacroAssembler::branchIfNotInterpretedConstructor
IMM32_16ADJ( (JSFunction::IS_FUN_PROTO | JSFunction::ARROW | JSFunction::SELF_HOSTED) );

I'm not sure if this needs a new r+ or not.

https://tbpl.mozilla.org/?tree=Try&rev=f26c247a0217

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a5de6212a18c

Comment 23

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/a5de6212a18c