1032786 - IonMonkey MIPS: Test asm.js/testFFI.js fails on MIPS

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Branislav Rankov [:rankov]]

Test asm.js/testFFI.js fails on MIPS with a segfault.

The failure manifested after the test was updated in bug 1024498

Comment 1

[Branislav Rankov [:rankov]]

Part of the problem here is in AsmJS.cpp line: 
masm.convertValueToInt32(JSReturnOperand, ReturnFloatReg, ReturnReg, &oolConvert,
                                 /* -0 check */ false);

The problem is that JSReturnOperand is clobbered by this line because on MIPS ReturnReg is v0 and JSReturnOperand is (v0, v1). The oolConvert code expects JSReturnOperand to be intact.

I decided to change JSReturnOperand to (a2, a3) similar to what is done in ARM.

The other part of the problem is that arguments to CoerceInPlace_ToInt32 and CoerceInPlace_ToNumber are not properly aligned.

Comment 2

[Branislav Rankov [:rankov]]

Attachment: GenerateOOLConvert-alignment.patch

Comment 3

[Branislav Rankov [:rankov]]

Attachment: Change-JSReturnOperand-regs.patch

Comment 4

[Luke Wagner [:luke]]

Comment on attachment 8449496 [details] [diff] [review]
GenerateOOLConvert-alignment.patch

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

d'oh!

Comment 5

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8449507 [details] [diff] [review]
Change-JSReturnOperand-regs.patch

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

::: js/src/jit/mips/Assembler-mips.h
@@ +103,5 @@
>  static MOZ_CONSTEXPR_VAR Register InvalidReg = { Registers::invalid_reg };
>  static MOZ_CONSTEXPR_VAR FloatRegister InvalidFloatReg = { FloatRegisters::invalid_freg };
>  
> +static MOZ_CONSTEXPR_VAR Register JSReturnReg_Type = a3;
> +static MOZ_CONSTEXPR_VAR Register JSReturnReg_Data = a2;

The way we write down Value in memory, we store the type first, then the payload, wouldn't it be faster to order the registers the same way, by using a2 for the type and a3 for the data?

::: js/src/jit/mips/Trampoline-mips.cpp
@@ +194,5 @@
>      if (type == EnterJitBaseline) {
>          // Handle OSR.
>          GeneralRegisterSet regs(GeneralRegisterSet::All());
>          regs.take(JSReturnOperand);
> +        regs.takeUnchecked(OsrFrameReg);

No, either we do things properly or we don't.  Do

  regs.add(OsrFrameReg);

  masm.call…

  regs.take(JSReturnOperand);

But takeUnchecked should not be used here, as this is basically saying don't bother I know what mess I am doing.

Comment 6

[Branislav Rankov [:rankov]]

(In reply to Nicolas B. Pierron [:nbp] from comment #5)
> The way we write down Value in memory, we store the type first, then the
> payload, wouldn't it be faster to order the registers the same way, by using
> a2 for the type and a3 for the data?

I don't think this is true for MIPS.

Comment 7

[Branislav Rankov [:rankov]]

Attachment: Change-JSReturnOperand-regs.patch

Comment 8

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8450966 [details] [diff] [review]
Change-JSReturnOperand-regs.patch

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

Thanks :)

Comment 9

[Branislav Rankov [:rankov]]

Attachment: Change-JSReturnOperand-regs.patch

Nicolas, I have found a bug in previous patch and fixed it. Can you do a quick review?

Comment 10

[Branislav Rankov [:rankov]]

patch GenerateOOLConvert-alignment was made obsolete by patch simplify-ion-exit from bug 1034330

Comment 11

[Branislav Rankov [:rankov]]

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

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

Comment 12

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

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