[maglev] Deopt when trying to load from Typed Array with detached buffer

Drive-by: fix wrong bound check for TypedArrays / DataView on arm64,
which sometimes resulted in unecessary deopts.

Bug: v8:7700, chromium:1405651
Change-Id: I9afb2008edb22c0cd63044a6700a9f276960c191
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4146437
Commit-Queue: Darius Mercadier <dmercadier@chromium.org>
Reviewed-by: Victor Gomes <victorgomes@chromium.org>
Cr-Commit-Position: refs/heads/main@{#85157}

src/maglev/arm64/maglev-assembler-arm64-inl.h

@@ -6,6 +6,7 @@
 #define V8_MAGLEV_ARM64_MAGLEV_ASSEMBLER_ARM64_INL_H_
 
 #include "src/codegen/macro-assembler-inl.h"
+#include "src/compiler/compilation-dependencies.h"
 #include "src/maglev/maglev-assembler.h"
 #include "src/maglev/maglev-basic-block.h"
 #include "src/maglev/maglev-code-gen-state.h"
@@ -335,7 +336,7 @@ inline void MaglevAssembler::LoadBoundedSizeFromObject(Register result,
                                                        int offset) {
   Move(result, FieldMemOperand(object, offset));
 #ifdef V8_ENABLE_SANDBOX
-  Lsl(result, result, kBoundedSizeShift);
+  Lsr(result, result, kBoundedSizeShift);
 #endif  // V8_ENABLE_SANDBOX
 }
 
@@ -451,6 +452,25 @@ inline void MaglevAssembler::SignExtend32To64Bits(Register dst, Register src) {
   Mov(dst, Operand(src.W(), SXTW));
 }
 
+template <typename NodeT>
+inline void MaglevAssembler::DeoptIfBufferDetached(Register array,
+                                                   Register scratch,
+                                                   NodeT* node) {
+  if (!code_gen_state()
+           ->broker()
+           ->dependencies()
+           ->DependOnArrayBufferDetachingProtector()) {
+    // A detached buffer leads to megamorphic feedback, so we won't have a deopt
+    // loop if we deopt here.
+    LoadTaggedPointerField(
+        scratch, FieldMemOperand(array, JSArrayBufferView::kBufferOffset));
+    LoadTaggedPointerField(
+        scratch, FieldMemOperand(scratch, JSArrayBuffer::kBitFieldOffset));
+    Tst(scratch.W(), Immediate(JSArrayBuffer::WasDetachedBit::kMask));
+    EmitEagerDeoptIf(ne, DeoptimizeReason::kArrayBufferWasDetached, node);
+  }
+}
+
 inline void MaglevAssembler::CompareInt32(Register src1, Register src2) {
   Cmp(src1.W(), src2.W());
 }

src/maglev/arm64/maglev-ir-arm64.cc

@@ -1793,7 +1793,10 @@ void LoadSignedIntTypedArrayElement::GenerateCode(
   }
 
   UseScratchRegisterScope temps(masm);
-  Register data_pointer = temps.AcquireX();
+  Register scratch = temps.AcquireX();
+  __ DeoptIfBufferDetached(object, scratch, this);
+
+  Register data_pointer = scratch;
   int element_size = ElementsKindSize(elements_kind_);
   __ BuildTypedArrayDataPointer(data_pointer, object);
   __ Add(data_pointer, data_pointer, Operand(index, LSL, element_size / 2));
@@ -1820,7 +1823,10 @@ void LoadUnsignedIntTypedArrayElement::GenerateCode(
   }
 
   UseScratchRegisterScope temps(masm);
-  Register data_pointer = temps.AcquireX();
+  Register scratch = temps.AcquireX();
+  __ DeoptIfBufferDetached(object, scratch, this);
+
+  Register data_pointer = scratch;
   int element_size = ElementsKindSize(elements_kind_);
   __ BuildTypedArrayDataPointer(data_pointer, object);
   __ Add(data_pointer, data_pointer, Operand(index, LSL, element_size / 2));
@@ -1847,7 +1853,10 @@ void LoadDoubleTypedArrayElement::GenerateCode(MaglevAssembler* masm,
   }
 
   UseScratchRegisterScope temps(masm);
-  Register data_pointer = temps.AcquireX();
+  Register scratch = temps.AcquireX();
+  __ DeoptIfBufferDetached(object, scratch, this);
+
+  Register data_pointer = scratch;
   __ BuildTypedArrayDataPointer(data_pointer, object);
   switch (elements_kind_) {
     case FLOAT32_ELEMENTS:

src/maglev/maglev-assembler.h

@@ -161,6 +161,10 @@ class MaglevAssembler : public MacroAssembler {
 
   inline void SignExtend32To64Bits(Register dst, Register src);
 
+  template <typename NodeT>
+  inline void DeoptIfBufferDetached(Register array, Register scratch,
+                                    NodeT* node);
+
   inline void CompareInt32(Register src1, Register src2);
 
   inline void Jump(Label* target);

src/maglev/maglev-ir.h

@@ -4182,8 +4182,12 @@ class LoadSignedIntTypedArrayElement
            elements_kind == INT32_ELEMENTS);
   }
 
-  static constexpr OpProperties kProperties =
-      OpProperties::Reading() | OpProperties::Int32();
+  // TODO(dmercadier): We should have 2 version of this Node: one that never
+  // deopts because it's emitted behind a DependOnArrayBufferDetachingProtector
+  // check, and one that can deopt.
+  static constexpr OpProperties kProperties = OpProperties::EagerDeopt() |
+                                              OpProperties::Reading() |
+                                              OpProperties::Int32();
   static constexpr typename Base::InputTypes kInputTypes{
       ValueRepresentation::kTagged, ValueRepresentation::kUint32};
 
@@ -4215,8 +4219,12 @@ class LoadUnsignedIntTypedArrayElement
            elements_kind == UINT32_ELEMENTS);
   }
 
-  static constexpr OpProperties kProperties =
-      OpProperties::Reading() | OpProperties::Uint32();
+  // TODO(dmercadier): We should have 2 version of this Node: one that never
+  // deopts because it's emitted behind a DependOnArrayBufferDetachingProtector
+  // check, and one that can deopt.
+  static constexpr OpProperties kProperties = OpProperties::EagerDeopt() |
+                                              OpProperties::Reading() |
+                                              OpProperties::Uint32();
   static constexpr typename Base::InputTypes kInputTypes{
       ValueRepresentation::kTagged, ValueRepresentation::kUint32};
 
@@ -4245,8 +4253,12 @@ class LoadDoubleTypedArrayElement
            elements_kind == FLOAT64_ELEMENTS);
   }
 
-  static constexpr OpProperties kProperties =
-      OpProperties::Reading() | OpProperties::Float64();
+  // TODO(dmercadier): We should have 2 version of this Node: one that never
+  // deopts because it's emitted behind a DependOnArrayBufferDetachingProtector
+  // check, and one that can deopt.
+  static constexpr OpProperties kProperties = OpProperties::EagerDeopt() |
+                                              OpProperties::Reading() |
+                                              OpProperties::Float64();
   static constexpr typename Base::InputTypes kInputTypes{
       ValueRepresentation::kTagged, ValueRepresentation::kUint32};
 

src/maglev/x64/maglev-assembler-x64-inl.h

@@ -11,6 +11,7 @@
 
 #include "src/codegen/interface-descriptors-inl.h"
 #include "src/codegen/macro-assembler-inl.h"
+#include "src/compiler/compilation-dependencies.h"
 #include "src/maglev/maglev-assembler.h"
 #include "src/maglev/maglev-basic-block.h"
 #include "src/maglev/maglev-code-gen-state.h"
@@ -339,6 +340,25 @@ inline void MaglevAssembler::SignExtend32To64Bits(Register dst, Register src) {
   movsxlq(dst, src);
 }
 
+template <typename NodeT>
+inline void MaglevAssembler::DeoptIfBufferDetached(Register array,
+                                                   Register scratch,
+                                                   NodeT* node) {
+  if (!code_gen_state()
+           ->broker()
+           ->dependencies()
+           ->DependOnArrayBufferDetachingProtector()) {
+    // A detached buffer leads to megamorphic feedback, so we won't have a deopt
+    // loop if we deopt here.
+    LoadTaggedPointerField(
+        scratch, FieldOperand(array, JSArrayBufferView::kBufferOffset));
+    LoadTaggedPointerField(
+        scratch, FieldOperand(scratch, JSArrayBuffer::kBitFieldOffset));
+    testl(scratch, Immediate(JSArrayBuffer::WasDetachedBit::kMask));
+    EmitEagerDeoptIf(not_zero, DeoptimizeReason::kArrayBufferWasDetached, node);
+  }
+}
+
 inline void MaglevAssembler::CompareInt32(Register src1, Register src2) {
   cmpl(src1, src2);
 }

src/maglev/x64/maglev-ir-x64.cc

@@ -1078,6 +1078,7 @@ void LoadSignedIntTypedArrayElement::GenerateCode(
   Register object = ToRegister(object_input());
   Register index = ToRegister(index_input());
   Register result_reg = ToRegister(result());
+  Register scratch = general_temporaries().PopFirst();
 
   __ AssertNotSmi(object);
   if (v8_flags.debug_code) {
@@ -1085,7 +1086,9 @@ void LoadSignedIntTypedArrayElement::GenerateCode(
     __ Assert(equal, AbortReason::kUnexpectedValue);
   }
 
-  Register data_pointer = general_temporaries().PopFirst();
+  __ DeoptIfBufferDetached(object, scratch, this);
+
+  Register data_pointer = scratch;
   __ BuildTypedArrayDataPointer(data_pointer, object);
   int element_size = ElementsKindSize(elements_kind_);
   __ LoadSignedField(
@@ -1105,6 +1108,7 @@ void LoadUnsignedIntTypedArrayElement::GenerateCode(
   Register object = ToRegister(object_input());
   Register index = ToRegister(index_input());
   Register result_reg = ToRegister(result());
+  Register scratch = general_temporaries().PopFirst();
 
   __ AssertNotSmi(object);
   if (v8_flags.debug_code) {
@@ -1112,7 +1116,9 @@ void LoadUnsignedIntTypedArrayElement::GenerateCode(
     __ Assert(equal, AbortReason::kUnexpectedValue);
   }
 
-  Register data_pointer = general_temporaries().PopFirst();
+  __ DeoptIfBufferDetached(object, scratch, this);
+
+  Register data_pointer = scratch;
   int element_size = ElementsKindSize(elements_kind_);
   __ BuildTypedArrayDataPointer(data_pointer, object);
   __ LoadUnsignedField(
@@ -1132,13 +1138,17 @@ void LoadDoubleTypedArrayElement::GenerateCode(MaglevAssembler* masm,
   Register object = ToRegister(object_input());
   Register index = ToRegister(index_input());
   DoubleRegister result_reg = ToDoubleRegister(result());
+  Register scratch = general_temporaries().PopFirst();
+
   __ AssertNotSmi(object);
   if (v8_flags.debug_code) {
     __ CmpObjectType(object, JS_TYPED_ARRAY_TYPE, kScratchRegister);
     __ Assert(equal, AbortReason::kUnexpectedValue);
   }
 
-  Register data_pointer = general_temporaries().PopFirst();
+  __ DeoptIfBufferDetached(object, scratch, this);
+
+  Register data_pointer = scratch;
   __ BuildTypedArrayDataPointer(data_pointer, object);
   switch (elements_kind_) {
     case FLOAT32_ELEMENTS:

test/mjsunit/maglev/regress/regress-1405651.js

@@ -0,0 +1,55 @@
+// Copyright 2023 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 --harmony --no-always-turbofan --maglev
+
+// TODO(dmercadier): re-enable this test once on-heap Typed Arrays are
+// re-supported by Maglev.
+/*
+function assertMaglevved(f) {
+  assertTrue(isMaglevved(f));
+}
+
+function f(x) {
+  return x[2];
+}
+
+let buff = new ArrayBuffer(1024);
+let arr = new Int32Array(buff);
+arr[2] = 42;
+
+%PrepareFunctionForOptimization(f);
+assertEquals(42, f(arr));
+
+%OptimizeMaglevOnNextCall(f);
+assertEquals(42, f(arr));
+
+assertMaglevved(f);
+// Detaching {buff} will cause {f} to deoptimize thanks to the protector.
+buff.transfer();
+assertUnoptimized(f);
+
+assertEquals(undefined, f(arr));
+
+let buff2 = new ArrayBuffer(1024);
+let arr2 = new Int32Array(buff2);
+arr2[2] = 42;
+
+// Re-optimizing {f} (which a fresh feedback), now that the protector for
+// detached array buffer doesn't hold anymore.
+%ClearFunctionFeedback(f);
+%PrepareFunctionForOptimization(f);
+assertEquals(42, f(arr2));
+%OptimizeMaglevOnNextCall(f);
+assertEquals(42, f(arr2));
+assertMaglevved(f);
+
+// The protector doesn't hold anymore, so detaching buff2 shouldn't deopt {f}.
+buff2.transfer();
+assertMaglevved(f);
+
+// The runtime check will call a deopt since {buff2} has been detached.
+assertEquals(undefined, f(arr2));
+assertUnoptimized(f);
+*/