From f2abe7bf761fbae48d4e7eba5ecf985ab6b35390 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Thu, 20 Dec 2018 14:34:20 +0100 Subject: [PATCH] deps: V8: backport 3e010af Original commit message: [CloneObjectIC] clone MutableHeapNumbers only if !FLAG_unbox_double_fields Change the macros added in bf84766a2cd3e09070adcd6228a3a487c8dc4bbd to only do the hard work if FLAG_unbox_double_fields is unset (otherwise, they will attempt to dereference raw float64s, which is bad!) Also adds a write barrier in CopyPropertyArrayValues for each store if it's possible that a MutableHeapNumber is cloned. BUG=chromium:901301, chromium:902965, chromium:903070, v8:7611 R=cbruni@chromium.org, jkummerow@chromium.org, ishell@chromium.org Change-Id: I224d3c4e7b0a887684bff68985b4d97021ba4cfb Reviewed-on: https://chromium-review.googlesource.com/c/1323911 Commit-Queue: Caitlin Potter Reviewed-by: Camillo Bruni Reviewed-by: Igor Sheludko Cr-Commit-Position: refs/heads/master@{#57368} PR-URL: https://github.com/nodejs/node/pull/25101 Refs: https://github.com/v8/v8/commit/3e010af274088493f3485d7a16dec4e31550e876 Fixes: https://github.com/nodejs/node/issues/25089 Reviewed-By: Richard Lau Reviewed-By: Gus Caplan Reviewed-By: James M Snell Reviewed-By: Ali Ijaz Sheikh Reviewed-By: Yang Guo --- common.gypi | 2 +- .../src/builtins/builtins-constructor-gen.cc | 3 +- deps/v8/src/code-stub-assembler.cc | 7 +++ deps/v8/src/ic/accessor-assembler.cc | 60 +++++++++++++------ .../mjsunit/es9/regress/regress-902965.js | 12 ++++ .../mjsunit/es9/regress/regress-903070.js | 15 +++++ 6 files changed, 77 insertions(+), 22 deletions(-) create mode 100644 deps/v8/test/mjsunit/es9/regress/regress-902965.js create mode 100644 deps/v8/test/mjsunit/es9/regress/regress-903070.js diff --git a/common.gypi b/common.gypi index 8e423e4cdfaca5..57a7b72c91b2e4 100644 --- a/common.gypi +++ b/common.gypi @@ -30,7 +30,7 @@ # Reset this number to 0 on major V8 upgrades. # Increment by one for each non-official patch applied to deps/v8. - 'v8_embedder_string': '-node.14', + 'v8_embedder_string': '-node.15', # Enable disassembler for `--print-code` v8 options 'v8_enable_disassembler': 1, diff --git a/deps/v8/src/builtins/builtins-constructor-gen.cc b/deps/v8/src/builtins/builtins-constructor-gen.cc index 8e54c4c36961a7..d70330c42528f5 100644 --- a/deps/v8/src/builtins/builtins-constructor-gen.cc +++ b/deps/v8/src/builtins/builtins-constructor-gen.cc @@ -525,8 +525,7 @@ Node* ConstructorBuiltinsAssembler::EmitCreateShallowObjectLiteral( VARIABLE(offset, MachineType::PointerRepresentation(), IntPtrConstant(JSObject::kHeaderSize)); // Mutable heap numbers only occur on 32-bit platforms. - bool may_use_mutable_heap_numbers = - FLAG_track_double_fields && !FLAG_unbox_double_fields; + bool may_use_mutable_heap_numbers = !FLAG_unbox_double_fields; { Comment("Copy in-object properties fast"); Label continue_fast(this, &offset); diff --git a/deps/v8/src/code-stub-assembler.cc b/deps/v8/src/code-stub-assembler.cc index df3a8e06cad9d2..a3c85df0ebd846 100644 --- a/deps/v8/src/code-stub-assembler.cc +++ b/deps/v8/src/code-stub-assembler.cc @@ -4432,6 +4432,13 @@ void CodeStubAssembler::CopyPropertyArrayValues(Node* from_array, Comment("[ CopyPropertyArrayValues"); bool needs_write_barrier = barrier_mode == UPDATE_WRITE_BARRIER; + + if (destroy_source == DestroySource::kNo) { + // PropertyArray may contain MutableHeapNumbers, which will be cloned on the + // heap, requiring a write barrier. + needs_write_barrier = true; + } + Node* start = IntPtrOrSmiConstant(0, mode); ElementsKind kind = PACKED_ELEMENTS; BuildFastFixedArrayForEach( diff --git a/deps/v8/src/ic/accessor-assembler.cc b/deps/v8/src/ic/accessor-assembler.cc index b93f4e6c0100da..06056f2d9657d6 100644 --- a/deps/v8/src/ic/accessor-assembler.cc +++ b/deps/v8/src/ic/accessor-assembler.cc @@ -3417,7 +3417,7 @@ void AccessorAssembler::GenerateStoreInArrayLiteralIC() { void AccessorAssembler::GenerateCloneObjectIC() { typedef CloneObjectWithVectorDescriptor Descriptor; - Node* source = Parameter(Descriptor::kSource); + TNode source = CAST(Parameter(Descriptor::kSource)); Node* flags = Parameter(Descriptor::kFlags); Node* slot = Parameter(Descriptor::kSlot); Node* vector = Parameter(Descriptor::kVector); @@ -3427,8 +3427,7 @@ void AccessorAssembler::GenerateCloneObjectIC() { Label miss(this, Label::kDeferred), try_polymorphic(this, Label::kDeferred), try_megamorphic(this, Label::kDeferred); - CSA_SLOW_ASSERT(this, TaggedIsNotSmi(source)); - Node* source_map = LoadMap(UncheckedCast(source)); + TNode source_map = LoadMap(UncheckedCast(source)); GotoIf(IsDeprecatedMap(source_map), &miss); TNode feedback = TryMonomorphicCase( slot, vector, source_map, &if_handler, &var_handler, &try_polymorphic); @@ -3449,7 +3448,7 @@ void AccessorAssembler::GenerateCloneObjectIC() { // The IC fast case should only be taken if the result map a compatible // elements kind with the source object. - TNode source_elements = LoadElements(source); + TNode source_elements = LoadElements(CAST(source)); auto flags = ExtractFixedArrayFlag::kAllFixedArraysDontCopyCOW; var_elements = CAST(CloneFixedArray(source_elements, flags)); @@ -3484,22 +3483,45 @@ void AccessorAssembler::GenerateCloneObjectIC() { // Lastly, clone any in-object properties. // Determine the inobject property capacity of both objects, and copy the // smaller number into the resulting object. - Node* source_start = LoadMapInobjectPropertiesStartInWords(source_map); - Node* source_size = LoadMapInstanceSizeInWords(source_map); - Node* result_start = LoadMapInobjectPropertiesStartInWords(result_map); - Node* field_offset_difference = + TNode source_start = + LoadMapInobjectPropertiesStartInWords(source_map); + TNode source_size = LoadMapInstanceSizeInWords(source_map); + TNode result_start = + LoadMapInobjectPropertiesStartInWords(result_map); + TNode field_offset_difference = TimesPointerSize(IntPtrSub(result_start, source_start)); - BuildFastLoop(source_start, source_size, - [=](Node* field_index) { - Node* field_offset = TimesPointerSize(field_index); - TNode field = LoadObjectField(source, field_offset); - field = CloneIfMutablePrimitive(field); - Node* result_offset = - IntPtrAdd(field_offset, field_offset_difference); - StoreObjectFieldNoWriteBarrier(object, result_offset, - field); - }, - 1, INTPTR_PARAMETERS, IndexAdvanceMode::kPost); + + // If MutableHeapNumbers may be present in-object, allocations may occur + // within this loop, thus the write barrier is required. + // + // TODO(caitp): skip the write barrier until the first MutableHeapNumber + // field is found + const bool may_use_mutable_heap_numbers = !FLAG_unbox_double_fields; + + BuildFastLoop( + source_start, source_size, + [=](Node* field_index) { + TNode field_offset = + TimesPointerSize(UncheckedCast(field_index)); + + if (may_use_mutable_heap_numbers) { + TNode field = LoadObjectField(source, field_offset); + field = CloneIfMutablePrimitive(field); + TNode result_offset = + IntPtrAdd(field_offset, field_offset_difference); + StoreObjectField(object, result_offset, field); + } else { + // Copy fields as raw data. + TNode field = UncheckedCast( + LoadObjectField(source, field_offset, MachineType::IntPtr())); + TNode result_offset = + IntPtrAdd(field_offset, field_offset_difference); + StoreObjectFieldNoWriteBarrier( + object, result_offset, field, + MachineType::IntPtr().representation()); + } + }, + 1, INTPTR_PARAMETERS, IndexAdvanceMode::kPost); Return(object); } diff --git a/deps/v8/test/mjsunit/es9/regress/regress-902965.js b/deps/v8/test/mjsunit/es9/regress/regress-902965.js new file mode 100644 index 00000000000000..e2035b242f25b4 --- /dev/null +++ b/deps/v8/test/mjsunit/es9/regress/regress-902965.js @@ -0,0 +1,12 @@ +// Copyright 2018 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. + +// Previously, spreading in-object properties would always treat double fields +// as tagged, potentially dereferencing a Float64. +function inobjectDouble() { + "use strict"; + this.x = -3.9; +} +const instance = new inobjectDouble(); +const clone = { ...instance, }; diff --git a/deps/v8/test/mjsunit/es9/regress/regress-903070.js b/deps/v8/test/mjsunit/es9/regress/regress-903070.js new file mode 100644 index 00000000000000..cca02ee0c41de2 --- /dev/null +++ b/deps/v8/test/mjsunit/es9/regress/regress-903070.js @@ -0,0 +1,15 @@ +// Copyright 2018 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. + +function clone(src) { + return { ...src }; +} + +function inobjectDoubles() { + "use strict"; + this.p0 = -6400510997704731; +} + +// Check that unboxed double is not treated as tagged +assertEquals({ p0: -6400510997704731 }, clone(new inobjectDoubles()));