Closed Bug 1256945 Opened 8 years ago Closed 8 years ago

SIMD: Coerce lane indexes with ToNumber()

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: jolesen, Unassigned)

References

Details

Attachments

(1 file)

The SIMDToLane() function in the SIMD.js specification requires that lane index arguments are coerced with ToNumber(). See <http://tc39.github.io/ecmascript_simd/#simd-to-lane>.

Currently, SpiderMonkey requires that lane indexes are integers. It does not apply the coercion.
The SIMDToLane() function in the SIMD.js specification uses ToNumber() to
coerce lane index arguments to a number before checking the the index is an
integer in range.

Add an ArgumentToLaneIndex() function to SIMD.cpp that implements the same
semantics. This function throws a RangeError if the coerced argument is not
integral or out of range.

Update tests to match the new semantics. The differences are:

- More values are accepted as lane indexes (null, true, false, "5.0", ...).
- A TypeError is only thrown if ToNumber fails. If the argument can be coerced
  to a number, a RangeError is thrown if other checks fail.

Fix the testShuffleForType() test in ests/ecma_7/SIMD/swizzle-shuffle.js which
should have been calling `shuffle` but was calling `swizzle`.

Review commit: https://reviewboard.mozilla.org/r/40207/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/40207/
Attachment #8730869 - Flags: review?(bbouvier)
Attachment #8730869 - Flags: review?(bbouvier) → review+
Comment on attachment 8730869 [details]
MozReview Request: Bug 1256945 - Coerce SIMD lane indexes with ToNumber(). r=bbouvier

https://reviewboard.mozilla.org/r/40207/#review36891

Great to see this; I remember I've argued for this at some point, for consistency with other JavaScript APIs. It's a bit fun / weird that 0.5 will throw a RangeError; strictly talking, it is in range but not the right type. Thank you for the patch.

Please update the commit message to r=bbouvier, not r=benj.

::: js/src/builtin/SIMD.cpp:1322
(Diff revision 1)
>  
> +// Extract an integer lane index from a function argument.
> +//
> +// Register an exception and return false if the argument is not suitable.
> +static bool
> +ArgumentToLaneIndex(JSContext* cx, JS::HandleValue v, unsigned* lane, unsigned limit)

May I ask to put the outparam `lane` at the last position in the signature, please?

::: js/src/builtin/SIMD.cpp:1328
(Diff revision 1)
> +{
> +    uint64_t arg;
> +    if (!ArgumentToIntegerIndex(cx, v, &arg))
> +        return false;
> +    if (arg >= limit) {
> +        JS_ReportErrorNumber(cx, GetErrorMessage, nullptr, JSMSG_BAD_INDEX);

If you want to, feel free to make an helper function ThrowRangeError which returns false all the time and reports the error number. It could be reused in the function above a few times too.

::: js/src/jit-test/tests/SIMD/shuffle.js
(Diff revision 1)
> -            assertEqX4(SIMD.Int32x4.shuffle(i1, i2, value, 2, 4, 5), [4, 3, 5, 6]);
> +            assertEqX4(SIMD.Int32x4.shuffle(i1, i2, value, 2, 4, 5), [1, 3, 5, 6]);
>          } catch(e) {
>              print(e);
>              caught = true;
>              assertEq(i, 149);
> -            assertEq(e instanceof TypeError, true);

Is it that we need to update ion to make sure that it throws the same error as well?

::: js/src/jit-test/tests/SIMD/shuffle.js:74
(Diff revision 1)
> -            assertEq(e instanceof TypeError, true);
>          }
> +        if (expectException)
> -        assertEq(i < 149 || caught, true);
> +            assertEq(i < 149 || caught, true);
> +        else
> +            assertEq(caught, false);

Can we just assertEq(i < 149 || caught, expectException)? It seems so...
https://reviewboard.mozilla.org/r/40207/#review36891

Thanks!

I guess, technically, JS doesn't have an integer type. Everything is a double. The reasoning in the spec is something like:

1. If `ToNumber(arg)` can't even convert your argument to a number, throw a `TypeError`. This happens with symbols and SIMD values and a few other things.
2. If you can coerce to a number, but it's not a number you like, throw a `RangeError`. This includes fractional numbers, out-of-range integers, and things like NaN.

The discussion in [the Github issue](https://github.com/tc39/ecmascript_simd/issues/328) is interesting. Indexes in `typedArray[index]` are treated differently.

> Is it that we need to update ion to make sure that it throws the same error as well?

As I understand it, Ion doesn't throw exceptions. It just bails out and lets the VM code throw the exception. (Maybe I am wrong?).

In this case, we used to throw a TypeError for any kind of lane index we didn't like, but now we sometimes throw a RangeError instead. The `uglyDuckling` values tested here cover all three kinds of bailout:

1. Bail to throw a `TypeError`.
2. Bail to throw a `RangeError`.
3. Bail to coerce a weird argument like `"  +3.0e0   "`. Ion doesn't handle this argument, but it is still valid.

I could check that the exception is either a `RangeError` or a `TypeError`.
Comment on attachment 8730869 [details]
MozReview Request: Bug 1256945 - Coerce SIMD lane indexes with ToNumber(). r=bbouvier

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40207/diff/1-2/
Attachment #8730869 - Attachment description: MozReview Request: Bug 1256945 - Coerce SIMD lane indexes with ToNumber(). r?benj → MozReview Request: Bug 1256945 - Coerce SIMD lane indexes with ToNumber(). r=bbouvier
https://hg.mozilla.org/mozilla-central/rev/26242740f980
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.