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

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

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

src/builtins.cc

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

src/js/runtime.js

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

src/js/typedarray.js

@@ -46,7 +46,6 @@ 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");
@@ -100,7 +99,6 @@ utils.Import(function(from) {
   PackedArrayReverse = from.PackedArrayReverse;
   SpeciesConstructor = from.SpeciesConstructor;
   ToPositiveInteger = from.ToPositiveInteger;
-  ToIndex = from.ToIndex;
 });
 
 // --------------- Typed Arrays ---------------------
@@ -145,15 +143,10 @@ 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 = ToIndex(byteOffset, kInvalidTypedArrayLength);
+    byteOffset = ToPositiveInteger(byteOffset, kInvalidTypedArrayLength);
   }
   if (!IS_UNDEFINED(length)) {
-    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 MakeRangeError(kInvalidTypedArrayLength);
+    length = ToPositiveInteger(length, kInvalidTypedArrayLength);
   }
 
   var bufferByteLength = %_ArrayBufferGetByteLength(buffer);
@@ -167,35 +160,35 @@ function NAMEConstructByArrayBuffer(obj, buffer, byteOffset, length) {
       throw MakeRangeError(kInvalidTypedArrayAlignment,
                            "start offset", "NAME", ELEMENT_SIZE);
     }
+    if (offset > bufferByteLength) {
+      throw MakeRangeError(kInvalidTypedArrayOffset);
+    }
   }
 
   var newByteLength;
+  var newLength;
   if (IS_UNDEFINED(length)) {
     if (bufferByteLength % ELEMENT_SIZE !== 0) {
       throw MakeRangeError(kInvalidTypedArrayAlignment,
                            "byte length", "NAME", ELEMENT_SIZE);
     }
     newByteLength = bufferByteLength - offset;
-    if (newByteLength < 0) {
-      throw MakeRangeError(kInvalidTypedArrayAlignment,
-                           "byte length", "NAME", ELEMENT_SIZE);
-    }
+    newLength = newByteLength / ELEMENT_SIZE;
   } else {
-    newByteLength = length * ELEMENT_SIZE;
-    if (offset + newByteLength > bufferByteLength) {
-      throw MakeRangeError(kInvalidTypedArrayAlignment,
-                           "byte length", "NAME", ELEMENT_SIZE);
-    }
+    var newLength = length;
+    newByteLength = newLength * ELEMENT_SIZE;
+  }
+  if ((offset + newByteLength > bufferByteLength)
+      || (newLength > %_MaxSmi())) {
+    throw MakeRangeError(kInvalidTypedArrayLength);
   }
   %_TypedArrayInitialize(obj, ARRAY_ID, buffer, offset, newByteLength, true);
 }
 
 function NAMEConstructByLength(obj, length) {
   var l = IS_UNDEFINED(length) ?
-    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).
+    0 : ToPositiveInteger(length, kInvalidTypedArrayLength);
+  if (l > %_MaxSmi()) {
     throw MakeRangeError(kInvalidTypedArrayLength);
   }
   var byteLength = l * ELEMENT_SIZE;
@@ -872,7 +865,8 @@ function DataViewGetTYPENAMEJS(offset, little_endian) {
     throw MakeTypeError(kIncompatibleMethodReceiver,
                         'DataView.getTYPENAME', this);
   }
-  offset = IS_UNDEFINED(offset) ? 0 : ToIndex(offset, kInvalidDataViewAccessorOffset);
+  if (arguments.length < 1) throw MakeTypeError(kInvalidArgument);
+  offset = ToPositiveInteger(offset, kInvalidDataViewAccessorOffset);
   return %DataViewGetTYPENAME(this, offset, !!little_endian);
 }
 %FunctionSetLength(DataViewGetTYPENAMEJS, 1);
@@ -882,7 +876,8 @@ function DataViewSetTYPENAMEJS(offset, value, little_endian) {
     throw MakeTypeError(kIncompatibleMethodReceiver,
                         'DataView.setTYPENAME', this);
   }
-  offset = IS_UNDEFINED(offset) ? 0 : ToIndex(offset, kInvalidDataViewAccessorOffset);
+  if (arguments.length < 2) throw MakeTypeError(kInvalidArgument);
+  offset = ToPositiveInteger(offset, kInvalidDataViewAccessorOffset);
   %DataViewSetTYPENAME(this, offset, TO_NUMBER(value), !!little_endian);
 }
 %FunctionSetLength(DataViewSetTYPENAMEJS, 2);

src/messages.h

@@ -327,9 +327,8 @@ class CallSite {
   T(InvalidCurrencyCode, "Invalid currency code: %")                           \
   T(InvalidDataViewAccessorOffset,                                             \
     "Offset is outside the bounds of the DataView")                            \
-  T(InvalidDataViewLength, "Invalid DataView length %")                        \
-  T(InvalidDataViewOffset,                                                     \
-    "Start offset % is outside the bounds of the buffer")                      \
+  T(InvalidDataViewLength, "Invalid data view 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

@@ -228,18 +228,6 @@ 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,10 +18,9 @@
 #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"
+#include "src/unicode-decoder.h"
 #include "src/zone.h"
 
 #if V8_TARGET_ARCH_ARM
@@ -1178,11 +1177,6 @@ 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,8 +400,12 @@ function TestGeneralAccessors() {
     assertThrows(function() { f(); }, TypeError);
     f.call(a, 0, 0); // should not throw
     assertThrows(function() { f.call({}, 0, 0); }, TypeError);
-    f.call(a);
-    f.call(a, 1); // should not throw
+    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
+    }
   }
   CheckAccessor("getUint8");
   CheckAccessor("setUint8");
@@ -425,27 +429,33 @@ 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);
 
-    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");
+  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);
 }
 
 TestInsufficientArguments();

test/test262/test262.status

@@ -357,6 +357,34 @@
   '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],
 
@@ -496,20 +524,6 @@
   # https://github.com/tc39/test262/issues/696
   '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.