1407986 - [MIPS] Fix unwinding during the interrupt in wasm prologue/epilogue

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect, P3)

Description

[Dragan Mladjenovic]

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/51.0.2704.106 Safari/537.36

Steps to reproduce:

Run jit_test.py  --repeat=1000   dist/bin/js asm.js/testTimeout1.js on mips32/debug


Actual results:

Some test runs start failing with: 
Assertion failure: code, at ~/work/gecko-dev/js/src/wasm/WasmFrameIter.cpp:600
Exit code: -11


Expected results:

Tests passing.

Comment 1

[Dragan Mladjenovic]

Attachment: bug1407986.patch

The bug manifests when we receive interrupt signal in wasm prologue/epilogue. On mips32/64 push, pop and ret macro-instructions are not atomic so we can receive signal when only part of instruction is executed. ProfilingFrame iterator needs to handle unwinding when pc points in between two macro-instructions used in prologue/epilogue.

Comment 2

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8917824 [details] [diff] [review]
bug1407986.patch

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

Apart from a few nits, looks good!

In ARM, to detect issues with interrupts, we've implemented a special mode for the simulator to trigger this kind of issues more easily:
http://searchfox.org/mozilla-central/source/js/src/jit/arm/Simulator-arm.cpp#4852

It can be triggered via environment variables or programmatically in JS tests: http://searchfox.org/mozilla-central/source/js/src/jit-test/tests/wasm/timeout/2.js#29
Would it help to implement such a mode in the MIPS simulator too?

::: js/src/wasm/WasmFrameIter.cpp
@@ +453,5 @@
>      masm.pop(WasmTlsReg);
>      DebugOnly<uint32_t> poppedExitReason = masm.currentOffset();
>  
>      masm.pop(WasmTlsReg);
> +    DebugOnly<uint32_t> poppedTlsReg = masm.currentOffset();

nit: \n after this line

@@ +461,2 @@
>      *ret = masm.currentOffset();
> +#if defined(JS_CODEGEN_MIPS32) || defined(JS_CODEGEN_MIPS64)

To make it easier to follow, even if it duplicates just a bit more code, can you fuse the two #if defined blocks?

@@ +749,5 @@
>        case CodeRange::TrapExit:
>        case CodeRange::DebugTrap:
> +#if defined(JS_CODEGEN_MIPS32) || defined(JS_CODEGEN_MIPS64)
> +        if ((offsetFromEntry >= BeforePushRetAddr && offsetFromEntry < PushedFP) || codeRange->isThunk()) {
> +            // See BUG.

nit: missing bug number here

@@ +751,5 @@
> +#if defined(JS_CODEGEN_MIPS32) || defined(JS_CODEGEN_MIPS64)
> +        if ((offsetFromEntry >= BeforePushRetAddr && offsetFromEntry < PushedFP) || codeRange->isThunk()) {
> +            // See BUG.
> +            // On MIPS push is emulated by two instruction: adjusting the $sp
> +            // and storing the value to $sp.

nit: we don't use $ in comments to refer to registers, let's juse use `sp` and `ra` everywhere.

@@ +752,5 @@
> +        if ((offsetFromEntry >= BeforePushRetAddr && offsetFromEntry < PushedFP) || codeRange->isThunk()) {
> +            // See BUG.
> +            // On MIPS push is emulated by two instruction: adjusting the $sp
> +            // and storing the value to $sp.
> +            // Execution might be interrupted in between the two operation so we

nit: two operations*

@@ +762,5 @@
> +            fixedFP = fp;
> +            AssertMatchesCallSite(activation, fixedPC, fixedFP);
> +        } else
> +#endif
> +#if defined(JS_CODEGEN_ARM) || defined(JS_CODEGEN_ARM64)

nit: #elif

@@ +795,5 @@
>              fixedPC = reinterpret_cast<Frame*>(sp)->returnAddress;
>              fixedFP = fp;
>              AssertMatchesCallSite(activation, fixedPC, fixedFP);
> +        } else if (offsetInCode >= codeRange->ret() - PoppedFP
> +                    && offsetInCode < codeRange->ret() - PoppedExitReason) {

nit: && on previous line, then align the two offsetInCode at lines starts

@@ +802,5 @@
>              fixedPC = sp[2];
>              fixedFP = fp;
>              AssertMatchesCallSite(activation, fixedPC, fixedFP);
> +        } else if (offsetInCode >= codeRange->ret() - PoppedExitReason
> +                    && offsetInCode < codeRange->ret() - PoppedTLSReg ) {

ditto

@@ +810,5 @@
>              fixedFP = fp;
>              AssertMatchesCallSite(activation, fixedPC, fixedFP);
> +#if defined(JS_CODEGEN_MIPS32) || defined(JS_CODEGEN_MIPS64)
> +        } else if (offsetInCode >= codeRange->ret() - PoppedTLSReg
> +                    && offsetInCode < codeRange->ret()) {

ditto

@@ +816,5 @@
> +            // exit reason hasn't been popped yet.
> +            fixedPC = sp[0];
> +            fixedFP = fp;
> +            AssertMatchesCallSite(activation, fixedPC, fixedFP);
> +        } else if (offsetInCode == codeRange->ret()) {

Could offsetInCode also point to the nop (delay slot) hidden in the branch() call?

Comment 3

[Dragan Mladjenovic]

Attachment: bug1407986_iter.patch

Comment 4

[Dragan Mladjenovic]

Attachment: bug1407986_sim.patch

Comment 5

[Dragan Mladjenovic]

(In reply to Benjamin Bouvier [:bbouvier] from comment #2)
> Comment on attachment 8917824 [details] [diff] [review]
> bug1407986.patch
>
> In ARM, to detect issues with interrupts, we've implemented a special mode
> for the simulator to trigger this kind of issues more easily:
> http://searchfox.org/mozilla-central/source/js/src/jit/arm/Simulator-arm.
> cpp#4852
> 
> It can be triggered via environment variables or programmatically in JS
> tests:
> http://searchfox.org/mozilla-central/source/js/src/jit-test/tests/wasm/
> timeout/2.js#29
> Would it help to implement such a mode in the MIPS simulator too?
> 
 Thanks for the review.
 Neat. Added to both simulators. 

> Could offsetInCode also point to the nop (delay slot) hidden in the branch()
> call?
No. The branch + delay slot pair is atomic in that regard.

Comment 6

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8919363 [details] [diff] [review]
bug1407986_sim.patch

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

Nice!

::: js/src/jit/mips32/Simulator-mips32.cpp
@@ +3568,5 @@
>      instructionDecode(instr);
>  }
>  
> +static void
> +FakeInterruptHandler()

Looks like it could be a method of JSContext, or from a base class parent to all the Simulator classes (would be put in jit/shareds/Simulator-shared.h). Preferrable to do it in another patch, though.

Comment 7

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8919362 [details] [diff] [review]
bug1407986_iter.patch

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

Looks good with a few nits, thanks!

::: js/src/wasm/WasmFrameIter.cpp
@@ +464,3 @@
>      *ret = masm.currentOffset();
>      masm.ret();
> +#endif

nit: \n after endif

@@ +794,5 @@
>              fixedPC = reinterpret_cast<Frame*>(sp)->returnAddress;
>              fixedFP = fp;
>              AssertMatchesCallSite(activation, fixedPC, fixedFP);
> +        } else if (offsetInCode >= codeRange->ret() - PoppedFP &&
> +                   offsetInCode < codeRange->ret() - PoppedExitReason) {

nit: { on new line

@@ +801,5 @@
>              fixedPC = sp[2];
>              fixedFP = fp;
>              AssertMatchesCallSite(activation, fixedPC, fixedFP);
> +        } else if (offsetInCode >= codeRange->ret() - PoppedExitReason &&
> +                   offsetInCode < codeRange->ret() - PoppedTLSReg ) {

nit: remove space before )
{ on new line

@@ +809,5 @@
>              fixedFP = fp;
>              AssertMatchesCallSite(activation, fixedPC, fixedFP);
> +#if defined(JS_CODEGEN_MIPS32) || defined(JS_CODEGEN_MIPS64)
> +        } else if (offsetInCode >= codeRange->ret() - PoppedTLSReg &&
> +                   offsetInCode < codeRange->ret()) {

nit: { on new line

Comment 8

[Dragan Mladjenovic]

Attachment: bug1407986_iter2.patch

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb843afcec46
[MIPS] Make simulator handle simulator.always-interrupt option. r=bbouvier
https://hg.mozilla.org/integration/mozilla-inbound/rev/491f966f4e16
[MIPS] Fix unwinding during the interrupt in wasm prologue/epilogue. r=bbouvier

Comment 10

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/fb843afcec46
https://hg.mozilla.org/mozilla-central/rev/491f966f4e16