Closed Bug 1046452 Opened 7 years ago Closed 7 years ago

Unsupported architecture in MacroAssembler.h

Categories

(Core :: JavaScript Engine: JIT, defect)

PowerPC
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: stevensn, Assigned: bhackett1024)

References

Details

Attachments

(1 file, 7 obsolete files)

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."
Attached patch patch (obsolete) — Splinter Review
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.
Assignee: nobody → bhackett1024
Attachment #8465185 - Flags: review?(jdemooij)
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.
Attachment #8465185 - Flags: review?(jdemooij) → review+
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.
js/src/jit/none/BaseMacroAssembler-none.h:20:17: error: ?static bool JSC::MacroAssemblerNone::supportsFloatingPoint()? is private

I think supportFloatingPoint needs to be public.
Attached patch 1046452_WIP.diff (obsolete) — Splinter Review
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
Attached patch 1046452.diff (obsolete) — Splinter Review
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?)
Attachment #8465943 - Attachment is obsolete: true
Mostly a guess after reading the source.
Attached patch 1046452.diff (obsolete) — Splinter Review
I have merged in Jan's PC_sig patch into my larger patch.
Attachment #8467481 - Attachment is obsolete: true
Attachment #8467606 - Attachment is obsolete: true
Attachment #8469007 - Flags: review?(bhackett1024)
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
Attachment #8469007 - Flags: review?(bhackett1024)
(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).
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?
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.
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).
Attached patch 1046452.diff (obsolete) — Splinter Review
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.
Attachment #8469007 - Attachment is obsolete: true
Attachment #8470491 - Flags: review?(bhackett1024)
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.
Attachment #8470491 - Flags: review?(bhackett1024) → review+
Attached patch 1046452.diff (obsolete) — Splinter Review
Patch updated to remove the PC_sig changes.

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=8b58574186ef
Attachment #8470491 - Attachment is obsolete: true
Keywords: checkin-needed
Keywords: leave-open
There's a whole lot of jittest failures in that Try run and the run prior.
Keywords: checkin-needed
Attached patch 1046452.diffSplinter Review
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
Attachment #8471320 - Attachment is obsolete: true
Attachment #8474577 - Flags: review?(bhackett1024)
Attachment #8474577 - Flags: review?(bhackett1024) → review+
Keywords: checkin-needed
Attachment #8465185 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/a5de6212a18c
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
You need to log in before you can comment on or make changes to this bug.