Ensure inlined Array.protoype.shift() calls return non-COW arrays

Also ensure that CSA's CloneFixedArray and ExtractFixedArray correctly
transition COW to non-COW maps when doing a clone requiring copying.

Bug: chromium:775888
Change-Id: I31c97072761fdd2360d86f840c9fd6ab2d72973a
Reviewed-on: https://chromium-review.googlesource.com/727900
Commit-Queue: Daniel Clifford <danno@chromium.org>
Reviewed-by: Igor Sheludko <ishell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#48754}

src/builtins/builtins-constructor-gen.cc

@@ -374,12 +374,8 @@ Node* ConstructorBuiltinsAssembler::EmitCreateShallowArrayLiteral(
       allocation_site_mode == TRACK_ALLOCATION_SITE ? allocation_site : nullptr;
 
   CSA_ASSERT(this, IsJSArrayMap(LoadMap(boilerplate)));
-  Node* boilerplate_elements = LoadElements(boilerplate);
   ParameterMode mode = OptimalParameterMode();
-  Node* capacity =
-      TaggedToParameter(LoadFixedArrayBaseLength(boilerplate_elements), mode);
-  return CloneFastJSArray(context, boilerplate, mode, capacity,
-                          allocation_site);
+  return CloneFastJSArray(context, boilerplate, mode, allocation_site);
 }
 
 TF_BUILTIN(CreateShallowArrayLiteral, ConstructorBuiltinsAssembler) {
@@ -505,8 +501,8 @@ Node* ConstructorBuiltinsAssembler::EmitCreateShallowObjectLiteral(
                          IsFixedCOWArrayMap(LoadMap(boilerplate_elements))));
     ExtractFixedArrayFlags flags;
     flags |= ExtractFixedArrayFlag::kAllFixedArrays;
-    flags |= ExtractFixedArrayFlag::kForceCOWCopy;
     flags |= ExtractFixedArrayFlag::kNewSpaceAllocationOnly;
+    flags |= ExtractFixedArrayFlag::kDontCopyCOW;
     var_elements.Bind(CloneFixedArray(boilerplate_elements, flags));
     Goto(&done);
     BIND(&done);

src/builtins/builtins-internal-gen.cc

@@ -32,10 +32,7 @@ TF_BUILTIN(CopyFastSmiOrObjectElements, CodeStubAssembler) {
 
   // Load the {object}s elements.
   Node* source = LoadObjectField(object, JSObject::kElementsOffset);
-  ExtractFixedArrayFlags flags;
-  flags |= ExtractFixedArrayFlag::kFixedArrays;
-  flags |= ExtractFixedArrayFlag::kForceCOWCopy;
-  Node* target = CloneFixedArray(source, flags);
+  Node* target = CloneFixedArray(source, ExtractFixedArrayFlag::kFixedArrays);
   StoreObjectField(object, JSObject::kElementsOffset, target);
   Return(target);
 }

src/builtins/builtins-promise-gen.cc

@@ -335,7 +335,6 @@ void PromiseBuiltinsAssembler::AppendPromiseCallback(int offset, Node* promise,
 
   ExtractFixedArrayFlags flags;
   flags |= ExtractFixedArrayFlag::kFixedArrays;
-  flags |= ExtractFixedArrayFlag::kForceCOWCopy;
   Node* new_elements =
       ExtractFixedArray(elements, nullptr, length, new_capacity, flags, mode);
 

src/builtins/builtins-regexp-gen.cc

@@ -1961,7 +1961,6 @@ class GrowableFixedArray {
 
     CodeStubAssembler::ExtractFixedArrayFlags flags;
     flags |= CodeStubAssembler::ExtractFixedArrayFlag::kFixedArrays;
-    flags |= CodeStubAssembler::ExtractFixedArrayFlag::kForceCOWCopy;
     Node* to_array = a->ExtractFixedArray(from_array, nullptr, element_count,
                                           new_capacity, flags);
 

src/code-stub-assembler.cc

@@ -2641,18 +2641,23 @@ Node* CodeStubAssembler::ExtractFastJSArray(Node* context, Node* array,
 }
 
 Node* CodeStubAssembler::CloneFastJSArray(Node* context, Node* array,
-                                          ParameterMode mode, Node* capacity,
+                                          ParameterMode mode,
                                           Node* allocation_site) {
-  // Use the cannonical map for the Array's ElementsKind
-  Node* tagged_length = LoadJSArrayLength(array);
+  Node* length = LoadJSArrayLength(array);
+  Node* elements = LoadElements(array);
 
-  Node* length = TaggedToParameter(tagged_length, mode);
-  if (capacity == nullptr) {
-    capacity = length;
-  }
+  Node* original_array_map = LoadMap(array);
+  Node* elements_kind = LoadMapElementsKind(original_array_map);
+
+  Node* new_elements = CloneFixedArray(elements);
 
-  return ExtractFastJSArray(context, array, IntPtrOrSmiConstant(0, mode),
-                            length, mode, capacity, allocation_site);
+  // Use the cannonical map for the Array's ElementsKind
+  Node* native_context = LoadNativeContext(context);
+  Node* array_map = LoadJSArrayElementsMap(elements_kind, native_context);
+  Node* result = AllocateUninitializedJSArrayWithoutElements(array_map, length,
+                                                             allocation_site);
+  StoreObjectField(result, JSObject::kElementsOffset, new_elements);
+  return result;
 }
 
 Node* CodeStubAssembler::AllocateFixedArray(ElementsKind kind,
@@ -2696,7 +2701,8 @@ Node* CodeStubAssembler::ExtractFixedArray(Node* fixed_array, Node* first,
                                            Node* count, Node* capacity,
                                            ExtractFixedArrayFlags extract_flags,
                                            ParameterMode parameter_mode) {
-  VARIABLE(result, MachineRepresentation::kTagged);
+  VARIABLE(var_result, MachineRepresentation::kTagged);
+  VARIABLE(var_fixed_array_map, MachineRepresentation::kTagged);
   const AllocationFlags flags =
       (extract_flags & ExtractFixedArrayFlag::kNewSpaceAllocationOnly)
           ? CodeStubAssembler::kNone
@@ -2723,30 +2729,29 @@ Node* CodeStubAssembler::ExtractFixedArray(Node* fixed_array, Node* first,
   }
 
   Label if_fixed_double_array(this), empty(this), cow(this),
-      done(this, {&result});
+      done(this, {&var_result, &var_fixed_array_map});
+  var_fixed_array_map.Bind(LoadMap(fixed_array));
   GotoIf(WordEqual(IntPtrOrSmiConstant(0, parameter_mode), count), &empty);
-  Node* fixed_array_map = LoadMap(fixed_array);
 
   if (extract_flags & ExtractFixedArrayFlag::kFixedDoubleArrays) {
     if (extract_flags & ExtractFixedArrayFlag::kFixedArrays) {
-      GotoIf(IsFixedDoubleArrayMap(fixed_array_map), &if_fixed_double_array);
+      GotoIf(IsFixedDoubleArrayMap(var_fixed_array_map.value()),
+             &if_fixed_double_array);
     } else {
-      CSA_ASSERT(this, IsFixedDoubleArrayMap(fixed_array_map));
+      CSA_ASSERT(this, IsFixedDoubleArrayMap(var_fixed_array_map.value()));
     }
   } else {
     DCHECK(extract_flags & ExtractFixedArrayFlag::kFixedArrays);
-    CSA_ASSERT(this, Word32BinaryNot(IsFixedDoubleArrayMap(fixed_array_map)));
+    CSA_ASSERT(this, Word32BinaryNot(
+                         IsFixedDoubleArrayMap(var_fixed_array_map.value())));
   }
 
   if (extract_flags & ExtractFixedArrayFlag::kFixedArrays) {
-    Label new_space_check(this);
-    if (!(extract_flags & ExtractFixedArrayFlag::kForceCOWCopy)) {
-      Branch(WordEqual(fixed_array_map,
-                       LoadRoot(Heap::kFixedCOWArrayMapRootIndex)),
-             &cow, &new_space_check);
-    } else {
-      Goto(&new_space_check);
-    }
+    Label new_space_check(this, {&var_fixed_array_map});
+    Branch(WordEqual(var_fixed_array_map.value(),
+                     LoadRoot(Heap::kFixedCOWArrayMapRootIndex)),
+           &cow, &new_space_check);
+
     BIND(&new_space_check);
 
     bool handle_old_space = true;
@@ -2772,9 +2777,10 @@ Node* CodeStubAssembler::ExtractFixedArray(Node* fixed_array, Node* first,
     Comment("Copy PACKED_ELEMENTS new space");
 
     ElementsKind kind = PACKED_ELEMENTS;
-    Node* to_elements = AllocateFixedArray(
-        kind, capacity, parameter_mode, AllocationFlag::kNone, fixed_array_map);
-    result.Bind(to_elements);
+    Node* to_elements =
+        AllocateFixedArray(kind, capacity, parameter_mode,
+                           AllocationFlag::kNone, var_fixed_array_map.value());
+    var_result.Bind(to_elements);
     CopyFixedArrayElements(kind, fixed_array, kind, to_elements, first, count,
                            capacity, SKIP_WRITE_BARRIER, parameter_mode);
     Goto(&done);
@@ -2785,8 +2791,8 @@ Node* CodeStubAssembler::ExtractFixedArray(Node* fixed_array, Node* first,
         Comment("Copy PACKED_ELEMENTS old space");
 
         to_elements = AllocateFixedArray(kind, capacity, parameter_mode, flags,
-                                         fixed_array_map);
-        result.Bind(to_elements);
+                                         var_fixed_array_map.value());
+        var_result.Bind(to_elements);
         CopyFixedArrayElements(kind, fixed_array, kind, to_elements, first,
                                count, capacity, UPDATE_WRITE_BARRIER,
                                parameter_mode);
@@ -2794,14 +2800,17 @@ Node* CodeStubAssembler::ExtractFixedArray(Node* fixed_array, Node* first,
       }
     }
 
-    if (!(extract_flags & ExtractFixedArrayFlag::kForceCOWCopy)) {
-      BIND(&cow);
-      {
+    BIND(&cow);
+    {
+      if (extract_flags & ExtractFixedArrayFlag::kDontCopyCOW) {
         GotoIf(WordNotEqual(IntPtrOrSmiConstant(0, parameter_mode), first),
                &new_space_check);
 
-        result.Bind(fixed_array);
+        var_result.Bind(fixed_array);
         Goto(&done);
+      } else {
+        var_fixed_array_map.Bind(LoadRoot(Heap::kFixedArrayMapRootIndex));
+        Goto(&new_space_check);
       }
     }
   } else {
@@ -2815,8 +2824,8 @@ Node* CodeStubAssembler::ExtractFixedArray(Node* fixed_array, Node* first,
 
     ElementsKind kind = PACKED_DOUBLE_ELEMENTS;
     Node* to_elements = AllocateFixedArray(kind, capacity, parameter_mode,
-                                           flags, fixed_array_map);
-    result.Bind(to_elements);
+                                           flags, var_fixed_array_map.value());
+    var_result.Bind(to_elements);
     CopyFixedArrayElements(kind, fixed_array, kind, to_elements, first, count,
                            capacity, SKIP_WRITE_BARRIER, parameter_mode);
 
@@ -2827,12 +2836,12 @@ Node* CodeStubAssembler::ExtractFixedArray(Node* fixed_array, Node* first,
   {
     Comment("Copy empty array");
 
-    result.Bind(EmptyFixedArrayConstant());
+    var_result.Bind(EmptyFixedArrayConstant());
     Goto(&done);
   }
 
   BIND(&done);
-  return result.value();
+  return var_result.value();
 }
 
 void CodeStubAssembler::InitializePropertyArrayLength(Node* property_array,

src/code-stub-assembler.h

@@ -730,7 +730,6 @@ class V8_EXPORT_PRIVATE CodeStubAssembler : public compiler::CodeAssembler {
 
   Node* CloneFastJSArray(Node* context, Node* array,
                          ParameterMode mode = INTPTR_PARAMETERS,
-                         Node* capacity = nullptr,
                          Node* allocation_site = nullptr);
 
   Node* ExtractFastJSArray(Node* context, Node* array, Node* begin, Node* count,
@@ -807,11 +806,10 @@ class V8_EXPORT_PRIVATE CodeStubAssembler : public compiler::CodeAssembler {
   enum class ExtractFixedArrayFlag {
     kFixedArrays = 1,
     kFixedDoubleArrays = 2,
-    // Forcing COW copying removes special COW handling, resulting in better
-    // code if the source array has already been validated to not be COW.
-    kForceCOWCopy = 4,
+    kDontCopyCOW = 4,
     kNewSpaceAllocationOnly = 8,
-    kAllFixedArrays = kFixedArrays | kFixedDoubleArrays
+    kAllFixedArrays = kFixedArrays | kFixedDoubleArrays,
+    kAllFixedArraysDontCopyCOW = kAllFixedArrays | kDontCopyCOW
   };
 
   typedef base::Flags<ExtractFixedArrayFlag> ExtractFixedArrayFlags;
@@ -855,9 +853,9 @@ class V8_EXPORT_PRIVATE CodeStubAssembler : public compiler::CodeAssembler {
   // kAllFixedArrays, the generated code is more compact and efficient if the
   // caller can specify whether only FixedArrays or FixedDoubleArrays will be
   // passed as the |source| parameter.
-  Node* CloneFixedArray(
-      Node* source,
-      ExtractFixedArrayFlags flags = ExtractFixedArrayFlag::kAllFixedArrays) {
+  Node* CloneFixedArray(Node* source,
+                        ExtractFixedArrayFlags flags =
+                            ExtractFixedArrayFlag::kAllFixedArraysDontCopyCOW) {
     ParameterMode mode = OptimalParameterMode();
     return ExtractFixedArray(source, IntPtrOrSmiConstant(0, mode), nullptr,
                              nullptr, flags, mode);

test/cctest/test-code-stub-assembler.cc

@@ -2911,7 +2911,7 @@ TEST(CloneFixedArrayCOW) {
   CHECK_EQ(*source, result);
 }
 
-TEST(CloneFixedArrayCOWForceCopy) {
+TEST(ExtractFixedArrayCOWForceCopy) {
   Isolate* isolate(CcTest::InitIsolateOnce());
   const int kNumParams = 1;
   CodeAssemblerTester asm_tester(isolate, kNumParams);
@@ -2919,8 +2919,9 @@ TEST(CloneFixedArrayCOWForceCopy) {
     CodeStubAssembler m(asm_tester.state());
     CodeStubAssembler::ExtractFixedArrayFlags flags;
     flags |= CodeStubAssembler::ExtractFixedArrayFlag::kAllFixedArrays;
-    flags |= CodeStubAssembler::ExtractFixedArrayFlag::kForceCOWCopy;
-    m.Return(m.CloneFixedArray(m.Parameter(0), flags));
+    m.Return(m.ExtractFixedArray(m.Parameter(0), m.SmiConstant(0), nullptr,
+                                 nullptr, flags,
+                                 CodeStubAssembler::SMI_PARAMETERS));
   }
   FunctionTester ft(asm_tester.GenerateCode(), kNumParams);
 
@@ -2944,10 +2945,12 @@ TEST(ExtractFixedArraySimple) {
   CodeAssemblerTester asm_tester(isolate, kNumParams);
   {
     CodeStubAssembler m(asm_tester.state());
-    m.Return(m.ExtractFixedArray(
-        m.Parameter(0), m.Parameter(1), m.Parameter(2), nullptr,
-        CodeStubAssembler::ExtractFixedArrayFlag::kAllFixedArrays,
-        CodeStubAssembler::SMI_PARAMETERS));
+    CodeStubAssembler::ExtractFixedArrayFlags flags;
+    flags |= CodeStubAssembler::ExtractFixedArrayFlag::kAllFixedArrays;
+    flags |= CodeStubAssembler::ExtractFixedArrayFlag::kDontCopyCOW;
+    m.Return(m.ExtractFixedArray(m.Parameter(0), m.Parameter(1), m.Parameter(2),
+                                 nullptr, flags,
+                                 CodeStubAssembler::SMI_PARAMETERS));
   }
   FunctionTester ft(asm_tester.GenerateCode(), kNumParams);
 
@@ -2969,10 +2972,12 @@ TEST(ExtractFixedArraySimpleSmiConstant) {
   CodeAssemblerTester asm_tester(isolate, kNumParams);
   {
     CodeStubAssembler m(asm_tester.state());
-    m.Return(m.ExtractFixedArray(
-        m.Parameter(0), m.SmiConstant(1), m.SmiConstant(2), nullptr,
-        CodeStubAssembler::ExtractFixedArrayFlag::kAllFixedArrays,
-        CodeStubAssembler::SMI_PARAMETERS));
+    CodeStubAssembler::ExtractFixedArrayFlags flags;
+    flags |= CodeStubAssembler::ExtractFixedArrayFlag::kAllFixedArrays;
+    flags |= CodeStubAssembler::ExtractFixedArrayFlag::kDontCopyCOW;
+    m.Return(m.ExtractFixedArray(m.Parameter(0), m.SmiConstant(1),
+                                 m.SmiConstant(2), nullptr, flags,
+                                 CodeStubAssembler::SMI_PARAMETERS));
   }
   FunctionTester ft(asm_tester.GenerateCode(), kNumParams);
 
@@ -2991,10 +2996,12 @@ TEST(ExtractFixedArraySimpleIntPtrConstant) {
   CodeAssemblerTester asm_tester(isolate, kNumParams);
   {
     CodeStubAssembler m(asm_tester.state());
-    m.Return(m.ExtractFixedArray(
-        m.Parameter(0), m.IntPtrConstant(1), m.IntPtrConstant(2), nullptr,
-        CodeStubAssembler::ExtractFixedArrayFlag::kAllFixedArrays,
-        CodeStubAssembler::INTPTR_PARAMETERS));
+    CodeStubAssembler::ExtractFixedArrayFlags flags;
+    flags |= CodeStubAssembler::ExtractFixedArrayFlag::kAllFixedArrays;
+    flags |= CodeStubAssembler::ExtractFixedArrayFlag::kDontCopyCOW;
+    m.Return(m.ExtractFixedArray(m.Parameter(0), m.IntPtrConstant(1),
+                                 m.IntPtrConstant(2), nullptr, flags,
+                                 CodeStubAssembler::INTPTR_PARAMETERS));
   }
   FunctionTester ft(asm_tester.GenerateCode(), kNumParams);
 

test/mjsunit/regress/regress-775888.js

+// Copyright 2017 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+// Flags: --allow-natives-syntax
+
+function __f_7586(__v_27535) {
+  let a = __v_27535.shift();
+  return a;
+}
+
+function __f_7587() {
+  var __v_27536 = [ 1, 15, 16];
+  __f_7586(__v_27536);
+  __v_27536.unshift(__v_27536);
+}
+__f_7587();
+__f_7587();
+
+%OptimizeFunctionOnNextCall(__f_7586);
+__f_7587();