[runtime] Fix spec bug in TypedArrayConstruct with mutating iterables.

The spec requires that we use IterableToList, which we skipped for
some arrays as an optimization. We can't skip this for arrays with
objects though, because the objects may mutate the array during
the copying step via valueOf side effects.

Also clean up the implementation to use a runtime function rather
than a builtin as the helper. Also reverses the result of the helper
because I think it is a bit more intuitive that way.

Bug: v8:6224
Change-Id: I9199491abede4479785df6d9068331bc2d6e9c5e
Reviewed-on: https://chromium-review.googlesource.com/471986Reviewed-by: Camillo Bruni <cbruni@chromium.org>
Commit-Queue: Peter Marshall <petermarshall@chromium.org>
Cr-Commit-Position: refs/heads/master@{#44507}

src/bootstrapper.cc

@@ -1524,11 +1524,6 @@ void Genesis::InitializeGlobal(Handle<JSGlobalObject> global_object,
         array_function, isolate->factory()->InternalizeUtf8String("isArray"),
         Builtins::kArrayIsArray, 1, true);
     native_context()->set_is_arraylike(*is_arraylike);
-
-    Handle<JSFunction> has_side_effects = SimpleCreateFunction(
-        isolate, factory->NewStringFromAsciiChecked("hasIterationSideEffects"),
-        Builtins::kHasIterationSideEffects, 1, false);
-    native_context()->set_has_iteration_side_effects(*has_side_effects);
   }
 
   {  // --- A r r a y I t e r a t o r ---

src/builtins/builtins-array.cc

@@ -1239,14 +1239,5 @@ BUILTIN(ArrayConcat) {
   return Slow_ArrayConcat(&args, species, isolate);
 }
 
-BUILTIN(HasIterationSideEffects) {
-  HandleScope scope(isolate);
-  DCHECK_EQ(2, args.length());
-
-  bool observable = args[1]->IterationHasObservableEffects();
-
-  return isolate->heap()->ToBoolean(observable);
-}
-
 }  // namespace internal
 }  // namespace v8

src/builtins/builtins-definitions.h

@@ -243,7 +243,6 @@ namespace internal {
   /* Array */                                                                  \
   ASM(ArrayCode)                                                               \
   ASM(InternalArrayCode)                                                       \
-  CPP(HasIterationSideEffects)                                                 \
   CPP(ArrayConcat)                                                             \
   /* ES6 #sec-array.isarray */                                                 \
   TFJ(ArrayIsArray, 1, kArg)                                                   \

src/contexts.h

@@ -47,7 +47,6 @@ enum ContextLookupFlags {
   V(IS_ARRAYLIKE, JSFunction, is_arraylike)                                   \
   V(GENERATOR_NEXT_INTERNAL, JSFunction, generator_next_internal)             \
   V(GET_TEMPLATE_CALL_SITE_INDEX, JSFunction, get_template_call_site)         \
-  V(HAS_ITERATION_SIDE_EFFECTS_INDEX, JSFunction, has_iteration_side_effects) \
   V(MAKE_ERROR_INDEX, JSFunction, make_error)                                 \
   V(MAKE_RANGE_ERROR_INDEX, JSFunction, make_range_error)                     \
   V(MAKE_SYNTAX_ERROR_INDEX, JSFunction, make_syntax_error)                   \

src/elements-kind.h

@@ -150,6 +150,10 @@ inline bool IsFastSmiElementsKind(ElementsKind kind) {
       kind == FAST_HOLEY_SMI_ELEMENTS;
 }
 
+inline bool IsFastNumberElementsKind(ElementsKind kind) {
+  return IsFastSmiElementsKind(kind) || IsFastDoubleElementsKind(kind);
+}
+
 
 inline bool IsFastObjectElementsKind(ElementsKind kind) {
   return kind == FAST_ELEMENTS ||

src/js/typedarray.js

@@ -113,7 +113,12 @@ function TypedArraySpeciesCreate(exemplar, arg0, arg1, arg2, conservative) {
 
 macro TYPED_ARRAY_CONSTRUCTOR(NAME, ELEMENT_SIZE)
 function NAMEConstructByIterable(obj, iterable, iteratorFn) {
-  if (%has_iteration_side_effects(iterable)) {
+  if (%IterableToListCanBeElided(iterable)) {
+    // This .length access is unobservable, because it being observable would
+    // mean that iteration has side effects, and we wouldn't reach this path.
+    %typed_array_construct_by_array_like(
+        obj, iterable, iterable.length, ELEMENT_SIZE);
+  } else {
     var list = new InternalArray();
     // Reading the Symbol.iterator property of iterable twice would be
     // observable with getters, so instead, we call the function which
@@ -131,11 +136,6 @@ function NAMEConstructByIterable(obj, iterable, iteratorFn) {
       list.push(value);
     }
     %typed_array_construct_by_array_like(obj, list, list.length, ELEMENT_SIZE);
-  } else {
-    // This .length access is unobservable, because it being observable would
-    // mean that iteration has side effects, and we wouldn't reach this path.
-    %typed_array_construct_by_array_like(
-        obj, iterable, iterable.length, ELEMENT_SIZE);
   }
 }
 

src/runtime/runtime-object.cc

@@ -1028,6 +1028,22 @@ RUNTIME_FUNCTION(Runtime_CreateDataProperty) {
   return *value;
 }
 
+// Checks that 22.2.2.1.1 Runtime Semantics: IterableToList produces exactly the
+// same result as doing nothing.
+RUNTIME_FUNCTION(Runtime_IterableToListCanBeElided) {
+  HandleScope scope(isolate);
+  DCHECK_EQ(1, args.length());
+  CONVERT_ARG_HANDLE_CHECKED(JSReceiver, obj, 0);
+
+  if (!obj->IsJSObject()) return isolate->heap()->ToBoolean(false);
+
+  // While iteration alone may not have observable side-effects, calling
+  // toNumber on an object will. Make sure the arg is not an array of objects.
+  ElementsKind kind = JSObject::cast(*obj)->GetElementsKind();
+  if (!IsFastNumberElementsKind(kind)) return isolate->heap()->ToBoolean(false);
+
+  return isolate->heap()->ToBoolean(!obj->IterationHasObservableEffects());
+}
 
 }  // namespace internal
 }  // namespace v8

src/runtime/runtime.h

@@ -422,7 +422,8 @@ namespace internal {
   F(CreateIterResultObject, 2, 1)                               \
   F(CreateKeyValueArray, 2, 1)                                  \
   F(IsAccessCheckNeeded, 1, 1)                                  \
-  F(CreateDataProperty, 3, 1)
+  F(CreateDataProperty, 3, 1)                                   \
+  F(IterableToListCanBeElided, 1, 1)
 
 #define FOR_EACH_INTRINSIC_OPERATORS(F) \
   F(Multiply, 2, 1)                     \

test/mjsunit/es6/typedarray-construct-by-array-like.js

@@ -30,6 +30,27 @@ function TestConstructLargeObject(constr) {
   }
 }
 
+function TestConstructFromArrayWithSideEffects(constr) {
+  var arr = [{ valueOf() { arr[1] = 20; return 1; }}, 2];
+
+  var ta = new constr(arr);
+
+  assertEquals(1, ta[0]);
+  assertEquals(2, ta[1]);
+}
+
+function TestConstructFromArrayWithSideEffectsHoley(constr) {
+  var arr = [{ valueOf() { arr[1] = 20; return 1; }}, 2, , 4];
+
+  var ta = new constr(arr);
+
+  assertEquals(1, ta[0]);
+  assertEquals(2, ta[1]);
+  // ta[2] will be the default value, but we aren't testing that here.
+  assertEquals(4, ta[3]);
+}
+
+
 function TestConstructFromArray(constr) {
   var n = 64;
   var jsArray = [];
@@ -97,6 +118,8 @@ function TestOffsetIsUsed(constr, n) {
 
 Test(TestConstructSmallObject);
 Test(TestConstructLargeObject);
+Test(TestConstructFromArrayWithSideEffects);
+Test(TestConstructFromArrayWithSideEffectsHoley);
 Test(TestConstructFromArray);
 Test(TestConstructFromTypedArray);
 Test(TestLengthIsMaxSmi);