1252270 - SIMD: Coerce non-numeric indexes to load/store functions

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jakob Stoklund Olesen [:jolesen]]

This should throw a TypeError:

    SIMD.Int32x4.load(typedArray, "Hi!")

Currently, it loads from index 0 in the array.

The SIMD.js spec does not apply the ToNumber function to load/store indexes. It requires numeric, integer values.

Comment 1

[Jakob Stoklund Olesen [:jolesen]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=658cbd996fed

Comment 2

[Jakob Stoklund Olesen [:jolesen]]

Attachment: SIMD: Reject non-numeric indexes to load/store functions.

Throw a TypeError when we see a non-integer index or a number outside the
ToLength() range.

Throw a RangeError for negative and positive indexes that would be outside the
range of the array.

Comment 3

[Lars T Hansen [:lth]]

I think I'd like to wait on this until https://github.com/tc39/ecmascript_simd/issues/328 has been resolved, see my last comment there.

Comment 4

[Jakob Stoklund Olesen [:jolesen]]

Thanks, Lars. I agree, we should aim for some consistency.

Comment 5

[Lars T Hansen [:lth]]

Comment on attachment 8725012 [details] [diff] [review]
SIMD: Reject non-numeric indexes to load/store functions.

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

I added some comments but it's probably the case that this patch is out of date now, given the resolutions on the spec.

::: js/src/builtin/SIMD.cpp
@@ +1261,2 @@
>  static bool
> +ValueIsIntegerIndex(const Value& v, int64_t* index)

This should do n=ToNumber(v), i=ToInteger(n), and then check that i==n, first of all.

::: js/src/tests/ecma_7/SIMD/load.js
@@ +125,5 @@
>          // Invalid args
>          assertThrowsInstanceOf(() => SIMD[kind].load(), TypeError);
>          assertThrowsInstanceOf(() => SIMD[kind].load(ta), TypeError);
>          assertThrowsInstanceOf(() => SIMD[kind].load("hello", 0), TypeError);
> +        assertThrowsInstanceOf(() => SIMD[kind].load(ta, "hello"), TypeError);

ToNumber("hello") == NaN, so RangeError.

@@ +130,3 @@
>          assertThrowsInstanceOf(() => SIMD[kind].load(ta, -1), RangeError);
> +        // Indexes must be integers, there is no rounding.
> +        assertThrowsInstanceOf(() => SIMD[kind].load(ta, 1.5), TypeError);

RangeError (numberIndex != getIndex, in DataView terms)

@@ +132,5 @@
> +        assertThrowsInstanceOf(() => SIMD[kind].load(ta, 1.5), TypeError);
> +        var obj = {
> +            valueOf: function() { return 12 }
> +        }
> +        assertThrowsInstanceOf(() => SIMD[kind].load(ta, obj), TypeError);

ToNumber should apply ToPrimitive with hint Number, which should invokve valueOf, which should return 12.  That should either work or be a RangeError.

Comment 6

[Jakob Stoklund Olesen [:jolesen]]

Attachment: SIMD: Coerce non-numeric indexes to load/store functions.

This implements the behavior discussed in
<https://github.com/tc39/ecmascript_simd/issues/328>, following the precedent
set by the specification of the DataView functions.

Note that our implementation of DataView uses ToInt32() which applies
ToNumber(), but then truncates to an integer value and wraps mod 2^32.

If we want to share the ArgumentToIntegerIndex() function between SIMD and
shared array functions, where would be a good place to define it?

Comment 7

[Lars T Hansen [:lth]]

Comment on attachment 8727625 [details] [diff] [review]
SIMD: Coerce non-numeric indexes to load/store functions.

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

OK.  We can keep the function here, I'll make an annotation on the bug on my side of the fence and then we can move it when I get around to fixing that, which won't be until after Easter.

::: js/src/builtin/SIMD.cpp
@@ +1261,5 @@
> +// laundered like this:
> +//
> +//   1. numericIndex = ToNumber(argument)            (may throw TypeError)
> +//   2. intIndex = ToInteger(numericIndex)
> +//   3. if intIndex != numericIndex throw RangeError

or if intIndex < 0

@@ +1289,5 @@
> +    // Check that |d| is an integer in the valid range.
> +    //
> +    // Not all floating point integers fit in the range of an int64_t, so we
> +    // need a rough range check before the real range check in our caller. The
> +    // ES ToLength() abstract function limits the length of a typed array to

It's really ToInteger that is relevant here, according to prose above and the DataView code.

@@ +1295,5 @@
> +    // 2^53 don't really make sense as not all integers above 2^53 are floating
> +    // point numbers.
> +    //
> +    // The exact value 2^53 is a floating point number, but it is not allowed by
> +    // ES ToNumber(). We'll do the same here.

Surely ToNumber does not have a problem with that?  ToLength does but ToLength is not relevant here.

@@ +1301,5 @@
> +    // Reject infinities, NaNs, and numbers outside the contiguous integer
> +    // range with a RangeError.
> +
> +    // Write relation so NaNs throw a RangeError.
> +    if (!(0 <= d && d <= UINT64_C(0x1fffffffffffff))) {

Might I ask for (uint64_t(1) << 53) - 1 here, for clarity?  (Java now allows underscores in literals like this but I don't know if C++ does.)

Comment 8

[Jakob Stoklund Olesen [:jolesen]]

(In reply to Lars T Hansen [:lth] from comment #7)
> Comment on attachment 8727625 [details] [diff] [review]
> SIMD: Coerce non-numeric indexes to load/store functions.
> 
> OK.  We can keep the function here, I'll make an annotation on the bug on my
> side of the fence and then we can move it when I get around to fixing that,
> which won't be until after Easter.

Makes sense.

> @@ +1295,5 @@
> > +    // 2^53 don't really make sense as not all integers above 2^53 are floating
> > +    // point numbers.
> > +    //
> > +    // The exact value 2^53 is a floating point number, but it is not allowed by
> > +    // ES ToNumber(). We'll do the same here.
> 
> Surely ToNumber does not have a problem with that?  ToLength does but
> ToLength is not relevant here.

You are right. I was struggling with this. We could allow integers > 2^53 here, but I would still need to impose a somewhat arbitrary UINT64_MAX limit before converting to uint64_t.

It is very convenient to have a bound significant smaller than UINT64_MAX, for example I immediately scale the index by the array's bytesPerElement, and I don't have to worry about overflow.

Imposing the 2^53 bound here is presuming that any array will be smaller than that (and you would be in trouble trying to index into something larger with doubles). I'll try to clean up the comment.

> 
> @@ +1301,5 @@
> > +    // Reject infinities, NaNs, and numbers outside the contiguous integer
> > +    // range with a RangeError.
> > +
> > +    // Write relation so NaNs throw a RangeError.
> > +    if (!(0 <= d && d <= UINT64_C(0x1fffffffffffff))) {
> 
> Might I ask for (uint64_t(1) << 53) - 1 here, for clarity?  (Java now allows
> underscores in literals like this but I don't know if C++ does.)

As of C++14, you can use ticks: UINT64_C(0x1f'ffff'ffff'ffff), but your suggestion is clearer.

Comment 9

[Lars T Hansen [:lth]]

(In reply to Jakob Stoklund Olesen [:jolesen] from comment #8)
> (In reply to Lars T Hansen [:lth] from comment #7)
> 
> > @@ +1295,5 @@
> > > +    // 2^53 don't really make sense as not all integers above 2^53 are floating
> > > +    // point numbers.
> > > +    //
> > > +    // The exact value 2^53 is a floating point number, but it is not allowed by
> > > +    // ES ToNumber(). We'll do the same here.
> > 
> > Surely ToNumber does not have a problem with that?  ToLength does but
> > ToLength is not relevant here.
> 
> You are right. I was struggling with this. We could allow integers > 2^53
> here, but I would still need to impose a somewhat arbitrary UINT64_MAX limit
> before converting to uint64_t.
> 
> It is very convenient to have a bound significant smaller than UINT64_MAX,
> for example I immediately scale the index by the array's bytesPerElement,
> and I don't have to worry about overflow.
> 
> Imposing the 2^53 bound here is presuming that any array will be smaller
> than that (and you would be in trouble trying to index into something larger
> with doubles). I'll try to clean up the comment.

Currently ArrayBuffers and SharedArrayBuffers are < 2^31 bytes, and I think a lot of work will be needed to change that, and I even made somebody (maybe arai?) change code that could have worked on larger arrays to work only with arrays up to that size, so you should probably feel free to change your code here, so long as we handle indices above 2^31-1 properly as out of bounds.

Comment 10

[Jakob Stoklund Olesen [:jolesen]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=91d39612f146

Comment 11

[Jakob Stoklund Olesen [:jolesen]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/61b9a39b0bb49ce7fe957ca670ed2169bbc9ae1e
Bug 1252270 - SIMD: Coerce non-numeric indexes to load/store functions. r=lth

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/61b9a39b0bb4