From d7d5049cfa995244b7d7bfb3641a7e63ed6e0a21 Mon Sep 17 00:00:00 2001 From: Victor Gomes Date: Fri, 13 Jan 2023 14:31:11 +0100 Subject: [PATCH] [maglev] Create DataViewGetVariableLength builtin kDataViewGetVariableLength has JS linkage, and so it has a strong requirement to what should be in the stack and in the registers (including having a JSFunction for kDataViewGetVariableLength). These were missing before, which would crash when checking the frame. Fixed: chromium:1406727 Bug: v8:7700 Change-Id: Iad878cbc06d46403e21162dfdfd3bcd1a2a063d3 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4162926 Reviewed-by: Leszek Swirski Commit-Queue: Victor Gomes Cr-Commit-Position: refs/heads/main@{#85284} --- BUILD.bazel | 1 + BUILD.gn | 1 + src/builtins/builtins-data-view-gen.cc | 30 ++++++++++++++++++++++++++ src/builtins/builtins-definitions.h | 1 + src/codegen/interface-descriptors.h | 11 ++++++++++ src/maglev/arm64/maglev-ir-arm64.cc | 19 ++++++++++------ src/maglev/x64/maglev-ir-x64.cc | 17 +++++++++------ 7 files changed, 67 insertions(+), 13 deletions(-) create mode 100644 src/builtins/builtins-data-view-gen.cc diff --git a/BUILD.bazel b/BUILD.bazel index 6e9b7fe3dc..c3f37247b8 100644 --- a/BUILD.bazel +++ b/BUILD.bazel @@ -3017,6 +3017,7 @@ filegroup( "src/builtins/builtins-constructor.h", "src/builtins/builtins-conversion-gen.cc", "src/builtins/builtins-data-view-gen.h", + "src/builtins/builtins-data-view-gen.cc", "src/builtins/builtins-date-gen.cc", "src/builtins/builtins-generator-gen.cc", "src/builtins/builtins-global-gen.cc", diff --git a/BUILD.gn b/BUILD.gn index 5e7db43f76..5e24bc8da7 100644 --- a/BUILD.gn +++ b/BUILD.gn @@ -2461,6 +2461,7 @@ v8_source_set("v8_initializers") { "src/builtins/builtins-constructor-gen.h", "src/builtins/builtins-constructor.h", "src/builtins/builtins-conversion-gen.cc", + "src/builtins/builtins-data-view-gen.cc", "src/builtins/builtins-data-view-gen.h", "src/builtins/builtins-date-gen.cc", "src/builtins/builtins-generator-gen.cc", diff --git a/src/builtins/builtins-data-view-gen.cc b/src/builtins/builtins-data-view-gen.cc new file mode 100644 index 0000000000..fc85a8783c --- /dev/null +++ b/src/builtins/builtins-data-view-gen.cc @@ -0,0 +1,30 @@ +// 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. + +#include "src/builtins/builtins-data-view-gen.h" + +#include "src/builtins/builtins-utils-gen.h" + +namespace v8 { +namespace internal { + +// Returns (intptr) a byte length value from [0..JSArrayBuffer::kMaxLength] +// If it fails (due to detached or OOB), it returns -1. +TF_BUILTIN(DataViewGetVariableLength, DataViewBuiltinsAssembler) { + auto dataview = Parameter(Descriptor::kDataView); + CSA_CHECK(this, IsVariableLengthJSArrayBufferView(dataview)); + + Label detached_or_oob(this); + auto buffer = + LoadObjectField(dataview, JSDataView::kBufferOffset); + TNode byte_length = LoadVariableLengthJSArrayBufferViewByteLength( + dataview, buffer, &detached_or_oob); + Return(byte_length); + + BIND(&detached_or_oob); + Return(IntPtrConstant(-1)); +} + +} // namespace internal +} // namespace v8 diff --git a/src/builtins/builtins-definitions.h b/src/builtins/builtins-definitions.h index 3107fb21e3..8c432526f4 100644 --- a/src/builtins/builtins-definitions.h +++ b/src/builtins/builtins-definitions.h @@ -502,6 +502,7 @@ namespace internal { /* DataView */ \ /* ES #sec-dataview-constructor */ \ CPP(DataViewConstructor) \ + TFC(DataViewGetVariableLength, DataViewGetVariableLength) \ \ /* Date */ \ /* ES #sec-date-constructor */ \ diff --git a/src/codegen/interface-descriptors.h b/src/codegen/interface-descriptors.h index e1fea903a0..2bf80f89fe 100644 --- a/src/codegen/interface-descriptors.h +++ b/src/codegen/interface-descriptors.h @@ -72,6 +72,7 @@ namespace internal { V(CopyDataPropertiesWithExcludedProperties) \ V(CopyDataPropertiesWithExcludedPropertiesOnStack) \ V(CppBuiltinAdaptor) \ + V(DataViewGetVariableLength) \ V(FastNewObject) \ V(FindNonDefaultConstructorOrConstruct) \ V(ForInPrepare) \ @@ -1639,6 +1640,16 @@ class CppBuiltinAdaptorDescriptor DECLARE_JS_COMPATIBLE_DESCRIPTOR(CppBuiltinAdaptorDescriptor) }; +class DataViewGetVariableLengthDescriptor + : public StaticCallInterfaceDescriptor< + DataViewGetVariableLengthDescriptor> { + public: + DEFINE_PARAMETERS(kDataView) + DEFINE_RESULT_AND_PARAMETER_TYPES(MachineType::IntPtr(), // Byte Length + MachineType::AnyTagged()) // kDataView + DECLARE_DESCRIPTOR(DataViewGetVariableLengthDescriptor) +}; + class CEntry1ArgvOnStackDescriptor : public StaticCallInterfaceDescriptor { public: diff --git a/src/maglev/arm64/maglev-ir-arm64.cc b/src/maglev/arm64/maglev-ir-arm64.cc index 916be99fab..abc53cc6e9 100644 --- a/src/maglev/arm64/maglev-ir-arm64.cc +++ b/src/maglev/arm64/maglev-ir-arm64.cc @@ -1433,17 +1433,22 @@ void CheckJSDataViewBounds::GenerateCode(MaglevAssembler* masm, [](MaglevAssembler* masm, CheckJSDataViewBounds* node, ZoneLabelRef done, Register object, Register index, Register byte_length) { RegisterSnapshot snapshot = node->register_snapshot(); + AddDeoptRegistersToSnapshot(&snapshot, node->eager_deopt_info()); snapshot.live_registers.set(index); // Make sure index is saved. + DCHECK(!snapshot.live_registers.has(byte_length)); { - // TODO(v8:7700): Inline DataViewPrototypeGetByteLength or create a - // different builtin that does not re-check the DataView object. + using D = CallInterfaceDescriptorFor< + Builtin::kDataViewGetVariableLength>::type; SaveRegisterStateForCall save_register_state(masm, snapshot); - __ PushReverse(object); - __ Mov(kContextRegister, masm->native_context().object()); - __ Mov(kJavaScriptCallArgCountRegister, 1); - __ CallBuiltin(Builtin::kDataViewPrototypeGetByteLength); + __ Move(D::GetRegisterParameter(D::kDataView), object); + __ Move(kContextRegister, masm->native_context().object()); + __ CallBuiltin(Builtin::kDataViewGetVariableLength); + __ Move(byte_length, kReturnRegister0); } - __ SmiUntag(byte_length, kReturnRegister0); + __ Cmp(byte_length, Immediate(0)); + // The reason might not be OOB, but because array was detached. + // Unfortunately we can only add one reason type in Maglev. + __ EmitEagerDeoptIf(lt, DeoptimizeReason::kOutOfBounds, node); __ B(*done); }, this, done_byte_length, object, index, byte_length); diff --git a/src/maglev/x64/maglev-ir-x64.cc b/src/maglev/x64/maglev-ir-x64.cc index a4e616d9c6..8b2712a2ed 100644 --- a/src/maglev/x64/maglev-ir-x64.cc +++ b/src/maglev/x64/maglev-ir-x64.cc @@ -531,17 +531,22 @@ void CheckJSDataViewBounds::GenerateCode(MaglevAssembler* masm, [](MaglevAssembler* masm, CheckJSDataViewBounds* node, ZoneLabelRef done, Register object, Register index, Register byte_length) { RegisterSnapshot snapshot = node->register_snapshot(); + AddDeoptRegistersToSnapshot(&snapshot, node->eager_deopt_info()); snapshot.live_registers.set(index); // Make sure index is saved. + DCHECK(!snapshot.live_registers.has(byte_length)); { - // TODO(v8:7700): Inline DataViewPrototypeGetByteLength or create a - // different builtin that does not re-check the DataView object. + using D = CallInterfaceDescriptorFor< + Builtin::kDataViewGetVariableLength>::type; SaveRegisterStateForCall save_register_state(masm, snapshot); - __ PushReverse(object); + __ Move(D::GetRegisterParameter(D::kDataView), object); __ Move(kContextRegister, masm->native_context().object()); - __ Move(kJavaScriptCallArgCountRegister, 1); - __ CallBuiltin(Builtin::kDataViewPrototypeGetByteLength); + __ CallBuiltin(Builtin::kDataViewGetVariableLength); + __ Move(byte_length, kReturnRegister0); } - __ SmiUntag(byte_length, kReturnRegister0); + __ cmpq(byte_length, Immediate(0)); + // The reason might not be OOB, but because array was detached. + // Unfortunately we can only add one reason type in Maglev. + __ EmitEagerDeoptIf(less, DeoptimizeReason::kOutOfBounds, node); __ jmp(*done); }, this, done_byte_length, object, index, byte_length);