Bug 1252270 - SIMD: Coerce non-numeric indexes to load/store functions. r=lth
authorJakob Olesen <jolesen@mozilla.com>
Mon, 14 Mar 2016 12:19:09 -0700
changeset 288625 61b9a39b0bb49ce7fe957ca670ed2169bbc9ae1e
parent 288624 a1a8876174c3258948d979707b2df1b913956902
child 288626 f40c7931cc6b15d929a5df9dc8cecffeda268439
push id18174
push usercbook@mozilla.com
push dateTue, 15 Mar 2016 09:44:58 +0000
treeherderfx-team@dd0baa33759d [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerslth
bugs1252270
milestone48.0a1
Bug 1252270 - SIMD: Coerce non-numeric indexes to load/store functions. r=lth Follow the DataView functions and use Tonumber to coerce index arguments on the load/store functions. Throw a RangeError when we see a non-integer index or a number outside the range of the array. See https://github.com/tc39/ecmascript_simd/issues/328 MozReview-Commit-ID: IpHkfPyywU0
js/src/builtin/SIMD.cpp
js/src/tests/ecma_7/SIMD/load.js
--- a/js/src/builtin/SIMD.cpp
+++ b/js/src/builtin/SIMD.cpp
@@ -1253,59 +1253,131 @@ Select(JSContext* cx, unsigned argc, Val
 
     Elem result[V::lanes];
     for (unsigned i = 0; i < V::lanes; i++)
         result[i] = mask[i] ? tv[i] : fv[i];
 
     return StoreResult<V>(cx, args, result);
 }
 
-template<class VElem, unsigned NumElem>
+// Get an integer array index from a function argument. Coerce if necessary.
+//
+// When a JS function argument represents an integer index into an array, it is
+// laundered like this:
+//
+//   1. numericIndex = ToNumber(argument)            (may throw TypeError)
+//   2. intIndex = ToInteger(numericIndex)
+//   3. if intIndex != numericIndex throw RangeError
+//
+// This function additionally bounds the range to the non-negative contiguous
+// integers:
+//
+//   4. if intIndex < 0 or intIndex > 2^53 throw RangeError
+//
+// Return true and set |*index| to the integer value if |argument| is a valid
+// array index argument. Otherwise report an TypeError or RangeError and return
+// false.
+//
+// The returned index will always be in the range 0 <= *index <= 2^53.
 static bool
-TypedArrayFromArgs(JSContext* cx, const CallArgs& args,
-                   MutableHandleObject typedArray, int32_t* byteStart)
+ArgumentToIntegerIndex(JSContext* cx, JS::HandleValue v, uint64_t* index)
+{
+    // Fast common case.
+    if (v.isInt32()) {
+        int32_t i = v.toInt32();
+        if (i >= 0) {
+            *index = i;
+            return true;
+        }
+    }
+
+    // Slow case. Use ToNumber() to coerce. This may throw a TypeError.
+    double d;
+    if (!ToNumber(cx, v, &d))
+        return false;
+
+    // Check that |d| is an integer in the valid range.
+    //
+    // Not all floating point integers fit in the range of a uint64_t, so we
+    // need a rough range check before the real range check in our caller. We
+    // could limit indexes to UINT64_MAX, but this would mean that our callers
+    // have to be very careful about integer overflow. The contiguous integer
+    // floating point numbers end at 2^53, so make that our upper limit. If we
+    // ever support arrays with more than 2^53 elements, this will need to
+    // change.
+    //
+    // 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_t(1) << 53))) {
+        JS_ReportErrorNumber(cx, GetErrorMessage, nullptr, JSMSG_BAD_INDEX);
+        return false;
+    }
+
+    // Check that d is an integer, throw a RangeError if not.
+    // Note that this conversion could invoke undefined behaviour without the
+    // range check above.
+    uint64_t i(d);
+    if (d != double(i)) {
+        JS_ReportErrorNumber(cx, GetErrorMessage, nullptr, JSMSG_BAD_INDEX);
+        return false;
+    }
+
+    *index = i;
+    return true;
+}
+
+// Look for arguments (ta, idx) where ta is a TypedArray and idx is a
+// non-negative integer.
+// Check that accessBytes can be accessed starting from index idx in the array.
+// Return the array handle in typedArray and idx converted to a byte offset in byteStart.
+static bool
+TypedArrayFromArgs(JSContext* cx, const CallArgs& args, uint32_t accessBytes,
+                   MutableHandleObject typedArray, size_t* byteStart)
 {
     if (!args[0].isObject())
         return ErrorBadArgs(cx);
 
     JSObject& argobj = args[0].toObject();
     if (!argobj.is<TypedArrayObject>())
         return ErrorBadArgs(cx);
 
     typedArray.set(&argobj);
 
-    int32_t index;
-    if (!ToInt32(cx, args[1], &index))
+    uint64_t index;
+    if (!ArgumentToIntegerIndex(cx, args[1], &index))
         return false;
 
-    *byteStart = index * typedArray->as<TypedArrayObject>().bytesPerElement();
-    if (*byteStart < 0 || (uint32_t(*byteStart) + NumElem * sizeof(VElem)) >
-                          typedArray->as<TypedArrayObject>().byteLength())
-    {
+    // Do the range check in 64 bits even when size_t is 32 bits.
+    // This can't overflow because index <= 2^53.
+    uint64_t bytes = index * typedArray->as<TypedArrayObject>().bytesPerElement();
+    if ((bytes + accessBytes) > typedArray->as<TypedArrayObject>().byteLength()) {
         // Keep in sync with AsmJS OnOutOfBounds function.
         JS_ReportErrorNumber(cx, GetErrorMessage, nullptr, JSMSG_BAD_INDEX);
         return false;
     }
+    *byteStart = bytes;
 
     return true;
 }
 
 template<class V, unsigned NumElem>
 static bool
 Load(JSContext* cx, unsigned argc, Value* vp)
 {
     typedef typename V::Elem Elem;
 
     CallArgs args = CallArgsFromVp(argc, vp);
     if (args.length() != 2)
         return ErrorBadArgs(cx);
 
-    int32_t byteStart;
+    size_t byteStart;
     RootedObject typedArray(cx);
-    if (!TypedArrayFromArgs<Elem, NumElem>(cx, args, &typedArray, &byteStart))
+    if (!TypedArrayFromArgs(cx, args, sizeof(Elem) * NumElem, &typedArray, &byteStart))
         return false;
 
     Rooted<TypeDescr*> typeDescr(cx, GetTypeDescr<V>(cx));
     if (!typeDescr)
         return false;
 
     Rooted<TypedObject*> result(cx, TypedObject::createZeroed(cx, typeDescr, 0));
     if (!result)
@@ -1325,19 +1397,19 @@ static bool
 Store(JSContext* cx, unsigned argc, Value* vp)
 {
     typedef typename V::Elem Elem;
 
     CallArgs args = CallArgsFromVp(argc, vp);
     if (args.length() != 3)
         return ErrorBadArgs(cx);
 
-    int32_t byteStart;
+    size_t byteStart;
     RootedObject typedArray(cx);
-    if (!TypedArrayFromArgs<Elem, NumElem>(cx, args, &typedArray, &byteStart))
+    if (!TypedArrayFromArgs(cx, args, sizeof(Elem) * NumElem, &typedArray, &byteStart))
         return false;
 
     if (!IsVectorObject<V>(args[2]))
         return ErrorBadArgs(cx);
 
     Elem* src = TypedObjectMemory<Elem*>(args[2]);
     SharedMem<Elem*> dst =
         typedArray->as<TypedArrayObject>().viewDataEither().addBytes(byteStart).cast<Elem*>();
--- a/js/src/tests/ecma_7/SIMD/load.js
+++ b/js/src/tests/ecma_7/SIMD/load.js
@@ -121,17 +121,28 @@ function testLoad(kind, TA) {
                     new Float32Array(TA.buffer),
                     new Float64Array(TA.buffer)
                    ])
     {
         // Invalid args
         assertThrowsInstanceOf(() => SIMD[kind].load(), TypeError);
         assertThrowsInstanceOf(() => SIMD[kind].load(ta), TypeError);
         assertThrowsInstanceOf(() => SIMD[kind].load("hello", 0), TypeError);
+        // Indexes must be integers, there is no rounding.
+        assertThrowsInstanceOf(() => SIMD[kind].load(ta, 1.5), RangeError);
         assertThrowsInstanceOf(() => SIMD[kind].load(ta, -1), RangeError);
+        assertThrowsInstanceOf(() => SIMD[kind].load(ta, "hello"), RangeError);
+        assertThrowsInstanceOf(() => SIMD[kind].load(ta, NaN), RangeError);
+        // Try to trip up the bounds checking. Int32 is enough for everybody.
+        assertThrowsInstanceOf(() => SIMD[kind].load(ta, 0x100000000), RangeError);
+        assertThrowsInstanceOf(() => SIMD[kind].load(ta, 0x80000000), RangeError);
+        assertThrowsInstanceOf(() => SIMD[kind].load(ta, 0x40000000), RangeError);
+        assertThrowsInstanceOf(() => SIMD[kind].load(ta, 0x20000000), RangeError);
+        assertThrowsInstanceOf(() => SIMD[kind].load(ta, (1<<30) * (1<<23) - 1), RangeError);
+        assertThrowsInstanceOf(() => SIMD[kind].load(ta, (1<<30) * (1<<23)), RangeError);
 
         // Valid and invalid reads
         var C = MakeComparator(kind, ta);
         var bpe = ta.BYTES_PER_ELEMENT;
 
         var lastValidArgLoad1   = (SIZE_BYTES - (16 / lanes))  / bpe | 0;
         var lastValidArgLoad2   = (SIZE_BYTES - 8)  / bpe | 0;
         var lastValidArgLoad3   = (SIZE_BYTES - 12) / bpe | 0;
@@ -164,23 +175,25 @@ function testLoad(kind, TA) {
         assertThrowsInstanceOf(() => SIMD[kind].load(ta, lastValidArgLoad + 1), RangeError);
         if (lanes <= 4) {
             assertThrowsInstanceOf(() => SIMD[kind].load1(ta, lastValidArgLoad1 + 1), RangeError);
         }
         if (lanes == 4) {
             assertThrowsInstanceOf(() => SIMD[kind].load2(ta, lastValidArgLoad2 + 1), RangeError);
             assertThrowsInstanceOf(() => SIMD[kind].load3(ta, lastValidArgLoad3 + 1), RangeError);
         }
+
+        // Indexes are coerced with ToNumber. Try some strings that
+        // CanonicalNumericIndexString() would reject.
+        C.load("1.0e0");
+        C.load(" 2");
     }
 
     if (lanes == 4) {
-        // Test ToInt32 behavior
-        var v = SIMD[kind].load(TA, 12.5);
-        assertEqX4(v, [12, 13, 14, 15]);
-
+        // Test ToNumber behavior.
         var obj = {
             valueOf: function() { return 12 }
         }
         var v = SIMD[kind].load(TA, obj);
         assertEqX4(v, [12, 13, 14, 15]);
     }
 
     var obj = {