1266781 - Baldr: trap on out-of-bounds values for int32/int64 truncation

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Benjamin Bouvier [:bbouvier] (inactive)]

There are two different traps:
- either the input is NaN, and the error thrown is "invalid conversion to integer"
- or the input is out of the target's bounds, and the error thrown is "integer overflow"

After looking at the code handling MTruncateToInt32, down to codegen and the OOL paths when the result of the naive conversion looks suspicious, I think we'd better have a new MIR node here. In all the OOL paths, we end up calling JS::ToInt32(double), which seems to return the result of double % INT32_MAX, has inline assembly, etc. making it not appealing for changes.

Comment 1

[Benjamin Bouvier [:bbouvier] (inactive)]

This might subsume work in bug 1253581.

Comment 2

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: wip.patch

work in progress; works only on x64, introduces new traps in wasm, a new MIR/LIR WasmTruncateToInt32 (the actual MTruncateToInt32 does a truncation then a modulo INT32_MAX and is not easy to be adapted).

There are many things remaining to do, so do not look too much at this patch, it is very likely to change a lot very soon.

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 1. New traps

I plan to add a way to pass an argument to a generic function WasmTrap, so that we don't need to expand the number of JumpTargets/SymbolicAddresses each time we want to add a new trap. But until then, this will do it.

Comment 4

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 2. Implement correct truncations

Jan, picking you as reviewer because Dan seems to be out, but feel free to r? him instead or anybody else.

The semantics of truncate are the following:
- if the input is NaN, trap (~throw) with the error message "invalid integer conversion"
- if the input is out of the target types' bounds, trap with "integer overflow"
- otherwise, do the correct conversion.

This correctly handles edge cases:
- on ARM, vcvt clamps the output so that it fits in the target. So we need to check the input whenever we see INT_MIN/INT_MAX as the output.
- on x86/x64, cvts{s,d}2s{i,q} will return INT_MIN in case of invalid conversion, but that could be a legitimate INT_MIN being truncated.

With this patch, a new spec test passes: conversions.wast, which handles all the edge cases \o/

https://github.com/WebAssembly/spec/blob/19e2c61f1ba7719b2c4e5fe6c51ad5d6aaa47dbf/ml-proto/test/conversions.wast

Comment 5

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 3. Rename TruncateToInt64 into WasmTruncateToInt64

Context:
- there is already a MTruncateToInt32, but it doesn't implement the semantics we want here (MTruncateToInt32 does a truncation modulo INT32_MAX, as JS::ToInt32(double) does).
- that's why I've introduced MWasmTruncateToInt32 in the previous patch
- as a matter of fact, the existing int64 equivalent should be renamed into MWasmTruncateToInt64 to tell the user which semantics it applies.

Comment 6

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 2. Implement correct truncations

Updated tests, simplified a few things.

Comment 7

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8745940 [details] [diff] [review]
2. Implement correct truncations

Doh, this is not done yet: asm.js shouldn't have the same semantics...

Comment 9

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 2. Implement correct truncations

That one looks better and dispatches the right MIR node with respect to asm.js mode.

Comment 10

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 3. Rename TruncateToInt64 into WasmTruncateToInt64

Rebased.

Comment 11

[Dan Gohman [:sunfish]]

Comment on attachment 8745963 [details] [diff] [review]
2. Implement correct truncations

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

Feel free to carry the r+ forward when the IonCompile parts are updated for 0xb, unless there are any surprises.

::: js/src/asmjs/WasmIonCompile.cpp
@@ +2492,5 @@
>  {
> +    bool isUnsigned = expr == Expr::I32TruncUF32 ||
> +                      expr == Expr::I32TruncUF64 ||
> +                      expr == Expr::I64TruncUF64 ||
> +                      expr == Expr::I64TruncUF32;

This works, though I'm fond of the boolean parameter, because the code that calls this is already doing a switch, so it can just have one call for the unsigned cases and one call for the signed cases and there's no need to do additional dispatching on the opcode. But it's ok either way.

::: js/src/jit/MIR.cpp
@@ +3807,5 @@
> +
> +        if (!isUnsigned_ && d <= double(INT32_MAX) && d >= double(INT32_MIN))
> +            return MConstant::New(alloc, Int32Value(ToInt32(d)));
> +
> +        if (isUnsigned_ && d <= double(UINT32_MAX) && d >= 0)

It's not super important, but technically the safe range for this transformation is d < double(UINT32_MAX)+1 && d > -1 (and so on for the signed and float32 cases) (and other places in the patch already do check for the correct range).

::: js/src/jit/MIR.h
@@ +5340,5 @@
> +    }
> +};
> +
> +// Truncate a value to an int32, with wasm semantics: the input shouldn't be
> +// NaN, nor should it be out of the target type's bounds.

"shouldn't" makes it unclear who's responsible for meeting the precondition. I suggest just saying it traps when the value is out of range.

::: js/src/jit/arm/CodeGenerator-arm.cpp
@@ +2897,5 @@
> +
> +    masm.ma_vxfer(scratch, output);
> +    masm.ma_cmp(output, Imm32(INT32_MAX));
> +    masm.ma_cmp(output, Imm32(INT32_MIN), Assembler::NotEqual);
> +    masm.ma_b(ool->entry(), Assembler::Equal);

Ah, clever. My idea here had been that we could add 1 so that INT32_MAX wraps around to INT32_MIN, and then test for the result being <= INT32_MIN+1, however using the condition codes as you did here to combine both checks into one branch is cleaner and probably faster.

@@ +2944,5 @@
> +        MOZ_ASSERT(fromType == MIRType::Float32);
> +        scratch = scratchScope.singleOverlay();
> +
> +        // For int32, float(minValue) == INT32_MIN, we want to fail when input < float(minValue).
> +        // For uint32, float(minValue) == -1, we want to fail when input <= -1.

Aha. This is fairly subtle. I'd suggest saying "float(minValue) rounds to INT32_MIN" instead of == to make it clear what's happening.

::: js/src/jit/x64/CodeGenerator-x64.cpp
@@ +1231,5 @@
> +
> +        // Check that the result is in the uint32_t range.
> +        ScratchRegisterScope scratch(masm);
> +        masm.movq(ImmWord(0xffffffff00000000), scratch);
> +        masm.testq(output, scratch);

It would be a little smaller to put 0xffffffff in scratch and test for unsigned greater than, because it'd avoid the 64-bit immediate.

::: js/src/jit/x86-shared/CodeGenerator-x86-shared.cpp
@@ +4056,5 @@
> +            if (fromType == MIRType::Double) {
> +                // We've used vcvtsd2sq. The only legit value whose i64
> +                // truncation is INT64_MIN is double(INT64_MIN): exponent is so
> +                // high that the highest resolution around is much more than 1.
> +                masm.loadConstantDouble(double(int64_t(0x8000000000000000)), ScratchDoubleReg);

The code just above this uses INT32_MIN, so it'd be nice for this code (and the other places that do this) to use INT64_MIN for consistency.

Comment 12

[Dan Gohman [:sunfish]]

Comment on attachment 8745964 [details] [diff] [review]
3. Rename TruncateToInt64 into WasmTruncateToInt64

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

Looks good. (btw, I'm stealing these reviews; hopefully jandem doesn't mind :))

Comment 13

[Benjamin Bouvier [:bbouvier] (inactive)]

Thank you! The port to the new iterator API went smoothly :)

(In reply to Dan Gohman [:sunfish] from comment #11)
> ::: js/src/jit/MIR.cpp
> @@ +3807,5 @@
> > +
> > +        if (!isUnsigned_ && d <= double(INT32_MAX) && d >= double(INT32_MIN))
> > +            return MConstant::New(alloc, Int32Value(ToInt32(d)));
> > +
> > +        if (isUnsigned_ && d <= double(UINT32_MAX) && d >= 0)
> 
> It's not super important, but technically the safe range for this
> transformation is d < double(UINT32_MAX)+1 && d > -1 (and so on for the
> signed and float32 cases) (and other places in the patch already do check
> for the correct range).
That's right, but ToInt32 does a truncation modulo INT32_MAX, e.g. ToInt32(double(INT32_MAX) + 0.5) == 0.5. I didn't want to add more conditions here (if d >= INT32_MAX && d < double(INT32_MAX)+1, etc.).

> 
> ::: js/src/jit/arm/CodeGenerator-arm.cpp
> @@ +2897,5 @@
> > +
> > +    masm.ma_vxfer(scratch, output);
> > +    masm.ma_cmp(output, Imm32(INT32_MAX));
> > +    masm.ma_cmp(output, Imm32(INT32_MIN), Assembler::NotEqual);
> > +    masm.ma_b(ool->entry(), Assembler::Equal);
> 
> Ah, clever. My idea here had been that we could add 1 so that INT32_MAX
> wraps around to INT32_MIN, and then test for the result being <=
> INT32_MIN+1, however using the condition codes as you did here to combine
> both checks into one branch is cleaner and probably faster.
Idea taken from the existing branchTruncateDouble code in the ARM generator :)

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/21073609c630
https://hg.mozilla.org/integration/mozilla-inbound/rev/3fea715e40ac
https://hg.mozilla.org/integration/mozilla-inbound/rev/25a26d56365c

Comment 15

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

https://hg.mozilla.org/mozilla-central/rev/21073609c630
https://hg.mozilla.org/mozilla-central/rev/3fea715e40ac
https://hg.mozilla.org/mozilla-central/rev/25a26d56365c