Skip to content

deps: V8: fix spread operator - #25101

Closed
BridgeAR wants to merge 3 commits into
nodejs:masterfrom
BridgeAR:fix-spread-operator
Closed

deps: V8: fix spread operator#25101
BridgeAR wants to merge 3 commits into
nodejs:masterfrom
BridgeAR:fix-spread-operator

Conversation

@BridgeAR

@BridgeARBridgeAR commented Dec 18, 2018

Copy link
Copy Markdown
Member

The spread operator currently mutates some input objects. This is already fixed in V8, so I just went ahead to backport these commits.

Fixes: #25089

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot

nodejs-github-bot commented Dec 18, 2018

Copy link
Copy Markdown
Collaborator

@BridgeAR build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/1986/pipeline

@nodejs-github-botnodejs-github-bot added build Issues and PRs related to build files or the CI. v8 engine Issues and PRs related to the V8 dependency. labels Dec 18, 2018
@BridgeAR

BridgeAR commented Dec 18, 2018

Copy link
Copy Markdown
MemberAuthor

CI https://ci.nodejs.org/job/node-test-pull-request/19647/
V8-CI https://ci.nodejs.org/job/node-test-commit-v8-linux/1944/

@BridgeARBridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 18, 2018
@targos

targos commented Dec 18, 2018

Copy link
Copy Markdown
Member

It seems this depends on some other change

@BridgeAR

BridgeAR commented Dec 18, 2018

Copy link
Copy Markdown
MemberAuthor

@targos seems so... the commits landed cleanly, that's why I expected it to work but that is obviously wrong.

@BridgeARBridgeAR added wip Issues and PRs that are still a work in progress. and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Dec 18, 2018
@jasnell

jasnell commented Dec 18, 2018

Copy link
Copy Markdown
Member

Idea of landing this is +1 ... obviously need to figure out what other change may be needed :-)

@Trott

Trott commented Dec 18, 2018•
edited
Loading

Copy link
Copy Markdown
Member

The linked CI above is for #22712. (Posting CI in the wrong window is something I've done many times myself.) The correct CI link is https://ci.nodejs.org/job/node-test-pull-request/19649/. Everything failed to compile so this definitely needs some adjustments (which is also indicated by the work in progress (WIP) label).

@BridgeAR

BridgeAR commented Dec 19, 2018

Copy link
Copy Markdown
MemberAuthor

@Trott thanks for catching that!

@BridgeAR
deps: patch V8 to 7.1.302.33
c9bbf5a
Refs: v8/v8@7.1.302.28...7.1.302.33
@BridgeAR
deps: V8: backport bf84766
feba312
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@{nodejs#57304}
Refs: v8/v8@bf84766
@BridgeAR
deps: V8: backport3e010af
6096720
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 <caitp@igalia.com>
Reviewed-by: Camillo Bruni <cbruni@chromium.org>
Reviewed-by: Igor Sheludko <ishell@chromium.org>
Cr-Commit-Position: refs/heads/master@{nodejs#57368}
Refs: v8/v8@3e010af
@BridgeAR

BridgeAR commented Dec 20, 2018

Copy link
Copy Markdown
MemberAuthor

@GeorgNeis was so kind to provide a patch that resolved the build issues. Since the patch was build upon 7.1.302.32, I went ahead and updated V8 first and then applied the necessary backports on top of that including the changes from the mentioned patch.

This will therefore also improve the Array.prototype.splice performance and add some other minor improvements.

CI https://ci.nodejs.org/job/node-test-pull-request/19692/
V8-CI https://ci.nodejs.org/job/node-test-commit-v8-linux/1949/

@BridgeARBridgeAR removed the wip Issues and PRs that are still a work in progress. label Dec 20, 2018
@targos

targos commented Dec 20, 2018

Copy link
Copy Markdown
Member

/cc @nodejs/v8-update

@BridgeARBridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 20, 2018
@danbev

danbev commented Dec 21, 2018

Copy link
Copy Markdown
Contributor

Landed in a981214, b5784fe, and 4884ca6

@danbevdanbev closed this Dec 21, 2018
danbev pushed a commit that referenced this pull request Dec 21, 2018
@BridgeAR@danbev
deps: patch V8 to 7.1.302.33
a981214
PR-URL: #25101
Refs: v8/v8@7.1.302.28...7.1.302.33Fixes: #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>
danbev pushed a commit that referenced this pull request Dec 21, 2018
@BridgeAR@danbev
deps: V8: backport bf84766
b5784fe
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@bf84766Fixes: #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>
danbev pushed a commit that referenced this pull request Dec 21, 2018
@BridgeAR@danbev
deps: V8: backport3e010af
4884ca6
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 <caitp@igalia.com>
Reviewed-by: Camillo Bruni <cbruni@chromium.org>
Reviewed-by: Igor Sheludko <ishell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#57368}
PR-URL: #25101
Refs: v8/v8@3e010afFixes: #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>
@MylesBorins

MylesBorins commented Dec 25, 2018

Copy link
Copy Markdown
Contributor

Should this be backported to v11.x?

@targostargos added backported-to-v11.x and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. build Issues and PRs related to build files or the CI. backport-requested-v11.x labels Jan 1, 2019
@targos

targos commented Jan 1, 2019

Copy link
Copy Markdown
Member

cherry-picked to v11.x-staging without the V8 update

targos pushed a commit that referenced this pull request Jan 1, 2019
@BridgeAR@targos
deps: V8: backport bf84766
201cf97
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@bf84766Fixes: #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>
targos pushed a commit that referenced this pull request Jan 1, 2019
@BridgeAR@targos
deps: V8: backport3e010af
f2abe7b
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 <caitp@igalia.com>
Reviewed-by: Camillo Bruni <cbruni@chromium.org>
Reviewed-by: Igor Sheludko <ishell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#57368}
PR-URL: #25101
Refs: v8/v8@3e010afFixes: #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>
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
@BridgeAR@refack
deps: patch V8 to 7.1.302.33
64a0cd5
PR-URL: nodejs#25101
Refs: v8/v8@7.1.302.28...7.1.302.33Fixes: nodejs#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>
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
@BridgeAR@refack
deps: V8: backport bf84766
8e2ba9e
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@{nodejs#57304}
PR-URL: nodejs#25101
Refs: v8/v8@bf84766Fixes: nodejs#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>
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
@BridgeAR@refack
deps: V8: backport3e010af
25a978e
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 <caitp@igalia.com>
Reviewed-by: Camillo Bruni <cbruni@chromium.org>
Reviewed-by: Igor Sheludko <ishell@chromium.org>
Cr-Commit-Position: refs/heads/master@{nodejs#57368}
PR-URL: nodejs#25101
Refs: v8/v8@3e010afFixes: nodejs#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>
@BridgeARBridgeAR mentioned this pull request Jan 16, 2019
Merged
@MylesBorinsMylesBorins mentioned this pull request Jan 24, 2019
Merged
This was referenced Apr 23, 2019
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
@BridgeAR
BridgeAR deleted the fix-spread-operator branch January 20, 2020 11:46
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnelljasnelljasnell approved these changes
@richardlaurichardlaurichardlau approved these changes
+3 more reviewers
@ofrobotsofrobotsofrobots approved these changes
@hashseedhashseedhashseed approved these changes
@devsnekdevsnekdevsnek approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

v8 engineIssues and PRs related to the V8 dependency.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

object spread operator mutates first argument

12 participants

@BridgeAR@nodejs-github-bot@targos@jasnell@Trott@danbev@MylesBorins@ofrobots@hashseed@richardlau@devsnek@BethGriggs