Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

deps: update V8 to 6.0 #14004

Closed
wants to merge 8 commits into from

Conversation

@targos
Copy link
Member

commented Jun 30, 2017

As discussed in nodejs/CTC#146, I am opening this PR early so we can have test builds of Node with V8 6.0 as soon as possible.
The update is ready but blocked on five issues:

/cc @nodejs/v8 @nodejs/ctc

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

V8

@targos

This comment has been minimized.

@targos targos referenced this pull request Jun 30, 2017

Closed

V8 6.0 failures #7

@targos

This comment has been minimized.

Copy link
Member Author

commented Jun 30, 2017

I forgot there are two other issues (initially reported here: nodejs/node-v8#7):

And finally a V8 cctest fails: https://ci.nodejs.org/job/node-test-commit-v8-linux/763/

@misterdjules

This comment has been minimized.

Copy link
Contributor

commented Jun 30, 2017

@targos When do you need those SmartOS build errors fixed? In other words, is it urgent?

@joaocgreis

This comment has been minimized.

Copy link
Member

commented Jun 30, 2017

@targos https://chromium-review.googlesource.com/c/556599/ landed upstream, it would be better to replace my floating patch (deps,win: increase msvs_shard in gyp for V8) with a backport of that commit.

@targos targos force-pushed the targos:v8-6.0 branch Jul 3, 2017

@targos

This comment has been minimized.

Copy link
Member Author

commented Jul 3, 2017

@joaocgreis Thank you. I cherry-picked that commit.

@targos

This comment has been minimized.

Copy link
Member Author

commented Jul 3, 2017

@misterdjules No, this is not urgent, and we have other issues to fix in the meantime

@targos targos force-pushed the targos:v8-6.0 branch Jul 5, 2017

@bmeurer

This comment has been minimized.

Copy link
Member

commented Jul 6, 2017

@targos targos force-pushed the targos:v8-6.0 branch to 42203ed Jul 10, 2017

@natorion

This comment has been minimized.

Copy link

commented Jul 11, 2017

Some comments/questions:

Node crashes when running jest. See: #13804, https://bugs.chromium.org/p/v8/issues/detail?id=6490 and https://github.com/targos/jest-crash.
==> scheduled to be merged upstream to 6.0

async-hooks/test-zlib.zlib-binding.deflate can crash (seems to happen only with the 32-bit build): https://ci.nodejs.org/job/node-test-commit-linux/10874/
==> Is there already a V8 issue? That sounds like something that might be fixed upstream. Sadly, I can't access ci.nodejs.org

V8 cctest failure: https://ci.nodejs.org/job/node-test-commit-v8-linux/763/
==> Same as above, it is hard to judge what needs to be done.

Compilation errors on SmartOS and FreeBSD =>
Are you going to float those patches? Are you expecting that they are going to be upstreamed? Please keep in mind that we will promote 6.0 to V8 stable next week. That means, that merges will have a much higher burden to overcome. Depending on the actual patch it might not be an issue though because of SmartOS and FreeBSD.

@Trott

This comment has been minimized.

Copy link
Member

commented Jul 11, 2017

Sadly, I can't access ci.nodejs.org

@natorion It should be available again in a few hours, if all goes well.

Access is extremely restricted right now because it's being used for the scheduled security release of Node.js. Any changes to that schedule are likely to be posted to #14153. If necessary to know more, @MylesBorins can fill you in.

@gibfahn

This comment has been minimized.

Copy link
Member

commented Jul 11, 2017

@natorion It should be available again in a few hours, if all goes well.

It's back btw.

@targos

This comment has been minimized.

Copy link
Member Author

commented Jul 11, 2017

@natorion

This comment has been minimized.

Copy link

commented Jul 12, 2017

Thanks, seems like https://ci.nodejs.org/job/node-test-pull-request/9075/ was aborted and the other links produce 404s.

@MylesBorins

This comment has been minimized.

Copy link
Member

commented Jul 12, 2017

@natorion it would appear that the test runner was hanging on centos 5

https://ci.nodejs.org/job/node-test-commit-linux/11105/nodes=centos5-64/console

@targos targos force-pushed the targos:v8-6.0 branch from 42203ed to 4f744d8 Jul 12, 2017

@targos

This comment has been minimized.

Copy link
Member Author

commented Jul 12, 2017

@MylesBorins

This comment has been minimized.

Copy link
Member

commented Jul 12, 2017

@targos @addaleax any interest in floating the bsd / smart os packages upstream?

@addaleax

This comment has been minimized.

Copy link
Member

commented Jul 12, 2017

Sorry, I didn’t quite follow here – which are the patches you are referring to?

@MylesBorins

This comment has been minimized.

Copy link
Member

commented Jul 12, 2017

@addaleax patches that fix the compilation errors, Im not sure we have them yet...

was trying to summarize @natorion's questions in #14004 (comment)

@targos

This comment has been minimized.

Copy link
Member Author

commented Jul 12, 2017

We don't have those patches yet and I don't know if anyone is working on it.

@addaleax

This comment has been minimized.

Copy link
Member

commented Jul 12, 2017

async-hooks/test-zlib.zlib-binding.deflate can crash (seems to happen only with the 32-bit build): https://ci.nodejs.org/job/node-test-commit-linux/10874/

Do you have a CI log that contains that failure? Could it be the same as #14161?

@targos

This comment has been minimized.

Copy link
Member Author

commented Jul 12, 2017

Do you have a CI log that contains that failure?

No, the logs have been deleted. It does not seem to be reproducible anymore. I'll mark this point as done for now.

@MylesBorins MylesBorins closed this Aug 1, 2017

MylesBorins added a commit that referenced this pull request Aug 1, 2017

deps: update V8 to 6.0.286.52
PR-URL: #14004
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

MylesBorins added a commit that referenced this pull request Aug 1, 2017

src: update NODE_MODULE_VERSION to 57
Major V8 updates are usually API/ABI incompatible with previous
versions. This commit adapts NODE_MODULE_VERSION for V8 6.0.

Refs: https://github.com/nodejs/CTC/blob/master/meetings/2016-09-28.md

PR-URL: #14004
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

MylesBorins added a commit that referenced this pull request Aug 1, 2017

deps: limit regress/regress-crbug-514081 v8 test
regress/regress-crbug-514081 allocates a 2G block of memory
and if there  are multiple variants running at the
same time this can lead to crashes, OOM kills or
the OS failing to allocate memory.  This patch
limits us to running a single variant of the test

Fixes: #6340
Refs: #6678

PR-URL: #14004
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

MylesBorins added a commit that referenced this pull request Aug 1, 2017

v8: fix stack overflow in recursive method
HGlobalValueNumberingPhase::CollectSideEffectsOnPathsToDominatedBlock()
used to self-recurse before this commit, causing stack overflows on
systems with small stack sizes.  Make it non-recursive by storing
intermediate results in a heap-allocated list.

Fixes: #11991
Refs: #12460

PR-URL: #14004
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

MylesBorins added a commit that referenced this pull request Aug 1, 2017

deps: fix addons compilation with VS2013
VS2013 does not support defaulting move constructor and assignment
operator. This adds explicit definitions of those methods for two
classes.
This fix is required because we still support building addons with
VS2013 and the incompatibility is in v8.h.

Fixes: nodejs/node-v8#4
Refs: #13263

PR-URL: #14004
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

MylesBorins added a commit that referenced this pull request Aug 1, 2017

deps: backport c0f1ff2 from upstream V8
Original commit message:

    Fix GCC 7 build errors

    BUG=chromium:691681
    R=franzih@chromium.org

    Change-Id: Id7e5698487f16dc217a804f6d3f24da7213c72b9
    Reviewed-on: https://chromium-review.googlesource.com/530227
    Commit-Queue: Toon Verwaest <verwaest@chromium.org>
    Reviewed-by: Toon Verwaest <verwaest@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#46045}

Refs: #13517
Fixes: #10388
Refs: #12392

PR-URL: #14004
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

MylesBorins added a commit that referenced this pull request Aug 1, 2017

src: fix new V8 compiler warnings
PR-URL: #14004
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

MylesBorins added a commit that referenced this pull request Aug 1, 2017

deps: backport rehash strings after deserialization
Original commit messages:
v8/v8@a2ab135
  [snapshot] Rehash strings after deserialization.

  See https://goo.gl/6aN8xA

  Bug: v8:6593
  Change-Id: Ic8b0b57195d01d41591397d5d45de3f0f3ebc3d9
  Reviewed-on: https://chromium-review.googlesource.com/574527
  Reviewed-by: Camillo Bruni <cbruni@chromium.org>
  Reviewed-by: Jakob Gruber <jgruber@chromium.org>
  Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
  Commit-Queue: Yang Guo <yangguo@chromium.org>
  Cr-Commit-Position: refs/heads/master@{#46732}

v8/v8@182caaf
  Do not track transitions for built-in objects.

  Objects created during bootstrapping do not need
  a transition tree except for elements kind transitions.

  Bug: v8:6596
  Change-Id: I237b8b2792f201336e1c9731c815095dd06bc182
  Reviewed-on: https://chromium-review.googlesource.com/571750
  Reviewed-by: Igor Sheludko <ishell@chromium.org>
  Commit-Queue: Yang Guo <yangguo@chromium.org>
  Cr-Commit-Position: refs/heads/master@{#46693}

Fixes: #14171
Refs: #14345

PR-URL: #14004
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

addaleax added a commit that referenced this pull request Aug 2, 2017

deps: limit regress/regress-crbug-514081 v8 test
regress/regress-crbug-514081 allocates a 2G block of memory
and if there  are multiple variants running at the
same time this can lead to crashes, OOM kills or
the OS failing to allocate memory.  This patch
limits us to running a single variant of the test

Fixes: #6340
Refs: #6678

Backport-PR-URL: #14574
Backport-Reviewed-By: Anna Henningsen <anna@addaleax.net>
Backport-Reviewed-By: Refael Ackermann <refack@gmail.com>

PR-URL: #14004
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

addaleax added a commit that referenced this pull request Aug 2, 2017

v8: fix stack overflow in recursive method
HGlobalValueNumberingPhase::CollectSideEffectsOnPathsToDominatedBlock()
used to self-recurse before this commit, causing stack overflows on
systems with small stack sizes.  Make it non-recursive by storing
intermediate results in a heap-allocated list.

Fixes: #11991
Refs: #12460

Backport-PR-URL: #14574
Backport-Reviewed-By: Anna Henningsen <anna@addaleax.net>
Backport-Reviewed-By: Refael Ackermann <refack@gmail.com>

PR-URL: #14004
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

addaleax added a commit that referenced this pull request Aug 2, 2017

deps: fix addons compilation with VS2013
VS2013 does not support defaulting move constructor and assignment
operator. This adds explicit definitions of those methods for two
classes.
This fix is required because we still support building addons with
VS2013 and the incompatibility is in v8.h.

Fixes: nodejs/node-v8#4
Refs: #13263

Backport-PR-URL: #14574
Backport-Reviewed-By: Anna Henningsen <anna@addaleax.net>
Backport-Reviewed-By: Refael Ackermann <refack@gmail.com>

PR-URL: #14004
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

addaleax added a commit that referenced this pull request Aug 2, 2017

deps: backport c0f1ff2 from upstream V8
Original commit message:

    Fix GCC 7 build errors

    BUG=chromium:691681
    R=franzih@chromium.org

    Change-Id: Id7e5698487f16dc217a804f6d3f24da7213c72b9
    Reviewed-on: https://chromium-review.googlesource.com/530227
    Commit-Queue: Toon Verwaest <verwaest@chromium.org>
    Reviewed-by: Toon Verwaest <verwaest@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#46045}

Refs: #13517
Fixes: #10388
Refs: #12392

Backport-PR-URL: #14574
Backport-Reviewed-By: Anna Henningsen <anna@addaleax.net>
Backport-Reviewed-By: Refael Ackermann <refack@gmail.com>

PR-URL: #14004
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

addaleax added a commit that referenced this pull request Aug 2, 2017

src: fix new V8 compiler warnings
Backport-PR-URL: #14574
Backport-Reviewed-By: Anna Henningsen <anna@addaleax.net>
Backport-Reviewed-By: Refael Ackermann <refack@gmail.com>

PR-URL: #14004
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

addaleax added a commit that referenced this pull request Aug 2, 2017

deps: backport rehash strings after deserialization
Original commit messages:
v8/v8@a2ab135
  [snapshot] Rehash strings after deserialization.

  See https://goo.gl/6aN8xA

  Bug: v8:6593
  Change-Id: Ic8b0b57195d01d41591397d5d45de3f0f3ebc3d9
  Reviewed-on: https://chromium-review.googlesource.com/574527
  Reviewed-by: Camillo Bruni <cbruni@chromium.org>
  Reviewed-by: Jakob Gruber <jgruber@chromium.org>
  Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
  Commit-Queue: Yang Guo <yangguo@chromium.org>
  Cr-Commit-Position: refs/heads/master@{#46732}

v8/v8@182caaf
  Do not track transitions for built-in objects.

  Objects created during bootstrapping do not need
  a transition tree except for elements kind transitions.

  Bug: v8:6596
  Change-Id: I237b8b2792f201336e1c9731c815095dd06bc182
  Reviewed-on: https://chromium-review.googlesource.com/571750
  Reviewed-by: Igor Sheludko <ishell@chromium.org>
  Commit-Queue: Yang Guo <yangguo@chromium.org>
  Cr-Commit-Position: refs/heads/master@{#46693}

Fixes: #14171
Refs: #14345

Backport-PR-URL: #14574
Backport-Reviewed-By: Anna Henningsen <anna@addaleax.net>
Backport-Reviewed-By: Refael Ackermann <refack@gmail.com>

PR-URL: #14004
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

@addaleax addaleax referenced this pull request Aug 2, 2017

Merged

v8.3.0 proposal #14594

@targos targos deleted the targos:v8-6.0 branch Aug 10, 2017

MylesBorins added a commit to MylesBorins/node that referenced this pull request Oct 2, 2017

build: run test-hash-seed at the end of test-v8
The v8 and test-hash-seed targets cannot be run in parallel because they
need different copies of the deps/v8 directory.

Ref: nodejs#14004 (comment)
PR-URL: nodejs#14219
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>

MylesBorins added a commit that referenced this pull request Oct 18, 2017

build: run test-hash-seed at the end of test-v8
The v8 and test-hash-seed targets cannot be run in parallel because they
need different copies of the deps/v8 directory.

Ref: #14004 (comment)
Backport-PR-URL: #15562
PR-URL: #14219
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>

MylesBorins added a commit that referenced this pull request Oct 25, 2017

build: run test-hash-seed at the end of test-v8
The v8 and test-hash-seed targets cannot be run in parallel because they
need different copies of the deps/v8 directory.

Ref: #14004 (comment)
Backport-PR-URL: #15562
PR-URL: #14219
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.