deps: update V8 to 6.2 by targos · Pull Request #15362 · nodejs/node · GitHub

I'm opening this early to track the issues that need fixing.

Blocking issues

Compilation failure with Clang (macOS, FreeBSD)

Error

In file included from ../deps/v8/src/compiler/common-operator.cc:5:
In file included from ../deps/v8/src/compiler/common-operator.h:8:
In file included from ../deps/v8/src/assembler.h:38:
In file included from /Library/Developer/CommandLineTools/usr/bin/../include/c++/v1/forward_list:173:
In file included from /Library/Developer/CommandLineTools/usr/bin/../include/c++/v1/memory:602:
/Library/Developer/CommandLineTools/usr/bin/../include/c++/v1/utility:318:37: error: no type named 'type' in 'std::__1::enable_if<false, void>'; 'enable_if' cannot be used to disable this declaration
                 typename enable_if<is_convertible<_U1, _T1>::value &&
                                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../deps/v8/src/compiler/common-operator.h:137:62: note: in instantiation of member function 'std::__1::pair<unsigned int, const v8::internal::ZoneVector<v8::internal::MachineType> *>::pair' requested here
  using std::pair<uint32_t, const ZoneVector<MachineType>*>::pair;
                                                             ^
../deps/v8/src/base/functional.h:143:69: note: while substituting deduced template arguments into function template 'TypedObjectStateInfo' [with _U1 = unsigned int, _U2 = int]
  V8_INLINE size_t operator()(T const& v) const { return hash_value(v); }
                                                                    ^
../deps/v8/src/compiler/operator.h:188:47: note: in instantiation of member function 'v8::base::hash<v8::internal::compiler::ObjectStateInfo>::operator()' requested here
    return base::hash_combine(this->opcode(), this->hash_(this->parameter()));
                                              ^
../deps/v8/src/compiler/operator.h:169:3: note: in instantiation of member function 'v8::internal::compiler::Operator1<v8::internal::compiler::ObjectStateInfo, v8::internal::compiler::OpEqualTo<v8::internal::compiler::ObjectStateInfo>, v8::internal::compiler::OpHash<v8::internal::compiler::ObjectStateInfo> >::HashCode' requested here
  Operator1(Opcode opcode, Properties properties, const char* mnemonic,
  ^
../deps/v8/src/compiler/common-operator.cc:1260:23: note: in instantiation of member function 'v8::internal::compiler::Operator1<v8::internal::compiler::ObjectStateInfo, v8::internal::compiler::OpEqualTo<v8::internal::compiler::ObjectStateInfo>, v8::internal::compiler::OpHash<v8::internal::compiler::ObjectStateInfo> >::Operator1' requested here
  return new (zone()) Operator1<ObjectStateInfo>(  // --
                      ^

Windows test failure

https://ci.nodejs.org/job/node-test-binary-windows/11061/RUN_SUBSET=0,VS_VERSION=vs2015,label=win2008r2/console

This one is because we do not support building addons with VS2013 anymore. We need to stop testing this on Node >=9.0.0.

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

Greate！

Will it be semver-major or landed on Node.js 8 LTS?

Semver-major

I updated the OP with two main issues.

/cc @nodejs/build @nodejs/platform-macos @nodejs/platform-freebsd

FYI: I built this branch and ran the llnode test suite. All tests passed.

@targos I've gone ahead and put in request to get all floated patches upstreamed / landed on 6.2

https://chromium-review.googlesource.com/c/v8/v8/+/666925
https://chromium-review.googlesource.com/c/v8/v8/+/666961

https://bugs.chromium.org/p/v8/issues/detail?id=6824
https://bugs.chromium.org/p/v8/issues/detail?id=6825
https://bugs.chromium.org/p/v8/issues/detail?id=6826

One more CI job to see if the gclient thing is a real problem or not

https://ci.nodejs.org/job/node-test-commit-v8-linux/904/

Here is an ABI-Smoker job being run against this current PR... it should break. If it works... we have a problem

https://ci.nodejs.org/view/Node.js-citgm/job/citgm-abi-smoker/39/

This one is because we do not support building addons with VS2013 anymore. We need to stop testing this on Node >=9.0.0.

@targos do you think the problems on VS_VERSION=vs2015-x86,label=win2008r2 is a bug in the build script?

I'll update CI to not use VS2013 for Node 9.

@mhdawson any idea why ci is breaking during gsync?

Updated and removed the V8 backports. I think some of them still need to be merged to 6.2?

@targos Which ones? If you’re talking about the ones from #15391, yeah, as internal refactoring + an API change I don’t think they would be eligible for merging into 6.2. Feel free to ask for it if you think otherwise…

I'm talking about the ones linked in #15362 (comment)

@addaleax I cherry-picked 8403d6b#diff-ff06109b32824fc20fec697f584a42e3 to this PR. Your other cherry-pick is already in 6.2.

Does anyone have an idea about how we can fix the error with Clang?

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

Haven't seen clang errors yet, perhaps they'll surface in above jobs

You can see the error in the FreeBSD job. OSX is still pending.

CI changed to not run Node >=9 on VS2013. This fixed the Windows issue in this PR. Here is a Windows CI run: https://ci.nodejs.org/job/node-test-commit-windows-fanned/11804/

Refs: https://v8project.blogspot.com/2017/09/disabling-escape-analysis.html

Michal, the plan is to eventually backport to 8.x, but it may happen during LTS

…

On Oct 19, 2017 4:01 AM, "Michał Gołębiowski-Owczarek" < ***@***.***> wrote: Is it still planned to backport that to 8.x before it reaches LTS? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#15362 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAecV1TrAVranCOGdoKpZD-FBLI1XLj0ks5stwHwgaJpZM4PUfXy> .

Some of the addons/stringbytes-external-exceed-max tests are known flaky after the upgrade to V8 6.2. The most likely reason is the increase of the maximum string length from 256 to 1024 MB, pushing their execution times over the time limit. Fixes: nodejs#16354 Refs: nodejs#15362

PR-URL: #16219 Refs: #15362 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

PR-URL: nodejs/node#15362 Reviewed-By: Myles Borins <myles.borins@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

Major V8 updates are usually API/ABI incompatible with previous versions. This commit adapts NODE_MODULE_VERSION for V8 6.2. Refs: https://github.com/nodejs/CTC/blob/master/meetings/2016-09-28.md PR-URL: nodejs/node#15362 Reviewed-By: Myles Borins <myles.borins@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

With V8 6.2 there is one line less in the promise trace. PR-URL: nodejs/node#15362 Reviewed-By: Myles Borins <myles.borins@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

PR-URL: nodejs/node#15362 Reviewed-By: Myles Borins <myles.borins@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

Original commit message: avoid constructor inheritance due to compilation issues Constructor inheritance of a templated constructor is causing compilation issues for node.js: https: //github.com/nodejs/node/pull/15362#issue-257007421 Change-Id: I7d099ff5a1a2fd5b19c11112ddef8fe824e509f7 Reviewed-on: https://chromium-review.googlesource.com/707008 Commit-Queue: Tobias Tebbi <tebbi@chromium.org> Commit-Queue: Benedikt Meurer <bmeurer@chromium.org> Reviewed-by: Benedikt Meurer <bmeurer@chromium.org> Cr-Commit-Position: refs/heads/master@{#48445} Refs: v8/v8@0f1dfae PR-URL: nodejs/node#15362 Reviewed-By: Myles Borins <myles.borins@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

PR-URL: nodejs/node#16219 Refs: nodejs/node#15362 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

nodejs-github-bot added tools Issues and PRs related to the tools directory. v8 engine Issues and PRs related to the V8 dependency. labels Sep 12, 2017

targos added wip Issues and PRs that are still a work in progress. and removed tools Issues and PRs related to the tools directory. labels Sep 12, 2017

MylesBorins added the semver-major PRs that contain breaking changes and should be released in the next major version. label Sep 12, 2017

targos force-pushed the v8-6.2 branch from 067fab1 to ec16c50 Compare September 13, 2017 22:53

joaocgreis mentioned this pull request Sep 14, 2017

Tracking issue: Change 4 Windows 2008 machines to Visual Studio 2017 nodejs/build#883

Closed

vsemozhetbyt mentioned this pull request Sep 18, 2017

[v8.x] deps: backport V8 update to 6.1 #15393

Closed

2 tasks

targos force-pushed the v8-6.2 branch from ec16c50 to a5054d9 Compare September 18, 2017 12:09

bnoordhuis mentioned this pull request Oct 21, 2017

test: mark big string tests flaky on freebsd #16358

Closed

targos deleted the v8-6.2 branch October 23, 2017 12:43

ignisf mentioned this pull request Oct 27, 2017

v8 6.2 fails to compile on macOS rubyjs/libv8#249

Closed

jasnell mentioned this pull request Oct 30, 2017

Proposal: 2017-10-31, Version 9.0.0 (Current) #15135

Merged

deps: update V8 to 6.2 by targos · Pull Request #15362 · nodejs/node · GitHub

Uh oh!

Uh oh!

deps: update V8 to 6.2 - #15362

deps: update V8 to 6.2#15362targos wants to merge 9 commits intonodejs:masternodejs/node:masterfrom targos:v8-6.2targos/node:v8-6.2Copy head branch name to clipboard

Conversation

targos commented Sep 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Blocking issues

Compilation failure with Clang (macOS, FreeBSD)

Windows test failure

Uh oh!

targos commented Sep 12, 2017

Uh oh!

SteveCruise commented Sep 12, 2017

Uh oh!

vsemozhetbyt commented Sep 12, 2017

Uh oh!

targos commented Sep 12, 2017

Uh oh!

targos commented Sep 13, 2017

Uh oh!

cjihrig commented Sep 14, 2017

Uh oh!

MylesBorins commented Sep 14, 2017

Uh oh!

MylesBorins commented Sep 14, 2017

Uh oh!

MylesBorins commented Sep 14, 2017

Uh oh!

refack commented Sep 14, 2017

Uh oh!

joaocgreis commented Sep 14, 2017

Uh oh!

MylesBorins commented Sep 14, 2017

Uh oh!

targos commented Sep 18, 2017

Uh oh!

addaleax commented Sep 18, 2017

Uh oh!

targos commented Sep 18, 2017

Uh oh!

targos commented Sep 19, 2017

Uh oh!

targos commented Sep 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MylesBorins commented Sep 19, 2017

Uh oh!

targos commented Sep 19, 2017

Uh oh!

joaocgreis commented Sep 20, 2017

Uh oh!

vsemozhetbyt commented Sep 22, 2017

Uh oh!

MylesBorins commented Oct 19, 2017 via email

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

20 participants

deps: update V8 to 6.2#15362
targos wants to merge 9 commits into
nodejs:masterfrom
targos:v8-6.2

targos commented Sep 12, 2017 •
edited
Loading

targos commented Sep 19, 2017 •
edited
Loading