Skip to content

Commit b5784fe

Browse files
BridgeARdanbev
authored andcommitted
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 [email protected], [email protected] Change-Id: I43df1ddc84dfa4840e680b6affeba452ce0b6629 Reviewed-on: https://chromium-review.googlesource.com/c/1318096 Commit-Queue: Caitlin Potter <[email protected]> Reviewed-by: Jakob Kummerow <[email protected]> Reviewed-by: Camillo Bruni <[email protected]> Cr-Commit-Position: refs/heads/master@{#57304} PR-URL: #25101 Refs: v8/v8@bf84766 Fixes: #25089 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ali Ijaz Sheikh <[email protected]> Reviewed-By: Yang Guo <[email protected]>
1 parent a981214 commit b5784fe

File tree

5 files changed

+83
-10
lines changed

5 files changed

+83
-10
lines changed

common.gypi

+1-1
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,7 @@
3838

3939
# Reset this number to 0 on major V8 upgrades.
4040
# Increment by one for each non-official patch applied to deps/v8.
41-
'v8_embedder_string': '-node.5',
41+
'v8_embedder_string': '-node.6',
4242

4343
##### V8 defaults for Node.js #####
4444

deps/v8/src/code-stub-assembler.cc

+38-2
Original file line numberDiff line numberDiff line change
@@ -3067,6 +3067,24 @@ TNode<MutableHeapNumber> CodeStubAssembler::AllocateMutableHeapNumber() {
30673067
return UncheckedCast<MutableHeapNumber>(result);
30683068
}
30693069

3070+
TNode<Object> CodeStubAssembler::CloneIfMutablePrimitive(TNode<Object> object) {
3071+
TVARIABLE(Object, result, object);
3072+
Label done(this);
3073+
3074+
GotoIf(TaggedIsSmi(object), &done);
3075+
GotoIfNot(IsMutableHeapNumber(UncheckedCast<HeapObject>(object)), &done);
3076+
{
3077+
// Mutable heap number found --- allocate a clone.
3078+
TNode<Float64T> value =
3079+
LoadHeapNumberValue(UncheckedCast<HeapNumber>(object));
3080+
result = AllocateMutableHeapNumberWithValue(value);
3081+
Goto(&done);
3082+
}
3083+
3084+
BIND(&done);
3085+
return result.value();
3086+
}
3087+
30703088
TNode<MutableHeapNumber> CodeStubAssembler::AllocateMutableHeapNumberWithValue(
30713089
SloppyTNode<Float64T> value) {
30723090
TNode<MutableHeapNumber> result = AllocateMutableHeapNumber();
@@ -4904,7 +4922,8 @@ void CodeStubAssembler::CopyPropertyArrayValues(Node* from_array,
49044922
Node* to_array,
49054923
Node* property_count,
49064924
WriteBarrierMode barrier_mode,
4907-
ParameterMode mode) {
4925+
ParameterMode mode,
4926+
DestroySource destroy_source) {
49084927
CSA_SLOW_ASSERT(this, MatchesParameterMode(property_count, mode));
49094928
CSA_SLOW_ASSERT(this, Word32Or(IsPropertyArray(from_array),
49104929
IsEmptyFixedArray(from_array)));
@@ -4916,9 +4935,14 @@ void CodeStubAssembler::CopyPropertyArrayValues(Node* from_array,
49164935
ElementsKind kind = PACKED_ELEMENTS;
49174936
BuildFastFixedArrayForEach(
49184937
from_array, kind, start, property_count,
4919-
[this, to_array, needs_write_barrier](Node* array, Node* offset) {
4938+
[this, to_array, needs_write_barrier, destroy_source](Node* array,
4939+
Node* offset) {
49204940
Node* value = Load(MachineType::AnyTagged(), array, offset);
49214941

4942+
if (destroy_source == DestroySource::kNo) {
4943+
value = CloneIfMutablePrimitive(CAST(value));
4944+
}
4945+
49224946
if (needs_write_barrier) {
49234947
Store(to_array, offset, value);
49244948
} else {
@@ -4927,6 +4951,18 @@ void CodeStubAssembler::CopyPropertyArrayValues(Node* from_array,
49274951
}
49284952
},
49294953
mode);
4954+
4955+
#ifdef DEBUG
4956+
// Zap {from_array} if the copying above has made it invalid.
4957+
if (destroy_source == DestroySource::kYes) {
4958+
Label did_zap(this);
4959+
GotoIf(IsEmptyFixedArray(from_array), &did_zap);
4960+
FillPropertyArrayWithUndefined(from_array, start, property_count, mode);
4961+
4962+
Goto(&did_zap);
4963+
BIND(&did_zap);
4964+
}
4965+
#endif
49304966
Comment("] CopyPropertyArrayValues");
49314967
}
49324968

deps/v8/src/code-stub-assembler.h

+17-4
Original file line numberDiff line numberDiff line change
@@ -1512,10 +1512,19 @@ class V8_EXPORT_PRIVATE CodeStubAssembler : public compiler::CodeAssembler {
15121512
Node* to_index,
15131513
ParameterMode mode = INTPTR_PARAMETERS);
15141514

1515-
void CopyPropertyArrayValues(
1516-
Node* from_array, Node* to_array, Node* length,
1517-
WriteBarrierMode barrier_mode = UPDATE_WRITE_BARRIER,
1518-
ParameterMode mode = INTPTR_PARAMETERS);
1515+
enum class DestroySource { kNo, kYes };
1516+
1517+
// Specify DestroySource::kYes if {from_array} is being supplanted by
1518+
// {to_array}. This offers a slight performance benefit by simply copying the
1519+
// array word by word. The source may be destroyed at the end of this macro.
1520+
//
1521+
// Otherwise, specify DestroySource::kNo for operations where an Object is
1522+
// being cloned, to ensure that MutableHeapNumbers are unique between the
1523+
// source and cloned object.
1524+
void CopyPropertyArrayValues(Node* from_array, Node* to_array, Node* length,
1525+
WriteBarrierMode barrier_mode,
1526+
ParameterMode mode,
1527+
DestroySource destroy_source);
15191528

15201529
// Copies all elements from |from_array| of |length| size to
15211530
// |to_array| of the same size respecting the elements kind.
@@ -3073,6 +3082,10 @@ class V8_EXPORT_PRIVATE CodeStubAssembler : public compiler::CodeAssembler {
30733082
void InitializeFunctionContext(Node* native_context, Node* context,
30743083
int slots);
30753084

3085+
// Allocate a clone of a mutable primitive, if {object} is a
3086+
// MutableHeapNumber.
3087+
TNode<Object> CloneIfMutablePrimitive(TNode<Object> object);
3088+
30763089
private:
30773090
friend class CodeStubArguments;
30783091

deps/v8/src/ic/accessor-assembler.cc

+5-3
Original file line numberDiff line numberDiff line change
@@ -1683,7 +1683,8 @@ Node* AccessorAssembler::ExtendPropertiesBackingStore(Node* object,
16831683
// |new_properties| is guaranteed to be in new space, so we can skip
16841684
// the write barrier.
16851685
CopyPropertyArrayValues(var_properties.value(), new_properties,
1686-
var_length.value(), SKIP_WRITE_BARRIER, mode);
1686+
var_length.value(), SKIP_WRITE_BARRIER, mode,
1687+
DestroySource::kYes);
16871688

16881689
// TODO(gsathya): Clean up the type conversions by creating smarter
16891690
// helpers that do the correct op based on the mode.
@@ -3620,7 +3621,7 @@ void AccessorAssembler::GenerateCloneObjectIC() {
36203621
auto mode = INTPTR_PARAMETERS;
36213622
var_properties = CAST(AllocatePropertyArray(length, mode));
36223623
CopyPropertyArrayValues(source_properties, var_properties.value(), length,
3623-
SKIP_WRITE_BARRIER, mode);
3624+
SKIP_WRITE_BARRIER, mode, DestroySource::kNo);
36243625
}
36253626

36263627
Goto(&allocate_object);
@@ -3640,7 +3641,8 @@ void AccessorAssembler::GenerateCloneObjectIC() {
36403641
BuildFastLoop(source_start, source_size,
36413642
[=](Node* field_index) {
36423643
Node* field_offset = TimesPointerSize(field_index);
3643-
Node* field = LoadObjectField(source, field_offset);
3644+
TNode<Object> field = LoadObjectField(source, field_offset);
3645+
field = CloneIfMutablePrimitive(field);
36443646
Node* result_offset =
36453647
IntPtrAdd(field_offset, field_offset_difference);
36463648
StoreObjectFieldNoWriteBarrier(object, result_offset,

deps/v8/test/mjsunit/es9/object-spread-ic.js

+22
Original file line numberDiff line numberDiff line change
@@ -99,3 +99,25 @@
9999
// Megamorphic
100100
assertEquals({ boop: 1 }, f({ boop: 1 }));
101101
})();
102+
103+
// There are 2 paths in CloneObjectIC's handler which need to handle double
104+
// fields specially --- in object properties, and copying the property array.
105+
function testMutableInlineProperties() {
106+
function inobject() { "use strict"; this.x = 1.1; }
107+
const src = new inobject();
108+
const x0 = src.x;
109+
const clone = { ...src, x: x0 + 1 };
110+
assertEquals(x0, src.x);
111+
assertEquals({ x: 2.1 }, clone);
112+
}
113+
testMutableInlineProperties()
114+
115+
function testMutableOutOfLineProperties() {
116+
const src = { a: 1, b: 2, c: 3 };
117+
src.x = 2.3;
118+
const x0 = src.x;
119+
const clone = { ...src, x: x0 + 1 };
120+
assertEquals(x0, src.x);
121+
assertEquals({ a: 1, b: 2, c: 3, x: 3.3 }, clone);
122+
}
123+
testMutableOutOfLineProperties();

0 commit comments

Comments
 (0)