Closed Bug 1130910 Opened 5 years ago Closed 5 years ago

Fix various non-ion breakages

Categories

(Core :: JavaScript Engine: JIT, defect)

Other
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox38 --- fixed

People

(Reporter: stevensn, Assigned: stevensn)

References

Details

Attachments

(1 file, 2 obsolete files)

Fix some issues preventing non ion builds from compiling.


Bug 1125236 introduced a reference to the 'Operator' this in CodeGenerator-shared-inl.h so we need to add a stub implementation to the none jit.

I am also switching the Register and FloatRegister type to be a enum (like the other backends use) instead of an int. 

This patch also fixes a few other non-ion breakages that have recently showed up.

JitStackAlignment (Bug 1112159)
generateProfilerExitFrameTailStub (Bug 1057082 )
Attached patch 1130910_ionfixes.diff (obsolete) — Splinter Review
Attachment #8561146 - Flags: review?(jdemooij)
Comment on attachment 8561146 [details] [diff] [review]
1130910_ionfixes.diff

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

Looks good, but some nits below.

::: js/src/jit/none/Architecture-none.h
@@ +20,5 @@
>  
>  class Registers
>  {
>    public:
> +    enum RegisterID { 

Nit: remove whitespace at the end of this line.

::: js/src/jit/none/MacroAssembler-none.h
@@ +140,5 @@
> +class VFPAddr {
> +};
> +
> +class DTRAddr {
> +};

These classes are very ARM-specific. Can you just add an empty Operand class or is that not enough?

@@ +505,5 @@
>      void loadAsmJSHeapRegisterFromGlobalData() { MOZ_CRASH(); }
>      void memIntToValue(Address, Address) { MOZ_CRASH(); }
>  
>      void setPrinter(Sprinter *) { MOZ_CRASH(); }
> +    Operand ToPayload(Operand base) { MOZ_CRASH(); } 

Nit: remove trailing whitespace.

@@ +509,5 @@
> +    Operand ToPayload(Operand base) { MOZ_CRASH(); } 
> +
> +    // Instrumentation for entering and leaving the profiler.
> +    void profilerEnterFrame(Register , Register ) { MOZ_CRASH(); }
> +    void profilerExitFrame() { MOZ_CRASH(); } 

And here.

@@ +519,5 @@
>  };
>  
>  typedef MacroAssemblerNone MacroAssemblerSpecific;
>  
> +

Nit: don't add a newline here.

@@ +537,5 @@
>  };
>  
> +
> +
> +

And here (two newlines).

::: js/src/jit/none/Trampoline-none.cpp
@@ +59,5 @@
>  bool ICCompare_Int32::Compiler::generateStubCode(MacroAssembler &) { MOZ_CRASH(); }
>  bool ICCompare_Double::Compiler::generateStubCode(MacroAssembler &) { MOZ_CRASH(); }
>  bool ICBinaryArith_Int32::Compiler::generateStubCode(MacroAssembler &) { MOZ_CRASH(); }
>  bool ICUnaryArith_Int32::Compiler::generateStubCode(MacroAssembler &) { MOZ_CRASH(); }
> +JitCode * JitRuntime::generateProfilerExitFrameTailStub(JSContext *) { MOZ_CRASH(); }

Nit: remove the space between '*' and JitRuntime.
Attachment #8561146 - Flags: review?(jdemooij)
Attached patch 1130910_ionfixes.diff (obsolete) — Splinter Review
The Operand class needs Address & constructor.
Other than that it can be empty(at least for now).

I think I've addressed the rest of your concerns.
Attachment #8561146 - Attachment is obsolete: true
Attachment #8563214 - Flags: review?(jdemooij)
Comment on attachment 8563214 [details] [diff] [review]
1130910_ionfixes.diff

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

Thanks!

::: js/src/jit/none/MacroAssembler-none.h
@@ +135,5 @@
>  
> +class Operand
> +{
> +public:
> +    Operand (const Address &) { MOZ_CRASH();}

Nit: indent 'public:' with two spaces, and remove space between 'Operand' and '('
Attachment #8563214 - Flags: review?(jdemooij) → review+
fixed indentation.
Attachment #8563214 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/0d0e9ab07f41
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in before you can comment on or make changes to this bug.