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 5.9 #13263

Closed
wants to merge 10 commits into from
Closed

deps: update V8 to 5.9 #13263

wants to merge 10 commits into from

Conversation

targos
Copy link
Member

@targos targos commented May 28, 2017

I kept out v8: fix stack overflow in recursive method because Crankshaft is disabled in 5.9.

/cc @nodejs/v8

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

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

@nodejs-github-bot nodejs-github-bot added the v8 engine Issues and PRs related to the V8 dependency. label May 28, 2017
@targos
Copy link
Member Author

targos commented May 28, 2017

There is one failing V8 test: https://ci.nodejs.org/job/node-test-commit-v8-linux/720/

@jasnell
Copy link
Member

jasnell commented May 28, 2017

In order to make things easiest, please don't land this until the 8.0.0 release is out.

@targos
Copy link
Member Author

targos commented May 29, 2017

I cannot reproduce the V8 test failure on Fedora 25.

@targos
Copy link
Member Author

targos commented May 30, 2017

Added 22b3ad952d4c0e96610ae6604febf5be5d372790 to fix the Windows issue.
New CI: https://ci.nodejs.org/job/node-test-commit/10238/

@targos
Copy link
Member Author

targos commented May 31, 2017

Another V8 CI run: https://ci.nodejs.org/job/node-test-commit-v8-linux/723/
The test seems to be flaky.

Copy link
Member

@gibfahn gibfahn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM on the basis that node CI is green.

@gibfahn
Copy link
Member

gibfahn commented May 31, 2017

v8tests.mjsunit/es6/typedarray-construct-offset-not-smi certainly seems flaky, do you know if there's an open issue for that?

@jbajwa
Copy link
Contributor

jbajwa commented May 31, 2017

Looks like the issue is similar to #6678, where OOM killer kicks in and kills the process.

[7242863.995926] [34119]  1004 34119   671796   449914    1796        0             0 d8
[7242863.995927] Out of memory: Kill process 34119 (d8) score 224 or sacrifice child
[7242863.995929] Killed process 34119 (d8) total-vm:2687184kB, anon-rss:1799632kB, file-rss:24kB

in typedarray-construct-offset-not-smi.js there is an allocation of 2G block of memory

var non_smi_byte_length = %_MaxSmi() + 11; // <<-- 2147483658
  try {
    var buffer = new ArrayBuffer(non_smi_byte_length); 
  } catch (e) {

Following the same approach as #6678, following patch should fix this flaky behavior

diff --git a/deps/v8/test/mjsunit/mjsunit.status b/deps/v8/test/mjsunit/mjsunit.status
index 66ecfcf..c8c3c4e 100644
--- a/deps/v8/test/mjsunit/mjsunit.status
+++ b/deps/v8/test/mjsunit/mjsunit.status
@@ -148,7 +148,7 @@
   # Slow tests.
   'copy-on-write-assert': [PASS, SLOW],
   'es6/tail-call-megatest*': [PASS, SLOW, FAST_VARIANTS, ['tsan', SKIP]],
-  'es6/typedarray-construct-offset-not-smi': [PASS, SLOW],
+  'es6/typedarray-construct-offset-not-smi': [PASS, SLOW, NO_VARIANTS],
   'harmony/regexp-property-script-extensions': [PASS, SLOW],
   'numops-fuzz-part*': [PASS, ['mode == debug', SLOW]],
   'readonly': [PASS, SLOW],

@bnoordhuis
Copy link
Member

I kept out v8: fix stack overflow in recursive method because Crankshaft is disabled in 5.9.

Disabled by default, not disabled altogether. Functions that don't pass --turbo_filter=... are still directed to crankshaft. If the patch is easy to forward-port, I'd keep it for now.

Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The VS 2013 support patch can be dropped and wasn't the plan to keep 5.9 ABI and API compatible with 5.8? In that case bumping NODE_MODULE_VERSION isn't necessary.

(I realize the pull request in its current shape breaks API/ABI but I'm wondering what the plan is.)

@addaleax
Copy link
Member

@bnoordhuis I think the plan is to have clean V8 versions on master and apply the compatibility fixes only on v8.x(-staging), so this should be fine

@targos
Copy link
Member Author

targos commented Jun 1, 2017

If the patch is easy to forward-port, I'd keep it for now.

OK, I'll add it back.

The VS 2013 support patch can be dropped

Do you mean we don't support VS 2013 anymore? Then the CI jobs must change.

...wasn't the plan to keep 5.9 ABI and API compatible with 5.8?

What @addaleax said.

@bnoordhuis
Copy link
Member

Do you mean we don't support VS 2013 anymore? Then the CI jobs must change.

The vs2013 buildbot is only used for the v4.x branch at this point, I think, and possibly addon tests in v6.x.

Copy link
Member

@fhinkel fhinkel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@targos
Copy link
Member Author

targos commented Jun 1, 2017

@bnoordhuis see nodejs/node-v8#4 (comment)

It looks like addon tests are compiled with vs2013 for all branches.

/cc @joaocgreis

@joaocgreis
Copy link
Member

We still support building native addons with VS2013. The CI machines with Windows 2008 all have VS2013 to make sure the addons test suite is run with VS2013.

@psmarshall
Copy link
Contributor

That test allocating 2GB of memory is a bit extreme, we'll look into it on the v8 side for the future: (v8 bug)

@bnoordhuis
Copy link
Member

It looks like addon tests are compiled with vs2013 for all branches.

Okay, but that means the V8 patch can be dropped. It's many things but an add-on it is not. LGTM apart from that.

@targos
Copy link
Member Author

targos commented Jun 1, 2017

But without the patch, win2008r2 tests are red: https://ci.nodejs.org/job/node-test-binary-windows/8831/

@bnoordhuis
Copy link
Member

Ah, I somehow had the idea that the patch only touched files in deps/v8/src. Okay, objection withdrawn.

@bnoordhuis
Copy link
Member

Raised #13372 to discuss dropping support for VS 2013 in node 9.

Copy link
Contributor

@MylesBorins MylesBorins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should only land this if it has the ABI compat patches for 6.0

@jasnell
Copy link
Member

jasnell commented Jun 5, 2017

Agreed. Landing this without the ABI compat patches would be a regression

@addaleax
Copy link
Member

addaleax commented Jun 5, 2017

@MylesBorins Even on master? We didn’t do that for V8 5.8

nodejs-ci pushed a commit to nodejs/node-v8 that referenced this pull request Aug 31, 2017
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: #4

PR-URL: nodejs/node#13263
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
nodejs-ci pushed a commit to nodejs/node-v8 that referenced this pull request Sep 1, 2017
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: #4

PR-URL: nodejs/node#13263
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
nodejs-ci pushed a commit to nodejs/node-v8 that referenced this pull request Sep 2, 2017
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: #4

PR-URL: nodejs/node#13263
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
nodejs-ci pushed a commit to nodejs/node-v8 that referenced this pull request Sep 3, 2017
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: #4

PR-URL: nodejs/node#13263
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
nodejs-ci pushed a commit to nodejs/node-v8 that referenced this pull request Sep 4, 2017
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: #4

PR-URL: nodejs/node#13263
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
nodejs-ci pushed a commit to nodejs/node-v8 that referenced this pull request Sep 5, 2017
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: #4

PR-URL: nodejs/node#13263
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
nodejs-ci pushed a commit to nodejs/node-v8 that referenced this pull request Sep 6, 2017
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: #4

PR-URL: nodejs/node#13263
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
nodejs-ci pushed a commit to nodejs/node-v8 that referenced this pull request Sep 6, 2017
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: #4

PR-URL: nodejs/node#13263
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
nodejs-ci pushed a commit to nodejs/node-v8 that referenced this pull request Sep 7, 2017
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: #4

PR-URL: nodejs/node#13263
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
nodejs-ci pushed a commit to nodejs/node-v8 that referenced this pull request Sep 8, 2017
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: #4

PR-URL: nodejs/node#13263
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
nodejs-ci pushed a commit to nodejs/node-v8 that referenced this pull request Sep 9, 2017
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: #4

PR-URL: nodejs/node#13263
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
nodejs-ci pushed a commit to nodejs/node-v8 that referenced this pull request Sep 10, 2017
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: #4

PR-URL: nodejs/node#13263
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
nodejs-ci pushed a commit to nodejs/node-v8 that referenced this pull request Sep 11, 2017
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: #4

PR-URL: nodejs/node#13263
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
nodejs-ci pushed a commit to nodejs/node-v8 that referenced this pull request Sep 12, 2017
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: #4

PR-URL: nodejs/node#13263
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
targos pushed a commit to targos/node that referenced this pull request Sep 12, 2017
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

PR-URL: nodejs#13263
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
nodejs-ci pushed a commit to nodejs/node-v8 that referenced this pull request Sep 13, 2017
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: #4

PR-URL: nodejs/node#13263
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
addaleax pushed a commit that referenced this pull request Sep 13, 2017
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

PR-URL: #13263
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
addaleax pushed a commit to addaleax/node that referenced this pull request Sep 13, 2017
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

PR-URL: nodejs#13263
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
nodejs-ci pushed a commit to nodejs/node-v8 that referenced this pull request Sep 14, 2017
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: #4

PR-URL: nodejs/node#13263
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
targos pushed a commit to targos/node that referenced this pull request Sep 14, 2017
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

PR-URL: nodejs#13263
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
targos pushed a commit to targos/node that referenced this pull request Sep 21, 2017
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

PR-URL: nodejs#13263
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
MylesBorins pushed a commit that referenced this pull request Sep 28, 2017
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

Backport-PR-URL: #15393
PR-URL: #13263
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
MylesBorins pushed a commit that referenced this pull request Sep 29, 2017
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

Backport-PR-URL: #15393
PR-URL: #13263
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Oct 3, 2017
MylesBorins pushed a commit that referenced this pull request Oct 3, 2017
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

Backport-PR-URL: #15393
PR-URL: #13263
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-major PRs that contain breaking changes and should be released in the next major version. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

Successfully merging this pull request may close these issues.