Skip to content

Commit

Permalink
deps: V8: backport bf84766
Browse files Browse the repository at this point in the history
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 <caitp@igalia.com>
    Reviewed-by: Jakob Kummerow <jkummerow@chromium.org>
    Reviewed-by: Camillo Bruni <cbruni@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#57304}

PR-URL: #25101
Refs: v8/v8@bf84766
Fixes: #25089
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Yang Guo <yangguo@chromium.org>
  • Loading branch information
BridgeAR authored and targos committed Jan 1, 2019
1 parent f62e35f commit 201cf97
Show file tree
Hide file tree
Showing 5 changed files with 83 additions and 10 deletions.
2 changes: 1 addition & 1 deletion common.gypi
Original file line number Diff line number Diff line change
Expand Up @@ -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.13',
'v8_embedder_string': '-node.14',

# Enable disassembler for `--print-code` v8 options
'v8_enable_disassembler': 1,
Expand Down
40 changes: 38 additions & 2 deletions deps/v8/src/code-stub-assembler.cc
Original file line number Diff line number Diff line change
Expand Up @@ -2984,6 +2984,24 @@ TNode<MutableHeapNumber> CodeStubAssembler::AllocateMutableHeapNumber() {
return UncheckedCast<MutableHeapNumber>(result);
}

TNode<Object> CodeStubAssembler::CloneIfMutablePrimitive(TNode<Object> object) {
TVARIABLE(Object, result, object);
Label done(this);

GotoIf(TaggedIsSmi(object), &done);
GotoIfNot(IsMutableHeapNumber(UncheckedCast<HeapObject>(object)), &done);
{
// Mutable heap number found --- allocate a clone.
TNode<Float64T> value =
LoadHeapNumberValue(UncheckedCast<HeapNumber>(object));
result = AllocateMutableHeapNumberWithValue(value);
Goto(&done);
}

BIND(&done);
return result.value();
}

TNode<MutableHeapNumber> CodeStubAssembler::AllocateMutableHeapNumberWithValue(
SloppyTNode<Float64T> value) {
TNode<MutableHeapNumber> result = AllocateMutableHeapNumber();
Expand Down Expand Up @@ -4405,7 +4423,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)));
Expand All @@ -4417,9 +4436,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 {
Expand All @@ -4428,6 +4452,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");
}

Expand Down
21 changes: 17 additions & 4 deletions deps/v8/src/code-stub-assembler.h
Original file line number Diff line number Diff line change
Expand Up @@ -1454,10 +1454,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.
Expand Down Expand Up @@ -2864,6 +2873,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<Object> CloneIfMutablePrimitive(TNode<Object> object);

private:
friend class CodeStubArguments;

Expand Down
8 changes: 5 additions & 3 deletions deps/v8/src/ic/accessor-assembler.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1667,7 +1667,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.
Expand Down Expand Up @@ -3471,7 +3472,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);
Expand All @@ -3491,7 +3492,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<Object> field = LoadObjectField(source, field_offset);
field = CloneIfMutablePrimitive(field);
Node* result_offset =
IntPtrAdd(field_offset, field_offset_difference);
StoreObjectFieldNoWriteBarrier(object, result_offset,
Expand Down
22 changes: 22 additions & 0 deletions deps/v8/test/mjsunit/es9/object-spread-ic.js
Original file line number Diff line number Diff line change
Expand Up @@ -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();

0 comments on commit 201cf97

Please sign in to comment.