[ic] Pass the converted value to the runtime when storing to a typed array

Preparing the value for storing into a typed array is user visible
operation in some cases (for ex: calling ToNumber). To avoid doing this
conversion twice pass the converted to the runtime when bailing out
from the handlers.

Bug: chromium:981236
Change-Id: I3de23d317d22cd6c201fe8a4db30014f4cf76251
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1692932
Commit-Queue: Mythri Alle <mythria@chromium.org>
Reviewed-by: Jakob Gruber <jgruber@chromium.org>
Reviewed-by: Leszek Swirski <leszeks@chromium.org>
Cr-Commit-Position: refs/heads/master@{#62879}

src/builtins/builtins-handler-gen.cc

@@ -28,7 +28,8 @@ class HandlerBuiltinsAssembler : public CodeStubAssembler {
   // kind. Use with caution. This produces a *lot* of code.
   using ElementsKindSwitchCase = std::function<void(ElementsKind)>;
   void DispatchByElementsKind(TNode<Int32T> elements_kind,
-                              const ElementsKindSwitchCase& case_function);
+                              const ElementsKindSwitchCase& case_function,
+                              bool handle_typed_elements_kind);
 
   // Dispatches over all possible combinations of {from,to} elements kinds.
   using ElementsKindTransitionSwitchCase =
@@ -227,7 +228,7 @@ void HandlerBuiltinsAssembler::Generate_ElementsTransitionAndStore(
         [=, &miss](ElementsKind from_kind, ElementsKind to_kind) {
           TransitionElementsKind(receiver, map, from_kind, to_kind, &miss);
           EmitElementStore(receiver, key, value, to_kind, store_mode, &miss,
-                           context);
+                           context, nullptr);
         });
     Return(value);
   }
@@ -280,7 +281,8 @@ TF_BUILTIN(ElementsTransitionAndStore_NoTransitionHandleCOW,
   V(BIGINT64_ELEMENTS)
 
 void HandlerBuiltinsAssembler::DispatchByElementsKind(
-    TNode<Int32T> elements_kind, const ElementsKindSwitchCase& case_function) {
+    TNode<Int32T> elements_kind, const ElementsKindSwitchCase& case_function,
+    bool handle_typed_elements_kind) {
   Label next(this), if_unknown_type(this, Label::kDeferred);
 
   int32_t elements_kinds[] = {
@@ -300,6 +302,8 @@ void HandlerBuiltinsAssembler::DispatchByElementsKind(
   };
   STATIC_ASSERT(arraysize(elements_kinds) == arraysize(elements_kind_labels));
 
+  // TODO(mythria): Do not emit cases for typed elements kind when
+  // handle_typed_elements is false to decrease the size of the jump table.
   Switch(elements_kind, &if_unknown_type, elements_kinds, elements_kind_labels,
          arraysize(elements_kinds));
 
@@ -310,6 +314,9 @@ void HandlerBuiltinsAssembler::DispatchByElementsKind(
         IsFrozenOrSealedElementsKindUnchecked(KIND)) {           \
       /* Disable support for frozen or sealed elements kinds. */ \
       Unreachable();                                             \
+    } else if (!handle_typed_elements_kind &&                    \
+               IsTypedArrayElementsKind(KIND)) {                 \
+      Unreachable();                                             \
     } else {                                                     \
       case_function(KIND);                                       \
       Goto(&next);                                               \
@@ -340,17 +347,26 @@ void HandlerBuiltinsAssembler::Generate_StoreFastElementIC(
 
   Label miss(this);
 
+  bool handle_typed_elements_kind =
+      store_mode == STANDARD_STORE || store_mode == STORE_IGNORE_OUT_OF_BOUNDS;
+  // For typed arrays maybe_converted_value contains the value obtained after
+  // calling ToNumber. We should pass the converted value to the runtime to
+  // avoid doing the user visible conversion again.
+  VARIABLE(maybe_converted_value, MachineRepresentation::kTagged, value);
+  maybe_converted_value.Bind(value);
   // TODO(v8:8481): Pass elements_kind in feedback vector slots.
-  DispatchByElementsKind(LoadElementsKind(receiver),
-                         [=, &miss](ElementsKind elements_kind) {
-                           EmitElementStore(receiver, key, value, elements_kind,
-                                            store_mode, &miss, context);
-                         });
+  DispatchByElementsKind(
+      LoadElementsKind(receiver),
+      [=, &miss, &maybe_converted_value](ElementsKind elements_kind) {
+        EmitElementStore(receiver, key, value, elements_kind, store_mode, &miss,
+                         context, &maybe_converted_value);
+      },
+      handle_typed_elements_kind);
   Return(value);
 
   BIND(&miss);
-  TailCallRuntime(Runtime::kKeyedStoreIC_Miss, context, value, slot, vector,
-                  receiver, key);
+  TailCallRuntime(Runtime::kKeyedStoreIC_Miss, context,
+                  maybe_converted_value.value(), slot, vector, receiver, key);
 }
 
 TF_BUILTIN(StoreFastElementIC_Standard, HandlerBuiltinsAssembler) {

src/codegen/code-stub-assembler.cc

@@ -10614,7 +10614,8 @@ void CodeStubAssembler::BigIntToRawBytes(TNode<BigInt> bigint,
 void CodeStubAssembler::EmitElementStore(Node* object, Node* key, Node* value,
                                          ElementsKind elements_kind,
                                          KeyedAccessStoreMode store_mode,
-                                         Label* bailout, Node* context) {
+                                         Label* bailout, Node* context,
+                                         Variable* maybe_converted_value) {
   CSA_ASSERT(this, Word32BinaryNot(IsJSProxy(object)));
 
   Node* elements = LoadElements(object);
@@ -10630,12 +10631,12 @@ void CodeStubAssembler::EmitElementStore(Node* object, Node* key, Node* value,
   TNode<IntPtrT> intptr_key = TryToIntptr(key, bailout);
 
   if (IsTypedArrayElementsKind(elements_kind)) {
-    Label done(this);
+    Label done(this), update_value_and_bailout(this, Label::kDeferred);
 
     // IntegerIndexedElementSet converts value to a Number/BigInt prior to the
     // bounds check.
-    value = PrepareValueForWriteToTypedArray(CAST(value), elements_kind,
-                                             CAST(context));
+    Node* converted_value = PrepareValueForWriteToTypedArray(
+        CAST(value), elements_kind, CAST(context));
 
     // There must be no allocations between the buffer load and
     // and the actual store to backing store, because GC may decide that
@@ -10644,7 +10645,11 @@ void CodeStubAssembler::EmitElementStore(Node* object, Node* key, Node* value,
 
     // Check if buffer has been detached.
     TNode<JSArrayBuffer> buffer = LoadJSArrayBufferViewBuffer(CAST(object));
-    GotoIf(IsDetachedBuffer(buffer), bailout);
+    if (maybe_converted_value) {
+      GotoIf(IsDetachedBuffer(buffer), &update_value_and_bailout);
+    } else {
+      GotoIf(IsDetachedBuffer(buffer), bailout);
+    }
 
     // Bounds check.
     TNode<UintPtrT> length = LoadJSTypedArrayLength(CAST(object));
@@ -10653,21 +10658,83 @@ void CodeStubAssembler::EmitElementStore(Node* object, Node* key, Node* value,
       // Skip the store if we write beyond the length or
       // to a property with a negative integer index.
       GotoIfNot(UintPtrLessThan(intptr_key, length), &done);
-    } else if (store_mode == STANDARD_STORE) {
-      GotoIfNot(UintPtrLessThan(intptr_key, length), bailout);
     } else {
-      // This case is produced due to the dispatched call in
-      // ElementsTransitionAndStore and StoreFastElement.
-      // TODO(jgruber): Avoid generating unsupported combinations to save code
-      // size.
-      DebugBreak();
+      DCHECK_EQ(store_mode, STANDARD_STORE);
+      GotoIfNot(UintPtrLessThan(intptr_key, length), &update_value_and_bailout);
     }
 
     TNode<RawPtrT> backing_store = LoadJSTypedArrayBackingStore(CAST(object));
-    StoreElement(backing_store, elements_kind, intptr_key, value,
+    StoreElement(backing_store, elements_kind, intptr_key, converted_value,
                  parameter_mode);
     Goto(&done);
 
+    BIND(&update_value_and_bailout);
+    // We already prepared the incoming value for storing into a typed array.
+    // This might involve calling ToNumber in some cases. We shouldn't call
+    // ToNumber again in the runtime so pass the converted value to the runtime.
+    // The prepared value is an untagged value. Convert it to a tagged value
+    // to pass it to runtime. It is not possible to do the detached buffer check
+    // before we prepare the value, since ToNumber can detach the ArrayBuffer.
+    // The spec specifies the order of these operations.
+    if (maybe_converted_value != nullptr) {
+      switch (elements_kind) {
+        case UINT8_ELEMENTS:
+        case INT8_ELEMENTS:
+        case UINT16_ELEMENTS:
+        case INT16_ELEMENTS:
+        case UINT8_CLAMPED_ELEMENTS:
+          maybe_converted_value->Bind(SmiFromInt32(converted_value));
+          break;
+        case UINT32_ELEMENTS:
+          maybe_converted_value->Bind(ChangeUint32ToTagged(converted_value));
+          break;
+        case INT32_ELEMENTS:
+          maybe_converted_value->Bind(ChangeInt32ToTagged(converted_value));
+          break;
+        case FLOAT32_ELEMENTS: {
+          Label dont_allocate_heap_number(this), end(this);
+          GotoIf(TaggedIsSmi(value), &dont_allocate_heap_number);
+          GotoIf(IsHeapNumber(value), &dont_allocate_heap_number);
+          {
+            maybe_converted_value->Bind(AllocateHeapNumberWithValue(
+                ChangeFloat32ToFloat64(converted_value)));
+            Goto(&end);
+          }
+          BIND(&dont_allocate_heap_number);
+          {
+            maybe_converted_value->Bind(value);
+            Goto(&end);
+          }
+          BIND(&end);
+          break;
+        }
+        case FLOAT64_ELEMENTS: {
+          Label dont_allocate_heap_number(this), end(this);
+          GotoIf(TaggedIsSmi(value), &dont_allocate_heap_number);
+          GotoIf(IsHeapNumber(value), &dont_allocate_heap_number);
+          {
+            maybe_converted_value->Bind(
+                AllocateHeapNumberWithValue(converted_value));
+            Goto(&end);
+          }
+          BIND(&dont_allocate_heap_number);
+          {
+            maybe_converted_value->Bind(value);
+            Goto(&end);
+          }
+          BIND(&end);
+          break;
+        }
+        case BIGINT64_ELEMENTS:
+        case BIGUINT64_ELEMENTS:
+          maybe_converted_value->Bind(converted_value);
+          break;
+        default:
+          UNREACHABLE();
+      }
+    }
+    Goto(bailout);
+
     BIND(&done);
     return;
   }

src/codegen/code-stub-assembler.h

@@ -3070,7 +3070,8 @@ class V8_EXPORT_PRIVATE CodeStubAssembler
   void EmitElementStore(Node* object, Node* key, Node* value,
                         ElementsKind elements_kind,
                         KeyedAccessStoreMode store_mode, Label* bailout,
-                        Node* context);
+                        Node* context,
+                        Variable* maybe_converted_value = nullptr);
 
   Node* CheckForCapacityGrow(Node* object, Node* elements, ElementsKind kind,
                              Node* length, Node* key, ParameterMode mode,

test/mjsunit/regress/regress-981236.js

+// Copyright 2019 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.
+
+var count = 0;
+function keyedSta(a) {
+  a[0] = {
+    valueOf() {
+      count += 1;
+      return 42n;
+    }
+  };
+};
+
+array1  = keyedSta(new BigInt64Array(1));
+var r = keyedSta(new BigInt64Array());
+assertEquals(count, 2);