Reland of Amend DataView, ArrayBuffer, and TypedArray methods to use ToIndex....

Reland of Amend DataView, ArrayBuffer, and TypedArray methods to use ToIndex. (patchset #2 id:170001 of https://codereview.chromium.org/2113593002/ )

Reason for revert:
WebGL tests have been updated and rolled (at https://codereview.chromium.org/2227023002), so this should no longer fail outdated tests.

Original issue's description:
> Revert of Amend DataView, ArrayBuffer, and TypedArray methods to use ToIndex. (patchset #8 id:140001 of https://codereview.chromium.org/2090353003/ )
>
> Reason for revert:
> Speculative revert to unblock roll: https://codereview.chromium.org/2107223003/
>
> Original issue's description:
> > Amend DataView, ArrayBuffer, and TypedArray methods to use ToIndex.
> >
> > The spec was modified to relax some requirements which implementors had not been
> > enforcing. Part of this process involved introducing a new abstract operation
> > ToIndex, which had partial overlap with our existing semantics as well as some
> > differences (most notably treating undefined as 0). Test262 tests were introduced to
> > check for the new semantics, some of which we were failing. This patch amends the
> > parts of our implementation corresponding to specification algorithms which use
> > ToIndex to follow its semantics precisely.
> >
> > BUG=v8:4784,v8:5120
> >
> > Committed: https://crrev.com/09720349ea058d178521ec58d0a5676443a5a132
> > Cr-Commit-Position: refs/heads/master@{#37406}
>
> TBR=littledan@chromium.org,adamk@chromium.org,bakkot@google.com
> # Skipping CQ checks because original CL landed less than 1 days ago.
> NOPRESUBMIT=true
> NOTREECHECKS=true
> NOTRY=true
> BUG=v8:4784,v8:5120
>
> Committed: https://crrev.com/b1f7f1f4e41a723d5f997738a07e35a031713b8f
> Cr-Commit-Position: refs/heads/master@{#37417}

TBR=littledan@chromium.org,adamk@chromium.org,hablich@chromium.org
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG=v8:4784,v8:5120

Review-Url: https://codereview.chromium.org/2247073004
Cr-Commit-Position: refs/heads/master@{#38689}

src/builtins/builtins-dataview.cc

@@ -38,65 +38,52 @@ BUILTIN(DataViewConstructor_ConstructStub) {
   }
   Handle<JSArrayBuffer> array_buffer = Handle<JSArrayBuffer>::cast(buffer);
 
-  // 4. Let numberOffset be ? ToNumber(byteOffset).
-  Handle<Object> number_offset;
-  if (byte_offset->IsUndefined(isolate)) {
-    // We intentionally violate the specification at this point to allow
-    // for new DataView(buffer) invocations to be equivalent to the full
-    // new DataView(buffer, 0) invocation.
-    number_offset = handle(Smi::FromInt(0), isolate);
-  } else {
-    ASSIGN_RETURN_FAILURE_ON_EXCEPTION(isolate, number_offset,
-                                       Object::ToNumber(byte_offset));
-  }
-
-  // 5. Let offset be ToInteger(numberOffset).
+  // 4. Let offset be ToIndex(byteOffset).
   Handle<Object> offset;
-  ASSIGN_RETURN_FAILURE_ON_EXCEPTION(isolate, offset,
-                                     Object::ToInteger(isolate, number_offset));
-
-  // 6. If numberOffset ≠ offset or offset < 0, throw a RangeError exception.
-  if (number_offset->Number() != offset->Number() || offset->Number() < 0.0) {
-    THROW_NEW_ERROR_RETURN_FAILURE(
-        isolate, NewRangeError(MessageTemplate::kInvalidDataViewOffset));
-  }
+  ASSIGN_RETURN_FAILURE_ON_EXCEPTION(
+      isolate, offset,
+      Object::ToIndex(isolate, byte_offset,
+                      MessageTemplate::kInvalidDataViewOffset));
 
-  // 7. If IsDetachedBuffer(buffer) is true, throw a TypeError exception.
+  // 5. If IsDetachedBuffer(buffer) is true, throw a TypeError exception.
   // We currently violate the specification at this point.
 
-  // 8. Let bufferByteLength be the value of buffer's [[ArrayBufferByteLength]]
+  // 6. Let bufferByteLength be the value of buffer's [[ArrayBufferByteLength]]
   // internal slot.
   double const buffer_byte_length = array_buffer->byte_length()->Number();
 
-  // 9. If offset > bufferByteLength, throw a RangeError exception
+  // 7. If offset > bufferByteLength, throw a RangeError exception
   if (offset->Number() > buffer_byte_length) {
     THROW_NEW_ERROR_RETURN_FAILURE(
-        isolate, NewRangeError(MessageTemplate::kInvalidDataViewOffset));
+        isolate,
+        NewRangeError(MessageTemplate::kInvalidDataViewOffset, offset));
   }
 
   Handle<Object> view_byte_length;
   if (byte_length->IsUndefined(isolate)) {
-    // 10. If byteLength is undefined, then
+    // 8. If byteLength is undefined, then
     //       a. Let viewByteLength be bufferByteLength - offset.
     view_byte_length =
         isolate->factory()->NewNumber(buffer_byte_length - offset->Number());
   } else {
-    // 11. Else,
-    //       a. Let viewByteLength be ? ToLength(byteLength).
+    // 9. Else,
+    //       a. Let viewByteLength be ? ToIndex(byteLength).
     //       b. If offset+viewByteLength > bufferByteLength, throw a RangeError
     //          exception
-    ASSIGN_RETURN_FAILURE_ON_EXCEPTION(isolate, view_byte_length,
-                                       Object::ToLength(isolate, byte_length));
+    ASSIGN_RETURN_FAILURE_ON_EXCEPTION(
+        isolate, view_byte_length,
+        Object::ToIndex(isolate, byte_length,
+                        MessageTemplate::kInvalidDataViewLength));
     if (offset->Number() + view_byte_length->Number() > buffer_byte_length) {
       THROW_NEW_ERROR_RETURN_FAILURE(
           isolate, NewRangeError(MessageTemplate::kInvalidDataViewLength));
     }
   }
 
-  // 12. Let O be ? OrdinaryCreateFromConstructor(NewTarget,
+  // 10. Let O be ? OrdinaryCreateFromConstructor(NewTarget,
   //     "%DataViewPrototype%", «[[DataView]], [[ViewedArrayBuffer]],
   //     [[ByteLength]], [[ByteOffset]]»).
-  // 13. Set O's [[DataView]] internal slot to true.
+  // 11. Set O's [[DataView]] internal slot to true.
   Handle<JSObject> result;
   ASSIGN_RETURN_FAILURE_ON_EXCEPTION(isolate, result,
                                      JSObject::New(target, new_target));
@@ -104,16 +91,16 @@ BUILTIN(DataViewConstructor_ConstructStub) {
     Handle<JSDataView>::cast(result)->SetInternalField(i, Smi::FromInt(0));
   }
 
-  // 14. Set O's [[ViewedArrayBuffer]] internal slot to buffer.
+  // 12. Set O's [[ViewedArrayBuffer]] internal slot to buffer.
   Handle<JSDataView>::cast(result)->set_buffer(*array_buffer);
 
-  // 15. Set O's [[ByteLength]] internal slot to viewByteLength.
+  // 13. Set O's [[ByteLength]] internal slot to viewByteLength.
   Handle<JSDataView>::cast(result)->set_byte_length(*view_byte_length);
 
-  // 16. Set O's [[ByteOffset]] internal slot to offset.
+  // 14. Set O's [[ByteOffset]] internal slot to offset.
   Handle<JSDataView>::cast(result)->set_byte_offset(*offset);
 
-  // 17. Return O.
+  // 15. Return O.
   return *result;
 }
 

src/js/runtime.js

@@ -41,6 +41,13 @@ function ToPositiveInteger(x, rangeErrorIndex) {
 }
 
 
+function ToIndex(x, rangeErrorIndex) {
+  var i = TO_INTEGER(x) + 0;
+  if (i < 0 || i > kMaxSafeInteger) throw %make_range_error(rangeErrorIndex);
+  return i;
+}
+
+
 function MaxSimple(a, b) {
   return a > b ? a : b;
 }
@@ -90,6 +97,7 @@ utils.Export(function(to) {
   to.MaxSimple = MaxSimple;
   to.MinSimple = MinSimple;
   to.ToPositiveInteger = ToPositiveInteger;
+  to.ToIndex = ToIndex;
   to.SpeciesConstructor = SpeciesConstructor;
 });
 

src/js/typedarray.js

@@ -41,6 +41,7 @@ var MinSimple;
 var PackedArrayReverse;
 var SpeciesConstructor;
 var ToPositiveInteger;
+var ToIndex;
 var iteratorSymbol = utils.ImportNow("iterator_symbol");
 var speciesSymbol = utils.ImportNow("species_symbol");
 var toStringTagSymbol = utils.ImportNow("to_string_tag_symbol");
@@ -89,6 +90,7 @@ utils.Import(function(from) {
   PackedArrayReverse = from.PackedArrayReverse;
   SpeciesConstructor = from.SpeciesConstructor;
   ToPositiveInteger = from.ToPositiveInteger;
+  ToIndex = from.ToIndex;
 });
 
 // --------------- Typed Arrays ---------------------
@@ -133,10 +135,15 @@ function TypedArraySpeciesCreate(exemplar, arg0, arg1, arg2, conservative) {
 macro TYPED_ARRAY_CONSTRUCTOR(ARRAY_ID, NAME, ELEMENT_SIZE)
 function NAMEConstructByArrayBuffer(obj, buffer, byteOffset, length) {
   if (!IS_UNDEFINED(byteOffset)) {
-    byteOffset = ToPositiveInteger(byteOffset, kInvalidTypedArrayLength);
+    byteOffset = ToIndex(byteOffset, kInvalidTypedArrayLength);
   }
   if (!IS_UNDEFINED(length)) {
-    length = ToPositiveInteger(length, kInvalidTypedArrayLength);
+    length = ToIndex(length, kInvalidTypedArrayLength);
+  }
+  if (length > %_MaxSmi()) {
+    // Note: this is not per spec, but rather a constraint of our current
+    // representation (which uses smi's).
+    throw %make_range_error(kInvalidTypedArrayLength);
   }
 
   var bufferByteLength = %_ArrayBufferGetByteLength(buffer);
@@ -150,35 +157,34 @@ function NAMEConstructByArrayBuffer(obj, buffer, byteOffset, length) {
       throw %make_range_error(kInvalidTypedArrayAlignment,
                            "start offset", "NAME", ELEMENT_SIZE);
     }
-    if (offset > bufferByteLength) {
-      throw %make_range_error(kInvalidTypedArrayOffset);
-    }
   }
 
   var newByteLength;
-  var newLength;
   if (IS_UNDEFINED(length)) {
     if (bufferByteLength % ELEMENT_SIZE !== 0) {
       throw %make_range_error(kInvalidTypedArrayAlignment,
                            "byte length", "NAME", ELEMENT_SIZE);
     }
     newByteLength = bufferByteLength - offset;
-    newLength = newByteLength / ELEMENT_SIZE;
+    if (newByteLength < 0) {
+      throw %make_range_error(kInvalidTypedArrayAlignment,
+                           "byte length", "NAME", ELEMENT_SIZE);
+    }
   } else {
-    var newLength = length;
-    newByteLength = newLength * ELEMENT_SIZE;
-  }
-  if ((offset + newByteLength > bufferByteLength)
-      || (newLength > %_MaxSmi())) {
-    throw %make_range_error(kInvalidTypedArrayLength);
+    newByteLength = length * ELEMENT_SIZE;
+    if (offset + newByteLength > bufferByteLength) {
+      throw %make_range_error(kInvalidTypedArrayLength);
+    }
   }
   %_TypedArrayInitialize(obj, ARRAY_ID, buffer, offset, newByteLength, true);
 }
 
 function NAMEConstructByLength(obj, length) {
   var l = IS_UNDEFINED(length) ?
-    0 : ToPositiveInteger(length, kInvalidTypedArrayLength);
-  if (l > %_MaxSmi()) {
+    0 : ToIndex(length, kInvalidTypedArrayLength);
+  if (length > %_MaxSmi()) {
+    // Note: this is not per spec, but rather a constraint of our current
+    // representation (which uses smi's).
     throw %make_range_error(kInvalidTypedArrayLength);
   }
   var byteLength = l * ELEMENT_SIZE;
@@ -930,8 +936,7 @@ function DataViewGetTYPENAMEJS(offset, little_endian) {
     throw %make_type_error(kIncompatibleMethodReceiver,
                         'DataView.getTYPENAME', this);
   }
-  if (arguments.length < 1) throw %make_type_error(kInvalidArgument);
-  offset = ToPositiveInteger(offset, kInvalidDataViewAccessorOffset);
+  offset = IS_UNDEFINED(offset) ? 0 : ToIndex(offset, kInvalidDataViewAccessorOffset);
   return %DataViewGetTYPENAME(this, offset, !!little_endian);
 }
 %FunctionSetLength(DataViewGetTYPENAMEJS, 1);
@@ -941,8 +946,7 @@ function DataViewSetTYPENAMEJS(offset, value, little_endian) {
     throw %make_type_error(kIncompatibleMethodReceiver,
                         'DataView.setTYPENAME', this);
   }
-  if (arguments.length < 2) throw %make_type_error(kInvalidArgument);
-  offset = ToPositiveInteger(offset, kInvalidDataViewAccessorOffset);
+  offset = IS_UNDEFINED(offset) ? 0 : ToIndex(offset, kInvalidDataViewAccessorOffset);
   %DataViewSetTYPENAME(this, offset, TO_NUMBER(value), !!little_endian);
 }
 %FunctionSetLength(DataViewSetTYPENAMEJS, 2);

src/messages.h

@@ -331,8 +331,9 @@ class ErrorUtils : public AllStatic {
   T(InvalidCurrencyCode, "Invalid currency code: %")                           \
   T(InvalidDataViewAccessorOffset,                                             \
     "Offset is outside the bounds of the DataView")                            \
-  T(InvalidDataViewLength, "Invalid data view length")                         \
-  T(InvalidDataViewOffset, "Start offset is outside the bounds of the buffer") \
+  T(InvalidDataViewLength, "Invalid DataView length %")                        \
+  T(InvalidDataViewOffset,                                                     \
+    "Start offset % is outside the bounds of the buffer")                      \
   T(InvalidHint, "Invalid hint: %")                                            \
   T(InvalidLanguageTag, "Invalid language tag: %")                             \
   T(InvalidWeakMapKey, "Invalid value used as weak map key")                   \

src/objects.cc

@@ -383,6 +383,18 @@ MaybeHandle<Object> Object::ToLength(Isolate* isolate, Handle<Object> input) {
   return isolate->factory()->NewNumber(len);
 }
 
+// static
+MaybeHandle<Object> Object::ToIndex(Isolate* isolate, Handle<Object> input,
+                                    MessageTemplate::Template error_index) {
+  if (input->IsUndefined(isolate)) return isolate->factory()->NewNumber(0.0);
+  ASSIGN_RETURN_ON_EXCEPTION(isolate, input, ToNumber(input), Object);
+  double len = DoubleToInteger(input->Number()) + 0.0;
+  auto js_len = isolate->factory()->NewNumber(len);
+  if (len < 0.0 || len > kMaxSafeInteger) {
+    THROW_NEW_ERROR(isolate, NewRangeError(error_index, js_len), Object);
+  }
+  return js_len;
+}
 
 bool Object::BooleanValue() {
   if (IsSmi()) return Smi::cast(this)->value() != 0;

src/objects.h

@@ -18,6 +18,7 @@
 #include "src/field-index.h"
 #include "src/flags.h"
 #include "src/list.h"
+#include "src/messages.h"
 #include "src/property-details.h"
 #include "src/unicode-decoder.h"
 #include "src/unicode.h"
@@ -1211,6 +1212,11 @@ class Object {
   MUST_USE_RESULT static MaybeHandle<Object> ToLength(Isolate* isolate,
                                                       Handle<Object> input);
 
+  // ES6 section 7.1.17 ToIndex
+  MUST_USE_RESULT static MaybeHandle<Object> ToIndex(
+      Isolate* isolate, Handle<Object> input,
+      MessageTemplate::Template error_index);
+
   // ES6 section 7.3.9 GetMethod
   MUST_USE_RESULT static MaybeHandle<Object> GetMethod(
       Handle<JSReceiver> receiver, Handle<Name> name);

test/mjsunit/harmony/dataview-accessors.js

@@ -400,12 +400,8 @@ function TestGeneralAccessors() {
     assertThrows(function() { f(); }, TypeError);
     f.call(a, 0, 0); // should not throw
     assertThrows(function() { f.call({}, 0, 0); }, TypeError);
-    assertThrows(function() { f.call(a); }, TypeError);
-    if (name.indexOf("set") == 0) {
-      assertThrows(function() { f.call(a, 1); }, TypeError);
-    } else {
-      f.call(a, 1); // should not throw
-    }
+    f.call(a);
+    f.call(a, 1); // should not throw
   }
   CheckAccessor("getUint8");
   CheckAccessor("setUint8");
@@ -429,33 +425,27 @@ TestGeneralAccessors();
 
 function TestInsufficientArguments() {
   var a = new DataView(new ArrayBuffer(256));
+  function CheckInsuficientArguments(type) {
+    var expectedValue = type === "Float32" || type === "Float64" ? NaN : 0;
+    var offset = getElementSize(type);
 
-  assertThrows(function() { a.getUint8(); }, TypeError);
-  assertThrows(function() { a.getInt8(); }, TypeError);
-  assertThrows(function() { a.getUint16(); }, TypeError);
-  assertThrows(function() { a.getInt16(); }, TypeError);
-  assertThrows(function() { a.getUint32(); }, TypeError);
-  assertThrows(function() { a.getInt32(); }, TypeError);
-  assertThrows(function() { a.getFloat32(); }, TypeError);
-  assertThrows(function() { a.getFloat64(); }, TypeError);
-
-  assertThrows(function() { a.setUint8(); }, TypeError);
-  assertThrows(function() { a.setInt8(); }, TypeError);
-  assertThrows(function() { a.setUint16(); }, TypeError);
-  assertThrows(function() { a.setInt16(); }, TypeError);
-  assertThrows(function() { a.setUint32(); }, TypeError);
-  assertThrows(function() { a.setInt32(); }, TypeError);
-  assertThrows(function() { a.setFloat32(); }, TypeError);
-  assertThrows(function() { a.setFloat64(); }, TypeError);
-
-  assertThrows(function() { a.setUint8(1) }, TypeError);
-  assertThrows(function() { a.setInt8(1) }, TypeError);
-  assertThrows(function() { a.setUint16(1) }, TypeError);
-  assertThrows(function() { a.setInt16(1) }, TypeError);
-  assertThrows(function() { a.setUint32(1) }, TypeError);
-  assertThrows(function() { a.setInt32(1) }, TypeError);
-  assertThrows(function() { a.setFloat32(1) }, TypeError);
-  assertThrows(function() { a.setFloat64(1) }, TypeError);
+    assertSame(undefined, a["set" + type](0, 7));
+    assertSame(undefined, a["set" + type]());
+    assertSame(expectedValue, a["get" + type]());
+
+    assertSame(undefined, a["set" + type](offset, 7));
+    assertSame(undefined, a["set" + type](offset));
+    assertSame(expectedValue, a["get" + type](offset));
+  }
+
+  CheckInsuficientArguments("Uint8");
+  CheckInsuficientArguments("Int8");
+  CheckInsuficientArguments("Uint16");
+  CheckInsuficientArguments("Int16");
+  CheckInsuficientArguments("Uint32");
+  CheckInsuficientArguments("Int32");
+  CheckInsuficientArguments("Float32");
+  CheckInsuficientArguments("Float64");
 }
 
 TestInsufficientArguments();

test/test262/test262.status

@@ -355,34 +355,6 @@
   'annexB/built-ins/Object/prototype/__lookupGetter__/this-non-obj': [FAIL],
   'annexB/built-ins/Object/prototype/__lookupSetter__/this-non-obj': [FAIL],
 
-  # https://bugs.chromium.org/p/v8/issues/detail?id=5120
-  'built-ins/DataView/negative-bytelength-throws': [FAIL],
-  'built-ins/DataView/prototype/getFloat32/toindex-byteoffset': [FAIL],
-  'built-ins/DataView/prototype/getFloat64/toindex-byteoffset': [FAIL],
-  'built-ins/DataView/prototype/getInt16/toindex-byteoffset': [FAIL],
-  'built-ins/DataView/prototype/getInt32/toindex-byteoffset': [FAIL],
-  'built-ins/DataView/prototype/getInt8/toindex-byteoffset': [FAIL],
-  'built-ins/DataView/prototype/getUint16/toindex-byteoffset': [FAIL],
-  'built-ins/DataView/prototype/getUint32/toindex-byteoffset': [FAIL],
-  'built-ins/DataView/prototype/getUint8/toindex-byteoffset': [FAIL],
-  'built-ins/DataView/prototype/setFloat32/no-value-arg': [FAIL],
-  'built-ins/DataView/prototype/setFloat32/toindex-byteoffset': [FAIL],
-  'built-ins/DataView/prototype/setFloat64/no-value-arg': [FAIL],
-  'built-ins/DataView/prototype/setFloat64/toindex-byteoffset': [FAIL],
-  'built-ins/DataView/prototype/setInt16/no-value-arg': [FAIL],
-  'built-ins/DataView/prototype/setInt16/toindex-byteoffset': [FAIL],
-  'built-ins/DataView/prototype/setInt32/no-value-arg': [FAIL],
-  'built-ins/DataView/prototype/setInt32/toindex-byteoffset': [FAIL],
-  'built-ins/DataView/prototype/setInt8/no-value-arg': [FAIL],
-  'built-ins/DataView/prototype/setInt8/toindex-byteoffset': [FAIL],
-  'built-ins/DataView/prototype/setUint16/no-value-arg': [FAIL],
-  'built-ins/DataView/prototype/setUint16/toindex-byteoffset': [FAIL],
-  'built-ins/DataView/prototype/setUint32/no-value-arg': [FAIL],
-  'built-ins/DataView/prototype/setUint32/toindex-byteoffset': [FAIL],
-  'built-ins/DataView/prototype/setUint8/no-value-arg': [FAIL],
-  'built-ins/DataView/prototype/setUint8/toindex-byteoffset': [FAIL],
-  'built-ins/DataView/toindex-byteoffset': [FAIL],
-
   # https://bugs.chromium.org/p/v8/issues/detail?id=5121
   'language/expressions/assignment/destructuring/obj-prop-__proto__dup': [FAIL],
 
@@ -522,6 +494,20 @@
   # https://github.com/tc39/test262/issues/694
   'language/statements/class/subclass/builtin-objects/ArrayBuffer/regular-subclassing': [FAIL],
 
+  # https://github.com/tc39/test262/issues/685
+  'built-ins/DataView/prototype/setUint8/range-check-after-value-conversion': [FAIL],
+  'built-ins/DataView/prototype/setUint16/range-check-after-value-conversion': [FAIL],
+  'built-ins/DataView/prototype/setUint32/range-check-after-value-conversion': [FAIL],
+  'built-ins/DataView/prototype/setInt8/range-check-after-value-conversion': [FAIL],
+  'built-ins/DataView/prototype/setInt16/range-check-after-value-conversion': [FAIL],
+  'built-ins/DataView/prototype/setInt32/range-check-after-value-conversion': [FAIL],
+  'built-ins/DataView/prototype/setFloat32/range-check-after-value-conversion': [FAIL],
+  'built-ins/DataView/prototype/setFloat64/range-check-after-value-conversion': [FAIL],
+
+  # https://github.com/tc39/test262/issues/686
+  'built-ins/DataView/prototype/setFloat32/toindex-byteoffset': [FAIL],
+  'built-ins/DataView/prototype/setFloat64/toindex-byteoffset': [FAIL],
+
   ############################ SKIPPED TESTS #############################
 
   # These tests take a looong time to run.