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.5 #9618

Closed
wants to merge 6 commits into
base: master
from

Conversation

Projects
None yet
@targos
Member

targos commented Nov 15, 2016

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

V8

Description of change

This PR updates V8 to the current 5.5-lkgr branch.

/cc @nodejs/v8

Previous discussion: nodejs/v8#2

CI: https://ci.nodejs.org/job/node-test-pull-request/4849/
V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/417/
V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/422/

@Fishrock123

This comment has been minimized.

Show comment
Hide comment
@Fishrock123

Fishrock123 Nov 15, 2016

Member

As brought up in nodejs/v8#2 I'm quite skeptical this is a good idea until we get better hooks for native promises. (Which seemed to be the plan to get in before node version 8, where this was originally supposed to land.)

Member

Fishrock123 commented Nov 15, 2016

As brought up in nodejs/v8#2 I'm quite skeptical this is a good idea until we get better hooks for native promises. (Which seemed to be the plan to get in before node version 8, where this was originally supposed to land.)

@mhdawson

This comment has been minimized.

Show comment
Hide comment
@mhdawson
Member

mhdawson commented Nov 15, 2016

CI run to validate across platforms: https://ci.nodejs.org/job/node-test-commit-v8-linux/421/

@ilkkao ilkkao referenced this pull request Nov 15, 2016

Closed

Koa 2.0.0 #533

@bnoordhuis

This comment has been minimized.

Show comment
Hide comment
@bnoordhuis

bnoordhuis Nov 15, 2016

Member

As brought up in nodejs/v8#2 I'm quite skeptical this is a good idea until we get better hooks for native promises.

That's a pretty cryptic comment on its own. I think the missing context is that 5.5 ships async/await without a flag?

Member

bnoordhuis commented Nov 15, 2016

As brought up in nodejs/v8#2 I'm quite skeptical this is a good idea until we get better hooks for native promises.

That's a pretty cryptic comment on its own. I think the missing context is that 5.5 ships async/await without a flag?

@mhdawson

This comment has been minimized.

Show comment
Hide comment
@mhdawson

mhdawson Nov 17, 2016

Member

Looks like CI failures across the board.

Member

mhdawson commented Nov 17, 2016

Looks like CI failures across the board.

@jbergstroem

This comment has been minimized.

Show comment
Hide comment
@jbergstroem

jbergstroem Nov 17, 2016

Member

Afaik smartos14 support is dropped for 5.5

Member

jbergstroem commented Nov 17, 2016

Afaik smartos14 support is dropped for 5.5

@targos

This comment has been minimized.

Show comment
Hide comment
@targos

targos Nov 26, 2016

Member

Updated.

There are some new utf-8 fixes (1, 2) that make test/parallel/test-string-decoder fail. I'm sure that someone already mentioned it but I can't find the related issue.

Example failure:

test('utf-8', Buffer.from('F0B841', 'hex'), '\ufffd\ufffdA');
// AssertionError: Expected "\ufffd\ufffd\u41", but got "\ufffd\u41"

If I change the expected value, the error is inverted:

test('utf-8', Buffer.from('F0B841', 'hex'), '\ufffdA');
// AssertionError: Expected "\ufffd\u41", but got "\ufffd\ufffd\u41"
Member

targos commented Nov 26, 2016

Updated.

There are some new utf-8 fixes (1, 2) that make test/parallel/test-string-decoder fail. I'm sure that someone already mentioned it but I can't find the related issue.

Example failure:

test('utf-8', Buffer.from('F0B841', 'hex'), '\ufffd\ufffdA');
// AssertionError: Expected "\ufffd\ufffd\u41", but got "\ufffd\u41"

If I change the expected value, the error is inverted:

test('utf-8', Buffer.from('F0B841', 'hex'), '\ufffdA');
// AssertionError: Expected "\ufffd\u41", but got "\ufffd\ufffd\u41"
@ofrobots

This comment has been minimized.

Show comment
Hide comment
@ofrobots
Contributor

ofrobots commented Nov 26, 2016

@targos

This comment has been minimized.

Show comment
Hide comment
@targos

targos Dec 3, 2016

Member

I'm turning the PR into a clean semver-major that can land on master. I'll then open another one to backport to v7.x.

Member

targos commented Dec 3, 2016

I'm turning the PR into a clean semver-major that can land on master. I'll then open another one to backport to v7.x.

@targos

This comment has been minimized.

Show comment
Hide comment
@targos

targos Dec 3, 2016

Member

This is still in progress because of v8@af842a7#commitcomment-19855022

Member

targos commented Dec 3, 2016

This is still in progress because of v8@af842a7#commitcomment-19855022

@targos

This comment has been minimized.

Show comment
Hide comment
@targos

targos Dec 3, 2016

Member

@nodejs/platform-smartos What are we going to do with SmartOS 14? The CI cannot be green because of the incompatibility with this platform.

Member

targos commented Dec 3, 2016

@nodejs/platform-smartos What are we going to do with SmartOS 14? The CI cannot be green because of the incompatibility with this platform.

@jbergstroem

This comment has been minimized.

Show comment
Hide comment
@jbergstroem

jbergstroem Dec 4, 2016

Member

@targos as far as I know, it is to be dropped for 55 and forward. I can look at skipping testing against smartos14 for a specific node version; for instance 7.3?

Member

jbergstroem commented Dec 4, 2016

@targos as far as I know, it is to be dropped for 55 and forward. I can look at skipping testing against smartos14 for a specific node version; for instance 7.3?

@misterdjules

This comment has been minimized.

Show comment
Hide comment
@misterdjules

misterdjules Dec 8, 2016

@targos FWIW, what @jbergstroem mentioned:

as far as I know, it is to be dropped for 55 and forward.

sounds good to me.

P.S: sorry for the delay, I was on vacation until today.

misterdjules commented Dec 8, 2016

@targos FWIW, what @jbergstroem mentioned:

as far as I know, it is to be dropped for 55 and forward.

sounds good to me.

P.S: sorry for the delay, I was on vacation until today.

@pixelgrid

This comment has been minimized.

Show comment
Hide comment
@pixelgrid

pixelgrid Dec 19, 2016

Hi everyone,
sorry for the offtopic question, but when when its v8(with 5.5 v8) scheduled to be released?

pixelgrid commented Dec 19, 2016

Hi everyone,
sorry for the offtopic question, but when when its v8(with 5.5 v8) scheduled to be released?

@gibfahn

This comment has been minimized.

Show comment
Hide comment
@gibfahn

gibfahn Dec 19, 2016

Member

@pixelgrid See https://github.com/nodejs/LTS, Node.js v8 should be released in April 2017 (and will go LTS in October 2017).

Member

gibfahn commented Dec 19, 2016

@pixelgrid See https://github.com/nodejs/LTS, Node.js v8 should be released in April 2017 (and will go LTS in October 2017).

@pixelgrid

This comment has been minimized.

Show comment
Hide comment
@pixelgrid

pixelgrid Dec 19, 2016

@gibfahn so support for async/await from v8 5.5 is going to be cherry picked or is finally going to be set for deployment in april?

Is there any conserning performance of other issue that changed during 5.4 with a flag and 5.5 without one?

pixelgrid commented Dec 19, 2016

@gibfahn so support for async/await from v8 5.5 is going to be cherry picked or is finally going to be set for deployment in april?

Is there any conserning performance of other issue that changed during 5.4 with a flag and 5.5 without one?

@kyptov

This comment has been minimized.

Show comment
Hide comment

kyptov commented Dec 19, 2016

@pixelgrid #9339

@bricss

This comment has been minimized.

Show comment
Hide comment
@bricss

bricss Dec 20, 2016

Geez, I hope we would see backport of V8 5.5 in current Node.js branch in Q1 of 2017.

bricss commented Dec 20, 2016

Geez, I hope we would see backport of V8 5.5 in current Node.js branch in Q1 of 2017.

@jbergstroem

This comment has been minimized.

Show comment
Hide comment
@jbergstroem

jbergstroem Dec 20, 2016

Member

I was under the impression that ABI/API will not change. True or false?

Member

jbergstroem commented Dec 20, 2016

I was under the impression that ABI/API will not change. True or false?

@ofrobots

This comment has been minimized.

Show comment
Hide comment
@ofrobots

ofrobots Dec 22, 2016

Contributor

@targos trying to summarize current status. Let me know if anything is incorrect.

  • Deal with the string decoder tests that rely on undefined behaviour in the utf8 spec. I suggest that these tests be dropped. Tracking issue: #10300.
  • Drop SmartOS 14 testing in the CI for Node versions 8.0 onwards. @jbergstroem.

Others: let's keep the focus of this issue on landing V8 5.5 onto master. Whether or not this should be backported to the 7.x branch is a separate discussion out of scope from this issue.

Contributor

ofrobots commented Dec 22, 2016

@targos trying to summarize current status. Let me know if anything is incorrect.

  • Deal with the string decoder tests that rely on undefined behaviour in the utf8 spec. I suggest that these tests be dropped. Tracking issue: #10300.
  • Drop SmartOS 14 testing in the CI for Node versions 8.0 onwards. @jbergstroem.

Others: let's keep the focus of this issue on landing V8 5.5 onto master. Whether or not this should be backported to the 7.x branch is a separate discussion out of scope from this issue.

@targos

This comment has been minimized.

Show comment
Hide comment
@targos

targos Dec 23, 2016

Member

@ofrobots That is correct.

The string decoder thing is not just a test problem. There is an inconsistency between string_decoder and V8 and it has to be fixed. Tracking issue: #10300

Member

targos commented Dec 23, 2016

@ofrobots That is correct.

The string decoder thing is not just a test problem. There is an inconsistency between string_decoder and V8 and it has to be fixed. Tracking issue: #10300

@pannous pannous referenced this pull request Dec 23, 2016

Closed

WebAssembly #19

@targos

This comment has been minimized.

Show comment
Hide comment
@targos

targos Dec 30, 2016

Member

There is another question about CI support for Node 8+. I'd like to stop floating the deps: cherry-pick workaround for clang-3.4 ICE and I think there is a platform on CI that requires this patch.

Member

targos commented Dec 30, 2016

There is another question about CI support for Node 8+. I'd like to stop floating the deps: cherry-pick workaround for clang-3.4 ICE and I think there is a platform on CI that requires this patch.

@targos

This comment has been minimized.

Show comment
Hide comment
@targos

targos Dec 30, 2016

Member

Also, as @mscdex pointed out in #10300 (comment), there are now two segfaults when tests are run with make test. It does not happen if the tests are run individually.

Member

targos commented Dec 30, 2016

Also, as @mscdex pointed out in #10300 (comment), there are now two segfaults when tests are run with make test. It does not happen if the tests are run individually.

@MylesBorins

This comment has been minimized.

Show comment
Hide comment
@MylesBorins

MylesBorins Dec 30, 2016

Member

/cc @jbergstroem and @nodejs/build re: @targos' question above

Member

MylesBorins commented Dec 30, 2016

/cc @jbergstroem and @nodejs/build re: @targos' question above

@gibfahn

This comment has been minimized.

Show comment
Hide comment
@gibfahn

gibfahn Dec 30, 2016

Member

There is another question about CI support for Node 8+

I think this probably comes under #8922

Member

gibfahn commented Dec 30, 2016

There is another question about CI support for Node 8+

I think this probably comes under #8922

@targos

This comment has been minimized.

Show comment
Hide comment
@targos

targos Jan 1, 2017

Member

Actually the failing test is test/parallel/test-trace-event.js and it does fail when run individually with ./node.

Member

targos commented Jan 1, 2017

Actually the failing test is test/parallel/test-trace-event.js and it does fail when run individually with ./node.

@fhinkel

This comment has been minimized.

Show comment
Hide comment
@fhinkel

fhinkel Jan 24, 2017

Member

I think it would be good to get 5.5. into master, because it gives us more time to test it with Node. Also, there are some API changes in 5.5 that are needed to address open issues.

5.7 was released this week, 5.8 should be out in time to be included in LTS 8.

Member

fhinkel commented Jan 24, 2017

I think it would be good to get 5.5. into master, because it gives us more time to test it with Node. Also, there are some API changes in 5.5 that are needed to address open issues.

5.7 was released this week, 5.8 should be out in time to be included in LTS 8.

@ofrobots

This comment has been minimized.

Show comment
Hide comment
@ofrobots

ofrobots Jan 24, 2017

Contributor

Actually 5.7 was branched this week, not released. Also the timeline for 5.8 is close enough for it to be possible to be included in LTS 8, but there is open discussion on whether LTS 8 should include it: see #10117 (comment), #10970 and the discussion from the last Diagnostic WG meeting.

Contributor

ofrobots commented Jan 24, 2017

Actually 5.7 was branched this week, not released. Also the timeline for 5.8 is close enough for it to be possible to be included in LTS 8, but there is open discussion on whether LTS 8 should include it: see #10117 (comment), #10970 and the discussion from the last Diagnostic WG meeting.

@ofrobots

This comment has been minimized.

Show comment
Hide comment
@ofrobots

ofrobots Jan 24, 2017

Contributor

Currently the landing of V8 5.5 onto master seems to be blocked by this build issue: nodejs/build#595

Contributor

ofrobots commented Jan 24, 2017

Currently the landing of V8 5.5 onto master seems to be blocked by this build issue: nodejs/build#595

@targos targos referenced this pull request Jan 25, 2017

Merged

deps: update V8 to 5.6 #10992

2 of 2 tasks complete
@mhdawson

This comment has been minimized.

Show comment
Hide comment
@mhdawson

mhdawson Jan 25, 2017

Member

CI run after change to job to not run on smartos14 if version is 8 or higher: https://ci.nodejs.org/job/node-test-pull-request/6052/

Member

mhdawson commented Jan 25, 2017

CI run after change to job to not run on smartos14 if version is 8 or higher: https://ci.nodejs.org/job/node-test-pull-request/6052/

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Jan 26, 2017

Member

The only real CI failure is the clang ICE here, I assume that is expected?

New CI run, hopefully without infrastructure-related failures: https://ci.nodejs.org/job/node-test-commit/7490/

Member

addaleax commented Jan 26, 2017

The only real CI failure is the clang ICE here, I assume that is expected?

New CI run, hopefully without infrastructure-related failures: https://ci.nodejs.org/job/node-test-commit/7490/

ofrobots added a commit to ofrobots/node that referenced this pull request Jan 26, 2017

string_decoder: align UTF-8 handling with V8
V8 5.5 changed how invalid characters are handled and it now appears
to follow the WHATWG Encoding standard, where all of an invalid
character's bytes are replaced by a single replacement character
(\ufffd) instead of replacing each invalid byte with separate
replacement characters.

Example: the byte sequence 0xF0,0xB8,0x41 is decoded as '\ufffdA' in
V8 5.5, but is decoded as '\ufffd\ufffdA' in previous versions of V8.

PR-URL: nodejs#9618
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@jbergstroem

This comment has been minimized.

Show comment
Hide comment
@jbergstroem

jbergstroem Jan 26, 2017

Member

@addaleax correct. We'll just skip on that platform for now.

Member

jbergstroem commented Jan 26, 2017

@addaleax correct. We'll just skip on that platform for now.

@mhdawson

This comment has been minimized.

Show comment
Hide comment
Member

mhdawson commented Jan 26, 2017

targos added a commit that referenced this pull request Jan 26, 2017

deps: update V8 to 5.5.372.40
PR-URL: #9618
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

targos added a commit that referenced this pull request Jan 26, 2017

src: update NODE_MODULE_VERSION to 52
V8 5.5 is not API/ABI compatible with 5.4.
This commit increments NODE_MODULE_VERSION by one.

Refs: https://github.com/nodejs/CTC/blob/master/meetings/2016-09-28.md
PR-URL: #9618
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

targos added a commit that referenced this pull request Jan 26, 2017

test: move test-vm-function-redefinition to parallel
With the upstream fix in V8, function declarations now
work fine in the vm module and the test is no longer failing.

Refs: https://codereview.chromium.org/2334733002
Fixes: #548
PR-URL: #9618
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

targos added a commit that referenced this pull request Jan 26, 2017

repl: remove workaround for function redefinition
The issue is fixed upstream in V8. Thus we do not need this workaround
in REPL.

Fixes: #548
PR-URL: #9618
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

targos added a commit that referenced this pull request Jan 26, 2017

string_decoder: align UTF-8 handling with V8
V8 5.5 changed how invalid characters are handled and it now appears
to follow the WHATWG Encoding standard, where all of an invalid
character's bytes are replaced by a single replacement character
(\ufffd) instead of replacing each invalid byte with separate
replacement characters.

Example: the byte sequence 0xF0,0xB8,0x41 is decoded as '\ufffdA' in
V8 5.5, but is decoded as '\ufffd\ufffdA' in previous versions of V8.

PR-URL: #9618
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@targos

This comment has been minimized.

Show comment
Hide comment
@targos

targos Jan 26, 2017

Member

Landed in a67a04d...24ef1e6. Thanks everyone!

Member

targos commented Jan 26, 2017

Landed in a67a04d...24ef1e6. Thanks everyone!

@targos targos closed this Jan 26, 2017

@targos targos deleted the targos:v8-5.5 branch Jan 26, 2017

@pi0

This comment has been minimized.

Show comment
Hide comment
@pi0

pi0 Jan 27, 2017

@targos Congratulations and thanks for hard work :) So does it lands on 7.x semver-minor updates? And what is your ETA?
Thanks again :)
Update: It seems back-port is on progress here #11029 :))

pi0 commented Jan 27, 2017

@targos Congratulations and thanks for hard work :) So does it lands on 7.x semver-minor updates? And what is your ETA?
Thanks again :)
Update: It seems back-port is on progress here #11029 :))

@iddan

This comment has been minimized.

Show comment
Hide comment
@iddan

iddan Jan 31, 2017

Is this means we can use async/await safely?

iddan commented Jan 31, 2017

Is this means we can use async/await safely?

@kyrylkov

This comment has been minimized.

Show comment
Hide comment
@kyrylkov

kyrylkov Jan 31, 2017

@aniddan In nightly builds, yes. In Node 7.x not yet, be V8 5.5 wasn't backported yet (#11029)

kyrylkov commented Jan 31, 2017

@aniddan In nightly builds, yes. In Node 7.x not yet, be V8 5.5 wasn't backported yet (#11029)

MylesBorins added a commit to MylesBorins/node that referenced this pull request Feb 1, 2017

test: move test-vm-function-redefinition to parallel
With the upstream fix in V8, function declarations now
work fine in the vm module and the test is no longer failing.

Refs: https://codereview.chromium.org/2334733002
Fixes: nodejs#548
PR-URL: nodejs#9618
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

MylesBorins added a commit to MylesBorins/node that referenced this pull request Feb 1, 2017

repl: remove workaround for function redefinition
The issue is fixed upstream in V8. Thus we do not need this workaround
in REPL.

Fixes: nodejs#548
PR-URL: nodejs#9618
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@leodutra

This comment has been minimized.

Show comment
Hide comment
@leodutra

leodutra Feb 3, 2017

Will it be included in version 8 without flags?
Very well done @targos.

leodutra commented Feb 3, 2017

Will it be included in version 8 without flags?
Very well done @targos.

@kyrylkov

This comment has been minimized.

Show comment
Hide comment
@kyrylkov

kyrylkov Feb 3, 2017

@leodutra It is already in master branch, thus in Node.js 8

kyrylkov commented Feb 3, 2017

@leodutra It is already in master branch, thus in Node.js 8

targos added a commit to targos/node that referenced this pull request Feb 7, 2017

test: move test-vm-function-redefinition to parallel
With the upstream fix in V8, function declarations now
work fine in the vm module and the test is no longer failing.

Fixes: nodejs#548
Refs: https://codereview.chromium.org/2334733002
Refs: nodejs#9618

PR-URL: nodejs#11029
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Myles Borins <myles.borins@gmail.com>

targos added a commit to targos/node that referenced this pull request Feb 7, 2017

repl: remove workaround for function redefinition
The issue is fixed upstream in V8. Thus we do not need this workaround
in REPL.

Fixes: nodejs#548
Refs: nodejs#9618

PR-URL: nodejs#11029
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Myles Borins <myles.borins@gmail.com>

italoacasas added a commit to italoacasas/node that referenced this pull request Feb 14, 2017

test: move test-vm-function-redefinition to parallel
With the upstream fix in V8, function declarations now
work fine in the vm module and the test is no longer failing.

Fixes: nodejs#548
Refs: https://codereview.chromium.org/2334733002
Refs: nodejs#9618

PR-URL: nodejs#11029
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Myles Borins <myles.borins@gmail.com>

italoacasas added a commit to italoacasas/node that referenced this pull request Feb 14, 2017

repl: remove workaround for function redefinition
The issue is fixed upstream in V8. Thus we do not need this workaround
in REPL.

Fixes: nodejs#548
Refs: nodejs#9618

PR-URL: nodejs#11029
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
@joaocgreis

This comment has been minimized.

Show comment
Hide comment
@joaocgreis

joaocgreis Mar 28, 2017

Member

@targos https://github.com/nodejs/node/blob/master/BUILDING.md#unix still mentions Clang 3.4 as the required version. Do you know what the new required version is?

Member

joaocgreis commented Mar 28, 2017

@targos https://github.com/nodejs/node/blob/master/BUILDING.md#unix still mentions Clang 3.4 as the required version. Do you know what the new required version is?

@targos

This comment has been minimized.

Show comment
Hide comment
@targos

targos Mar 29, 2017

Member

@joaocgreis I don't know what is the actual required version but since we dropped the workaround from #8343 it must be at least 3.4.2.

Member

targos commented Mar 29, 2017

@joaocgreis I don't know what is the actual required version but since we dropped the workaround from #8343 it must be at least 3.4.2.

@jasnell jasnell referenced this pull request Apr 4, 2017

Closed

8.0.0 Release Proposal #12220

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment