deps: V8: cherry-pick dfcdf7837e23 by bcoe · Pull Request #36573 · nodejs/node

bcoe · 2020-12-18T22:49:34Z

Coverage for nullish coalescing was broken in v8, and eating more characters than one would expect:

This patch fixes it:

Original commit message:

[coverage] fix greedy nullish coalescing

The SourceRangeScope helper was consuming too many characters, instead
explicitly create SourceRange, based on scanner position.

Bug: v8:11231
Change-Id: I852d211227abacf867e8f1ab3e3ab06dbdba2a9b
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2576006
Reviewed-by: Toon Verwaest <verwaest@chromium.org>
Commit-Queue: Toon Verwaest <verwaest@chromium.org>
Cr-Commit-Position: refs/heads/master@{#71765}

Refs: v8/v8@dfcdf78

Related Issues

Fixes: #36619

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

targos · 2020-12-19T10:04:58Z

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/3620/

nodejs-github-bot · 2020-12-19T10:29:55Z

CI: https://ci.nodejs.org/job/node-test-pull-request/35008/

bcoe · 2020-12-20T18:09:29Z

@targos it looks like the v8 build is failing across the board?

targos · 2020-12-21T13:51:11Z

@targos it looks like the v8 build is failing across the board?

It looks like a CI flake. Trying again: https://ci.nodejs.org/job/node-test-commit-v8-linux/3625/

Edit: again after cleaning the workspace: https://ci.nodejs.org/job/node-test-commit-v8-linux/3627/

mhdawson

LGTM

richardlau · 2020-12-21T17:27:45Z

@targos it looks like the v8 build is failing across the board?

It looks like a CI flake. Trying again: https://ci.nodejs.org/job/node-test-commit-v8-linux/3625/

Edit: again after cleaning the workspace: https://ci.nodejs.org/job/node-test-commit-v8-linux/3627/

Looking at https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=benchmark-ubuntu1604-intel-64,v8test=v8test/ I don't think this is a flake -- it looks like the build is broken. The timing of when the build broke appears to line up with 0e96dc1#diff-696862ddcbd7859bd9800af118c1a6e409c4f49f9c7c23d9025040f22c7f12c7 -- is it possible the script (tools/dev/v8gen.py) doesn't like empty arguments ("")?

richardlau · 2020-12-21T17:39:24Z

@targos it looks like the v8 build is failing across the board?

It looks like a CI flake. Trying again: https://ci.nodejs.org/job/node-test-commit-v8-linux/3625/
Edit: again after cleaning the workspace: https://ci.nodejs.org/job/node-test-commit-v8-linux/3627/

Looking at https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=benchmark-ubuntu1604-intel-64,v8test=v8test/ I don't think this is a flake -- it looks like the build is broken. The timing of when the build broke appears to line up with 0e96dc1#diff-696862ddcbd7859bd9800af118c1a6e409c4f49f9c7c23d9025040f22c7f12c7 -- is it possible the script (tools/dev/v8gen.py) doesn't like empty arguments ("")?

Looks so:

$ python2 tools/dev/v8gen.py "x64.release" --no-goma
$ python2 tools/dev/v8gen.py "x64.release" --no-goma ""
usage: v8gen.py [-h] {gen,list} ...
v8gen.py: error: unrecognized arguments:
$

Original commit message: [coverage] fix greedy nullish coalescing The SourceRangeScope helper was consuming too many characters, instead explicitly create SourceRange, based on scanner position. Bug: v8:11231 Change-Id: I852d211227abacf867e8f1ab3e3ab06dbdba2a9b Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2576006 Reviewed-by: Toon Verwaest <verwaest@chromium.org> Commit-Queue: Toon Verwaest <verwaest@chromium.org> Cr-Commit-Position: refs/heads/master@{#71765} Refs: v8/v8@dfcdf78

nodejs-github-bot · 2020-12-22T03:13:19Z

CI: https://ci.nodejs.org/job/node-test-pull-request/35033/
v8: https://ci.nodejs.org/job/node-test-commit-v8-linux/3630/

nodejs-github-bot · 2020-12-22T06:04:26Z

CI: https://ci.nodejs.org/job/node-test-pull-request/35035/

github-actions · 2020-12-24T09:39:00Z

Commit Queue failed

- Loading data for nodejs/node/pull/36573 ✔ Done loading data for nodejs/node/pull/36573 ----------------------------------- PR info ------------------------------------ Title deps: V8: cherry-pick dfcdf7837e23 (#36573) Author Benjamin E. Coe (@bcoe) Branch bcoe:fix-nullish -> nodejs:master Labels V8 Engine, build Commits 1 - deps: V8: cherry-pick dfcdf7837e23 Committers 1 - Benjamin Coe PR-URL: https://github.com/nodejs/node/pull/36573 Fixes: https://github.com/https://github.com/nodejs/node/issues/ Refs: https://github.com/v8/v8/commit/dfcdf7837e23cc0da31f9b2d4211f856413d13af Reviewed-By: Michaël Zasso Reviewed-By: Rich Trott Reviewed-By: Michael Dawson Reviewed-By: Jiawen Geng ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/36573 Fixes: https://github.com/https://github.com/nodejs/node/issues/ Refs: https://github.com/v8/v8/commit/dfcdf7837e23cc0da31f9b2d4211f856413d13af Reviewed-By: Michaël Zasso Reviewed-By: Rich Trott Reviewed-By: Michael Dawson Reviewed-By: Jiawen Geng -------------------------------------------------------------------------------- ✔ Last GitHub Actions successful ℹ Last V8 CI on 2020-12-22T03:13:19Z: https://ci.nodejs.org/job/node-test-commit-v8-linux/3630/ ℹ Last Full PR CI on 2020-12-22T06:04:26Z: https://ci.nodejs.org/job/node-test-pull-request/35035/ - Querying data for job/node-test-pull-request/35035/ ✔ Build data downloaded ✔ Last Jenkins CI successful ℹ This PR was created on Fri, 18 Dec 2020 22:49:34 GMT ✔ Approvals: 4 ✔ - Michaël Zasso (@targos) (TSC): https://github.com/nodejs/node/pull/36573#pullrequestreview-555931323 ✔ - Rich Trott (@Trott) (TSC): https://github.com/nodejs/node/pull/36573#pullrequestreview-556378221 ✔ - Michael Dawson (@mhdawson) (TSC): https://github.com/nodejs/node/pull/36573#pullrequestreview-556517155 ✔ - Jiawen Geng (@gengjiawen): https://github.com/nodejs/node/pull/36573#pullrequestreview-558419166 -------------------------------------------------------------------------------- ✔ No git cherry-pick in progress ✔ No git am in progress ✔ No git rebase in progress -------------------------------------------------------------------------------- - Bringing origin/master up to date... From https://github.com/nodejs/node * branch master -> FETCH_HEAD ✔ origin/master is now up-to-date - Downloading patch for 36573 From https://github.com/nodejs/node * branch refs/pull/36573/merge -> FETCH_HEAD ✔ Fetched commits as 2c8751cb855a..fba1e5f86bfa -------------------------------------------------------------------------------- [master bd35b3b3b2] deps: V8: cherry-pick dfcdf7837e23 Author: Benjamin Coe Date: Fri Dec 18 10:40:07 2020 -0800 3 files changed, 28 insertions(+), 7 deletions(-) ✔ Patches applied -------------------------------------------------------------------------------- ⚠ Found Refs: https://github.com/v8/v8/commit/dfcdf7837e23cc0da31f9b2d4211f856413d13af, skipping.. --------------------------------- New Message ---------------------------------- deps: V8: cherry-pick dfcdf7837e23

Original commit message:

[coverage] fix greedy nullish coalescing

The SourceRangeScope helper was consuming too many characters, instead
explicitly create SourceRange, based on scanner position.

Bug: v8:11231
Change-Id: I852d211227abacf867e8f1ab3e3ab06dbdba2a9b
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2576006
Reviewed-by: Toon Verwaest <verwaest@chromium.org>
Commit-Queue: Toon Verwaest <verwaest@chromium.org>
Cr-Commit-Position: refs/heads/master@{#71765}

Refs: v8/v8@dfcdf78

PR-URL: #36573
Fixes: https://github.com/https://github.com/nodejs/node/issues/<issue_number>
Reviewed-By: Michaël Zasso targos@protonmail.com
Reviewed-By: Rich Trott rtrott@gmail.com
Reviewed-By: Michael Dawson midawson@redhat.com
Reviewed-By: Jiawen Geng technicalcute@gmail.com

[master a2698e1924] deps: V8: cherry-pick dfcdf7837e23
Author: Benjamin Coe bencoe@google.com
Date: Fri Dec 18 10:40:07 2020 -0800
3 files changed, 28 insertions(+), 7 deletions(-)
✖ a2698e192446ae6606095100adab24da799b3647
✖ 18:7 Fixes must be a GitHub URL. fixes-url
✔ 0:0 blank line after title line-after-title
✔ 0:0 line-lengths are valid line-length
✔ 0:0 metadata is at end of message metadata-end
✔ 17:8 PR-URL is valid. pr-url
✔ 0:0 reviewers are valid reviewers
✔ 0:0 valid subsystems subsystem
✔ 0:0 Title is formatted correctly. title-format
✔ 0:0 Title is <= 50 columns. title-length

ℹ Please fix the commit message and try again.

https://github.com/nodejs/node/actions/runs/442076143

gengjiawen · 2020-12-24T09:42:57Z

Related Issues
Fixes: https://github.com/nodejs/node/issues/<issue_number>

@bcoe looks you forget write the issue number, which lead to commit failure.

github-actions · 2020-12-24T18:07:03Z

Landed in 3b0ecfc...33d99b6

Original commit message: [coverage] fix greedy nullish coalescing The SourceRangeScope helper was consuming too many characters, instead explicitly create SourceRange, based on scanner position. Bug: v8:11231 Change-Id: I852d211227abacf867e8f1ab3e3ab06dbdba2a9b Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2576006 Reviewed-by: Toon Verwaest <verwaest@chromium.org> Commit-Queue: Toon Verwaest <verwaest@chromium.org> Cr-Commit-Position: refs/heads/master@{#71765} Refs: v8/v8@dfcdf78 PR-URL: #36573 Fixes: #36619 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com> Reviewed-By: Jiawen Geng <technicalcute@gmail.com>

Original commit message: [coverage] fix greedy nullish coalescing The SourceRangeScope helper was consuming too many characters, instead explicitly create SourceRange, based on scanner position. Bug: v8:11231 Change-Id: I852d211227abacf867e8f1ab3e3ab06dbdba2a9b Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2576006 Reviewed-by: Toon Verwaest <verwaest@chromium.org> Commit-Queue: Toon Verwaest <verwaest@chromium.org> Cr-Commit-Position: refs/heads/master@{#71765} Refs: v8/v8@dfcdf78 PR-URL: nodejs#36573 Fixes: nodejs#36619 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com> Reviewed-By: Jiawen Geng <technicalcute@gmail.com>

Original commit message: [coverage] fix greedy nullish coalescing The SourceRangeScope helper was consuming too many characters, instead explicitly create SourceRange, based on scanner position. Bug: v8:11231 Change-Id: I852d211227abacf867e8f1ab3e3ab06dbdba2a9b Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2576006 Reviewed-by: Toon Verwaest <verwaest@chromium.org> Commit-Queue: Toon Verwaest <verwaest@chromium.org> Cr-Commit-Position: refs/heads/master@{#71765} Refs: v8/v8@dfcdf78 PR-URL: #36573 Fixes: #36619 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com> Reviewed-By: Jiawen Geng <technicalcute@gmail.com>

Original commit message: [coverage] fix greedy nullish coalescing The SourceRangeScope helper was consuming too many characters, instead explicitly create SourceRange, based on scanner position. Bug: v8:11231 Change-Id: I852d211227abacf867e8f1ab3e3ab06dbdba2a9b Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2576006 Reviewed-by: Toon Verwaest <verwaest@chromium.org> Commit-Queue: Toon Verwaest <verwaest@chromium.org> Cr-Commit-Position: refs/heads/master@{#71765} Refs: v8/v8@dfcdf78 PR-URL: nodejs#36573 Fixes: nodejs#36619 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com> Reviewed-By: Jiawen Geng <technicalcute@gmail.com>

Original commit message: [coverage] fix greedy nullish coalescing The SourceRangeScope helper was consuming too many characters, instead explicitly create SourceRange, based on scanner position. Bug: v8:11231 Change-Id: I852d211227abacf867e8f1ab3e3ab06dbdba2a9b Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2576006 Reviewed-by: Toon Verwaest <verwaest@chromium.org> Commit-Queue: Toon Verwaest <verwaest@chromium.org> Cr-Commit-Position: refs/heads/master@{#71765} Refs: v8/v8@dfcdf78 PR-URL: #36573 Fixes: #36619 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com> Reviewed-By: Jiawen Geng <technicalcute@gmail.com>

nodejs-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, 2020

bcoe requested a review from Trott December 18, 2020 22:50

targos approved these changes Dec 19, 2020

View reviewed changes

targos added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 19, 2020

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 19, 2020

Trott approved these changes Dec 21, 2020

View reviewed changes

mhdawson approved these changes Dec 21, 2020

View reviewed changes

richardlau mentioned this pull request Dec 21, 2020

tools: fix make-v8.sh #36594

Closed

2 tasks

bcoe force-pushed the fix-nullish branch from 9fe8590 to fba1e5f Compare December 22, 2020 03:09

gengjiawen approved these changes Dec 24, 2020

View reviewed changes

gengjiawen added the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 24, 2020

github-actions bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Dec 24, 2020

bcoe added commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Dec 24, 2020

github-actions bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 24, 2020

github-actions bot closed this Dec 24, 2020

danielleadams mentioned this pull request Jan 12, 2021

v15.6.0 proposal #36889

Merged

targos mentioned this pull request Jun 6, 2021

v14.17.1 release proposal #38948

Merged

Uh oh!

Conversation

bcoe commented Dec 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Related Issues

Checklist

Uh oh!

targos commented Dec 19, 2020

Uh oh!

nodejs-github-bot commented Dec 19, 2020

Uh oh!

bcoe commented Dec 20, 2020

Uh oh!

targos commented Dec 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mhdawson left a comment

Choose a reason for hiding this comment

Uh oh!

richardlau commented Dec 21, 2020

Uh oh!

richardlau commented Dec 21, 2020

Uh oh!

nodejs-github-bot commented Dec 22, 2020 • edited by bcoe Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Dec 22, 2020

Uh oh!

github-actions bot commented Dec 24, 2020

PR-URL: #36573 Fixes: https://github.com/https://github.com/nodejs/node/issues/<issue_number> Reviewed-By: Michaël Zasso targos@protonmail.com Reviewed-By: Rich Trott rtrott@gmail.com Reviewed-By: Michael Dawson midawson@redhat.com Reviewed-By: Jiawen Geng technicalcute@gmail.com

Uh oh!

gengjiawen commented Dec 24, 2020

Uh oh!

github-actions bot commented Dec 24, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

bcoe commented Dec 18, 2020 •

edited

Loading

targos commented Dec 21, 2020 •

edited

Loading

nodejs-github-bot commented Dec 22, 2020 •

edited by bcoe

Loading

PR-URL: #36573
Fixes: https://github.com/https://github.com/nodejs/node/issues/<issue_number>
Reviewed-By: Michaël Zasso targos@protonmail.com
Reviewed-By: Rich Trott rtrott@gmail.com
Reviewed-By: Michael Dawson midawson@redhat.com
Reviewed-By: Jiawen Geng technicalcute@gmail.com