From b5784fe5d598c9c8076cacaad7105a05457b22b4 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Thu, 20 Dec 2018 14:33:29 +0100 Subject: [PATCH] deps: V8: backport bf84766 Original commit message: [CloneObjectIC] clone MutableHeapNumbers instead of referencing them Adds a helper macro "CloneIfMutablePrimitive", which tests if the operand is a MutableHeapNumber, and if so, clones it, otherwise returning the original value. Also modifies the signature of "CopyPropertyArrayValues" to take a "DestroySource" enum, indicating whether or not the resulting object is supplanting the source object or not, and removes all default parameters from that macro (which were not used anyways). This corrects the issue reported in chromium:901301, where StaNamedOwnProperty was replacing the value of a MutableHeapNumber referenced by both the cloned object and the source object. BUG=chromium:901301, v8:7611 R=cbruni@chromium.org, jkummerow@chromium.org Change-Id: I43df1ddc84dfa4840e680b6affeba452ce0b6629 Reviewed-on: https://chromium-review.googlesource.com/c/1318096 Commit-Queue: Caitlin Potter Reviewed-by: Jakob Kummerow Reviewed-by: Camillo Bruni Cr-Commit-Position: refs/heads/master@{#57304} PR-URL: https://github.com/nodejs/node/pull/25101 Refs: https://github.com/v8/v8/commit/bf84766a2cd3e09070adcd6228a3a487c8dc4bbd 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 +- deps/v8/src/code-stub-assembler.cc | 40 +++++++++++++++++++- deps/v8/src/code-stub-assembler.h | 21 ++++++++-- deps/v8/src/ic/accessor-assembler.cc | 8 ++-- deps/v8/test/mjsunit/es9/object-spread-ic.js | 22 +++++++++++ 5 files changed, 83 insertions(+), 10 deletions(-) diff --git a/common.gypi b/common.gypi index cc916c6c226..6c4e3d5375b 100644 --- a/common.gypi +++ b/common.gypi @@ -38,7 +38,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.5', + 'v8_embedder_string': '-node.6', ##### V8 defaults for Node.js ##### diff --git a/deps/v8/src/code-stub-assembler.cc b/deps/v8/src/code-stub-assembler.cc index 1474e3d97de..e11beb6541d 100644 --- a/deps/v8/src/code-stub-assembler.cc +++ b/deps/v8/src/code-stub-assembler.cc @@ -3067,6 +3067,24 @@ TNode CodeStubAssembler::AllocateMutableHeapNumber() { return UncheckedCast(result); } +TNode CodeStubAssembler::CloneIfMutablePrimitive(TNode object) { + TVARIABLE(Object, result, object); + Label done(this); + + GotoIf(TaggedIsSmi(object), &done); + GotoIfNot(IsMutableHeapNumber(UncheckedCast(object)), &done); + { + // Mutable heap number found --- allocate a clone. + TNode value = + LoadHeapNumberValue(UncheckedCast(object)); + result = AllocateMutableHeapNumberWithValue(value); + Goto(&done); + } + + BIND(&done); + return result.value(); +} + TNode CodeStubAssembler::AllocateMutableHeapNumberWithValue( SloppyTNode value) { TNode result = AllocateMutableHeapNumber(); @@ -4904,7 +4922,8 @@ void CodeStubAssembler::CopyPropertyArrayValues(Node* from_array, Node* to_array, Node* property_count, WriteBarrierMode barrier_mode, - ParameterMode mode) { + ParameterMode mode, + DestroySource destroy_source) { CSA_SLOW_ASSERT(this, MatchesParameterMode(property_count, mode)); CSA_SLOW_ASSERT(this, Word32Or(IsPropertyArray(from_array), IsEmptyFixedArray(from_array))); @@ -4916,9 +4935,14 @@ void CodeStubAssembler::CopyPropertyArrayValues(Node* from_array, ElementsKind kind = PACKED_ELEMENTS; BuildFastFixedArrayForEach( from_array, kind, start, property_count, - [this, to_array, needs_write_barrier](Node* array, Node* offset) { + [this, to_array, needs_write_barrier, destroy_source](Node* array, + Node* offset) { Node* value = Load(MachineType::AnyTagged(), array, offset); + if (destroy_source == DestroySource::kNo) { + value = CloneIfMutablePrimitive(CAST(value)); + } + if (needs_write_barrier) { Store(to_array, offset, value); } else { @@ -4927,6 +4951,18 @@ void CodeStubAssembler::CopyPropertyArrayValues(Node* from_array, } }, mode); + +#ifdef DEBUG + // Zap {from_array} if the copying above has made it invalid. + if (destroy_source == DestroySource::kYes) { + Label did_zap(this); + GotoIf(IsEmptyFixedArray(from_array), &did_zap); + FillPropertyArrayWithUndefined(from_array, start, property_count, mode); + + Goto(&did_zap); + BIND(&did_zap); + } +#endif Comment("] CopyPropertyArrayValues"); } diff --git a/deps/v8/src/code-stub-assembler.h b/deps/v8/src/code-stub-assembler.h index 8bd39369b79..2be168af851 100644 --- a/deps/v8/src/code-stub-assembler.h +++ b/deps/v8/src/code-stub-assembler.h @@ -1512,10 +1512,19 @@ class V8_EXPORT_PRIVATE CodeStubAssembler : public compiler::CodeAssembler { Node* to_index, ParameterMode mode = INTPTR_PARAMETERS); - void CopyPropertyArrayValues( - Node* from_array, Node* to_array, Node* length, - WriteBarrierMode barrier_mode = UPDATE_WRITE_BARRIER, - ParameterMode mode = INTPTR_PARAMETERS); + enum class DestroySource { kNo, kYes }; + + // Specify DestroySource::kYes if {from_array} is being supplanted by + // {to_array}. This offers a slight performance benefit by simply copying the + // array word by word. The source may be destroyed at the end of this macro. + // + // Otherwise, specify DestroySource::kNo for operations where an Object is + // being cloned, to ensure that MutableHeapNumbers are unique between the + // source and cloned object. + void CopyPropertyArrayValues(Node* from_array, Node* to_array, Node* length, + WriteBarrierMode barrier_mode, + ParameterMode mode, + DestroySource destroy_source); // Copies all elements from |from_array| of |length| size to // |to_array| of the same size respecting the elements kind. @@ -3073,6 +3082,10 @@ class V8_EXPORT_PRIVATE CodeStubAssembler : public compiler::CodeAssembler { void InitializeFunctionContext(Node* native_context, Node* context, int slots); + // Allocate a clone of a mutable primitive, if {object} is a + // MutableHeapNumber. + TNode CloneIfMutablePrimitive(TNode object); + private: friend class CodeStubArguments; diff --git a/deps/v8/src/ic/accessor-assembler.cc b/deps/v8/src/ic/accessor-assembler.cc index f730c505552..9ba5cde7547 100644 --- a/deps/v8/src/ic/accessor-assembler.cc +++ b/deps/v8/src/ic/accessor-assembler.cc @@ -1683,7 +1683,8 @@ Node* AccessorAssembler::ExtendPropertiesBackingStore(Node* object, // |new_properties| is guaranteed to be in new space, so we can skip // the write barrier. CopyPropertyArrayValues(var_properties.value(), new_properties, - var_length.value(), SKIP_WRITE_BARRIER, mode); + var_length.value(), SKIP_WRITE_BARRIER, mode, + DestroySource::kYes); // TODO(gsathya): Clean up the type conversions by creating smarter // helpers that do the correct op based on the mode. @@ -3620,7 +3621,7 @@ void AccessorAssembler::GenerateCloneObjectIC() { auto mode = INTPTR_PARAMETERS; var_properties = CAST(AllocatePropertyArray(length, mode)); CopyPropertyArrayValues(source_properties, var_properties.value(), length, - SKIP_WRITE_BARRIER, mode); + SKIP_WRITE_BARRIER, mode, DestroySource::kNo); } Goto(&allocate_object); @@ -3640,7 +3641,8 @@ void AccessorAssembler::GenerateCloneObjectIC() { BuildFastLoop(source_start, source_size, [=](Node* field_index) { Node* field_offset = TimesPointerSize(field_index); - Node* field = LoadObjectField(source, field_offset); + TNode field = LoadObjectField(source, field_offset); + field = CloneIfMutablePrimitive(field); Node* result_offset = IntPtrAdd(field_offset, field_offset_difference); StoreObjectFieldNoWriteBarrier(object, result_offset, diff --git a/deps/v8/test/mjsunit/es9/object-spread-ic.js b/deps/v8/test/mjsunit/es9/object-spread-ic.js index d76ffd4eb80..55d60f2cf8a 100644 --- a/deps/v8/test/mjsunit/es9/object-spread-ic.js +++ b/deps/v8/test/mjsunit/es9/object-spread-ic.js @@ -99,3 +99,25 @@ // Megamorphic assertEquals({ boop: 1 }, f({ boop: 1 })); })(); + +// There are 2 paths in CloneObjectIC's handler which need to handle double +// fields specially --- in object properties, and copying the property array. +function testMutableInlineProperties() { + function inobject() { "use strict"; this.x = 1.1; } + const src = new inobject(); + const x0 = src.x; + const clone = { ...src, x: x0 + 1 }; + assertEquals(x0, src.x); + assertEquals({ x: 2.1 }, clone); +} +testMutableInlineProperties() + +function testMutableOutOfLineProperties() { + const src = { a: 1, b: 2, c: 3 }; + src.x = 2.3; + const x0 = src.x; + const clone = { ...src, x: x0 + 1 }; + assertEquals(x0, src.x); + assertEquals({ a: 1, b: 2, c: 3, x: 3.3 }, clone); +} +testMutableOutOfLineProperties();