1268910 - Baldr: trap on edge cases for arithmetic operations

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Benjamin Bouvier [:bbouvier] (inactive)]

e.g. I32Div should trap when we try to divide by zero.

We should also find a way to generalize traps, so we don't have to create a stub for each of them, and just generate a reason that we pass to the trap handler.

Comment 1

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: traps wip

Just asking feedback at the moment, to validate the approach. I don't think it's fine to generate an error stub inline, so instead we could have a JumpTarget::Trap, and we'd push the reason onto the stack first in MacroAssembler::wasmTrap; and would read it from the error stub (that is, the target of JumpTarget::Trap) later. How does that sound?

Comment 2

[Luke Wagner [:luke]]

Comment on attachment 8747110 [details] [diff] [review]
traps wip

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

Yes, this definitely looks good!  IIUC, you're saying the patch would *not* use callWithABI which I think is good b/c I think it doesn't work for wasm (in particular, wrt profiling).

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: traps.patch

Comment 4

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: wip-div.patch

And this is what it looks like to add a new trap thereafter.

Comment 5

[Luke Wagner [:luke]]

Comment on attachment 8747768 [details] [diff] [review]
traps.patch

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

::: js/src/asmjs/WasmStubs.cpp
@@ +869,5 @@
> +
> +    // sp can be anything at this point, so ensure it is aligned when calling
> +    // into C++.  We unconditionally jump to throw so don't worry about
> +    // restoring sp.
> +    masm.andToStackPtr(Imm32(~(ABIStackAlignment - 1)));

Pre-existing from GenerateErrorStub (which I'm hoping we can wholly replace by this function), but I think, after masking the stack pointer, you need to further subtract ShadowStackSpace (which is bytes above the stack pointer that may be clobbered by the callee).  (See GenerateInterruptStub.)

@@ +879,5 @@
> +    if (i->kind() == ABIArg::GPR) {
> +        if (WasmTrapStubArgReg != i->gpr())
> +            masm.move32(WasmTrapStubArgReg, i->gpr());
> +    } else {
> +        masm.storePtr(WasmTrapStubArgReg,

I think this should be store32.

@@ +937,5 @@
>          return GenerateErrorStub(masm, SymbolicAddress::OnOutOfBounds);
>        case JumpTarget::BadIndirectCall:
>          return GenerateErrorStub(masm, SymbolicAddress::BadIndirectCall);
> +      case JumpTarget::Trap:
> +        return GenerateTrapStub(masm);

Can you use GenerateTrapStub for the other two as well, removing GenerateErrorStub?

::: js/src/jit/x86-shared/CodeGenerator-x86-shared.cpp
@@ +2577,5 @@
>  
> +    Label done;
> +    if (gen->compilingAsmJS()) {
> +        masm.j(Assembler::Equal, &done);
> +        masm.wasmTrap(wasm::Trap::ImpreciseSimdConversion);

Can you give wasmTrap a condition?  Otherwise, this change deoptimizes the common case where there is no trap.

Comment 6

[Benjamin Bouvier [:bbouvier] (inactive)]

Thank you for the review!

(In reply to Luke Wagner [:luke] from comment #5)
> Comment on attachment 8747768 [details] [diff] [review]
> traps.patch
> 
> Review of attachment 8747768 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/asmjs/WasmStubs.cpp
> @@ +869,5 @@
> > +
> > +    // sp can be anything at this point, so ensure it is aligned when calling
> > +    // into C++.  We unconditionally jump to throw so don't worry about
> > +    // restoring sp.
> > +    masm.andToStackPtr(Imm32(~(ABIStackAlignment - 1)));
> 
> Pre-existing from GenerateErrorStub (which I'm hoping we can wholly replace
> by this function), but I think, after masking the stack pointer, you need to
> further subtract ShadowStackSpace (which is bytes above the stack pointer
> that may be clobbered by the callee).  (See GenerateInterruptStub.)

Ha! Could it be somehow related to bug 1267441, which happens only on win x64, which iirc is the only platform that has shadow stack space? The out of memory stub uses the GenerateErrorStub, so maybe the stack gets corrupted after the call and we don't jump to the throw handler, or go there with a stack that sometimes is wrong?

> @@ +937,5 @@
> >          return GenerateErrorStub(masm, SymbolicAddress::OnOutOfBounds);
> >        case JumpTarget::BadIndirectCall:
> >          return GenerateErrorStub(masm, SymbolicAddress::BadIndirectCall);
> > +      case JumpTarget::Trap:
> > +        return GenerateTrapStub(masm);
> 
> Can you use GenerateTrapStub for the other two as well, removing
> GenerateErrorStub?

I've tried this first, but the stubs entries are directly referenced from inline code (e.g. the out of bounds stubs is the one we redirect to from the OOB signal handler, for OOB SIMD accesses), and it makes things more complicated. I can look into it in another patch, though.

> 
> ::: js/src/jit/x86-shared/CodeGenerator-x86-shared.cpp
> @@ +2577,5 @@
> >  
> > +    Label done;
> > +    if (gen->compilingAsmJS()) {
> > +        masm.j(Assembler::Equal, &done);
> > +        masm.wasmTrap(wasm::Trap::ImpreciseSimdConversion);
> 
> Can you give wasmTrap a condition?  Otherwise, this change deoptimizes the
> common case where there is no trap.
I've done that: although there is no Assembler::Condition::Always on x86 and x64, so I made two variants (one with Assembler::Condition, one without it).

Comment 7

[Luke Wagner [:luke]]

Actually, now that I think about it, wasmTrap() would be more flexible if, in addition taking a Condition, it was a single conditional-branch instruction.  Otherwise you can't use the branch to either jump-to-trap or fallthrough-to-valid since the move is executed either way.  Rather, I think there should be 1 jump target per trap and each jump target stores the Trap code and calls a single C++ function.

Comment 8

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: traps.patch

Agreed on irc that the required changes are bigger than expected so another look would be good.

Comment 9

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: trap-div-mod.patch

This implements the traps on integer (signed or unsigned) div/mod:

- div/mod by zero
- div INTMIN by -1

As far as I can tell from the tests, there are no other traps for any arithmetic operations, be it on integers or non-integer operations: is that correct?

With this patch, the spec test i32.wast passes in our implementation.

Comment 10

[Dan Gohman [:sunfish]]

Comment on attachment 8748258 [details] [diff] [review]
trap-div-mod.patch

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

Looks good!

Comment 11

[Luke Wagner [:luke]]

Comment on attachment 8748253 [details] [diff] [review]
traps.patch

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

Great, thanks!

::: js/src/asmjs/WasmStubs.cpp
@@ +869,5 @@
> +    // into C++.  We unconditionally jump to throw so don't worry about
> +    // restoring sp.
> +    masm.andToStackPtr(Imm32(~(ABIStackAlignment - 1)));
> +    if (ShadowStackSpace)
> +        masm.subFromStackPtr(Imm32(ShadowStackSpace));

Could you fix up GenerateErrorStub as well?

Comment 12

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: oobstub.patch

Actually the OOB stub is trivial to remove.

Comment 13

[Luke Wagner [:luke]]

Comment on attachment 8748574 [details] [diff] [review]
oobstub.patch

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

Great!

::: js/src/asmjs/WasmStubs.cpp
@@ +935,2 @@
>        case JumpTarget::BadIndirectCall:
>          return GenerateErrorStub(masm, SymbolicAddress::BadIndirectCall);

Ah, and this straggler will probably get removed or rejiggered with the heterogeneous table work.

::: js/src/asmjs/WasmTypes.h
@@ +643,5 @@
>      // Integer division by zero.
>      IntegerDivideByZero,
>  
> +    // (asm.js only) out of bounds on SIMD and atomic memory accesses.
> +    OutOfBounds,

This will be exercised by wasm load/store oob too, so I'd tweak the comment and put it up with the others.

Comment 14

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: badindirectcall.patch

As said on IRC, I'm worried about one thing: if we call from an inline stub we can jump to, will profiling work? Testing suggests it does, but this might be our tests don't cover this.

Comment 15

[Luke Wagner [:luke]]

Comment on attachment 8748654 [details] [diff] [review]
badindirectcall.patch

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

Even better!  Good observation.

::: js/src/asmjs/WasmTypes.h
@@ +648,2 @@
>      // (asm.js only) out of bounds on SIMD and atomic memory accesses.
>      OutOfBounds,

In my previous comment, I didn't mean that the whole error would go away, just how we exercise it.  I think the target impl scheme will be to check the signature in the callee which will jump to this same stub.  So I don't think it's (temporary) and I'd give it a proper comment and move it up with the others.

Comment 16

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: addendum.patch

And an addendum, so that unsigned div/mod by zero don't get constant folded into the wrong thing. This also adds tests so that i32 edge cases are handled for the constant value cases; in particular, this ensures that div_s/mod_s are not getting DCE'd.

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d120b40f45a5
https://hg.mozilla.org/integration/mozilla-inbound/rev/c2421006f687
https://hg.mozilla.org/integration/mozilla-inbound/rev/b03a6ba4b80a
https://hg.mozilla.org/integration/mozilla-inbound/rev/027fc0be5ba9

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3fe6385db382

Comment 19

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

https://hg.mozilla.org/mozilla-central/rev/d120b40f45a5
https://hg.mozilla.org/mozilla-central/rev/c2421006f687
https://hg.mozilla.org/mozilla-central/rev/b03a6ba4b80a
https://hg.mozilla.org/mozilla-central/rev/027fc0be5ba9
https://hg.mozilla.org/mozilla-central/rev/3fe6385db382

Comment 20

[Dan Gohman [:sunfish]]

Comment on attachment 8750297 [details] [diff] [review]
addendum.patch

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

Looks good.

Comment 21

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4f43207abcb9

Comment 22

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

https://hg.mozilla.org/mozilla-central/rev/4f43207abcb9

Comment 23

[Benjamin Bouvier [:bbouvier] (inactive)]

Forgot to remove leave-open after the last push.