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

RESOLVED FIXED in Firefox 48

Status

()

Core
JavaScript Engine
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jolesen, Assigned: jolesen)

Tracking

(Blocks: 1 bug)

Trunk
mozilla48
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox47 affected, firefox48 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

2 years ago
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.
(Assignee)

Updated

2 years ago
Blocks: 894105
(Assignee)

Updated

2 years ago
Assignee: nobody → jolesen
(Assignee)

Comment 2

2 years ago
Created attachment 8725012 [details] [diff] [review]
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.
Attachment #8725012 - Flags: review?(lhansen)

Comment 3

2 years ago
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.
(Assignee)

Comment 4

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

Comment 5

2 years ago
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.
Attachment #8725012 - Flags: review?(lhansen)
(Assignee)

Comment 6

2 years ago
Created attachment 8727625 [details] [diff] [review]
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?
Attachment #8727625 - Flags: review?(lhansen)
(Assignee)

Updated

2 years ago
Attachment #8725012 - Attachment is obsolete: true

Comment 7

2 years ago
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.)
Attachment #8727625 - Flags: review?(lhansen) → review+
(Assignee)

Comment 8

2 years ago
(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

2 years ago
(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.
(Assignee)

Updated

2 years ago
Blocks: 1240791
Summary: SIMD: Reject non-numeric indexes to load/store functions → SIMD: Coerce non-numeric indexes to load/store functions

Comment 12

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/61b9a39b0bb4
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.