Closed
Bug 1256945
Opened 8 years ago
Closed 8 years ago
SIMD: Coerce lane indexes with ToNumber()
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
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.
Reporter | ||
Comment 1•8 years ago
|
||
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/
Reporter | ||
Updated•8 years ago
|
Attachment #8730869 -
Flags: review?(bbouvier)
Updated•8 years ago
|
Attachment #8730869 -
Flags: review?(bbouvier) → review+
Comment 2•8 years ago
|
||
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...
Reporter | ||
Comment 3•8 years ago
|
||
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`.
Reporter | ||
Comment 4•8 years ago
|
||
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
Comment 6•8 years ago
|
||
bugherder |
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.
Description
•