Closed
Bug 1252270
Opened 8 years ago
Closed 8 years ago
SIMD: Coerce non-numeric indexes to load/store functions
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla48
People
(Reporter: jolesen, Assigned: jolesen)
References
Details
Attachments
(1 file, 1 obsolete file)
9.14 KB,
patch
|
lth
:
review+
|
Details | Diff | Splinter Review |
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•8 years ago
|
Assignee: nobody → jolesen
Assignee | ||
Comment 1•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=658cbd996fed
Assignee | ||
Comment 2•8 years ago
|
||
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•8 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•8 years ago
|
||
Thanks, Lars. I agree, we should aim for some consistency.
Comment 5•8 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•8 years ago
|
||
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•8 years ago
|
Attachment #8725012 -
Attachment is obsolete: true
Comment 7•8 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•8 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•8 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 | ||
Comment 10•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=91d39612f146
Assignee | ||
Updated•8 years ago
|
Blocks: 1240791
Summary: SIMD: Reject non-numeric indexes to load/store functions → SIMD: Coerce non-numeric indexes to load/store functions
Assignee | ||
Comment 11•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/61b9a39b0bb49ce7fe957ca670ed2169bbc9ae1e Bug 1252270 - SIMD: Coerce non-numeric indexes to load/store functions. r=lth
Comment 12•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/61b9a39b0bb4
Status: NEW → RESOLVED
Closed: 8 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.
Description
•