Backport V8 5.1 to v6.x #8054

ofrobots · 2016-08-10T18:37:22Z

Checklist

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

Affected core subsystem(s)

deps, test, build, dtrace, repl

Description of change

This PR backports all the necessary commits for upgrading v6.x to V8 5.1 as requested here: #7016 (comment). This PR is expected to be ABI compatible with V8 5.0.

There were several commits on master that incrementally upgraded to 5.1.281.75 that have been collapsed into a single commit updating directly to 5.1.281.75 (this commit needs a rubber stamp review).

_NOTE:_ Omit --whitespace=fix at landing time. There is significant whitespace in some of the V8 tests included in this PR.

/cc @thealphanerd @nodejs/v8

addaleax · 2016-08-10T19:20:13Z

Rubber-stamp LGTM for the V8 update, thanks for putting this together!

MylesBorins · 2016-08-10T19:37:45Z

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

fingers crossed 😄

kibertoad · 2016-08-10T21:32:52Z

And the results are in: https://ci.nodejs.org/job/citgm-abi-smoker/

addaleax · 2016-08-10T23:12:21Z

I don’t know if ws is known to be problematic with citgm, but I couldn’t reproduce any problems manually.

ofrobots · 2016-08-11T17:32:10Z

Added some V8 5.1 backport commits that have landed on master.

@thealphanerd could you take a quick look at the smoker results to see which failures are real / worth investigating?

MylesBorins · 2016-08-11T18:39:55Z

I am going to dig into all of these results today. Re running with the new commits, and also running on v6.x so we can compare results

v6.x: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/347/
this pr tested against v6.3.1 build: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-abi-smoker/13/

Both of these jobs are being run using the new junit reporter, which should make analyzing the output a nicer experience!

MylesBorins · 2016-08-17T04:46:37Z

@ofrobots it looks like this needs a rebase.

ofrobots · 2016-08-17T17:24:11Z

Rebased + added backport from #8099.

MylesBorins · 2016-08-18T20:31:37Z

Howdy all, I've got an update.

The ABI smoker job is mostly working... I'm banging my head against a system failure on OSX, but this is CI issue, not related to ABI.

--> https://ci.nodejs.org/view/Node.js-citgm/job/citgm-abi-smoker/
When this flag is include npm rebuild and node-gyp rebuild are also run during the process, so we can confirm compilation using the base node version.

edit:

I was running the smoker job against the wrong $HEAD originally.

citgm ABI: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-abi-smoker/23/
citgm ABI /w rebuild: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-abi-smoker/24/

edit 2:

citgm abi is looking good, only expected failures.
citgm abi /w rebuild has a single unexpected failure, graceful-fs...

Running just the tests on that --> https://ci.nodejs.org/view/Node.js-citgm/job/citgm-abi-smoker/25/

edit 3:

Look like everything passed on the run of only graceful-fs 🎉

MylesBorins · 2016-08-19T14:42:30Z

Smoke testing implies that there are no ABI breakages in this PR.

LGTM

ofrobots · 2016-08-22T16:12:13Z

@thealphanerd Thanks for all the work in getting this verified w/ citgm smoke testing!

@nodejs/ctc Can I get some more LGTMs for landing this on v6.x?

mhdawson · 2016-08-22T22:59:49Z

Based on the Smoke testing LGTM.

evanlucas · 2016-08-23T00:25:35Z

rubberstamp LGTM

bnoordhuis · 2016-08-23T09:18:09Z

Also rubber-stamp LGTM.

added backport from #8099.

For posterity, this looks to have been a clean cherry-pick. Only the commit hash is different.

ofrobots · 2016-08-24T15:58:09Z

Thanks. I will land this today.

Pick up the latest branch-head for V8 5.1. This branch brings in improved language support and performance improvements. For full details: http://v8project.blogspot.com/2016/04/v8-release-51.html * Picks up the latest branch head for 5.1 [1] * Edit v8 gitignore to allow trace_event copy * Update V8 DEP trace_event as per deps/v8/DEPS [2] [1] https://chromium.googlesource.com/v8/v8.git/+/5.1.281.75 [2] https://chromium.googlesource.com/chromium/src/base/trace_event/common/+/c8c8665 Introduces a semver-minor overload of v8::Function::New() for use by v8_inspector. PR-URL: nodejs#8054 Refs: nodejs#7016 Refs: nodejs#7586 Refs: nodejs#7615 Reviewed-By: addaleax - Anna Henningsen <anna@addaleax.net> Reviewed-By: thealphanerd - Myles Borins <myles.borins@gmail.com> Reviewed-By: mhdawson - Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: evanlucas - Evan Lucas <evanlucas@me.com> Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>

V8 erroneously did null pointer checks on `this`. It can lead to a SIGSEGV crash if node is compiled with GCC 6. Backport relevant changes from [1] that fix this issue. [1]: https://codereview.chromium.org/1900423002 Fixes: nodejs#6272 PR-URL: nodejs#6544 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>

V8 improved the error message for illegal token in v8/v8@879b617b. This breaks the recoverable error handling in repl. Ref: nodejs#6482 PR-URL: nodejs#7016 Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>

* Changes to messages. * V8 enabled proxy support by default. The --harmony_proxies flag is now gone. PR-URL: nodejs#6482 Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>

V8 5.1 changes the layout of stack frames. PR-URL: nodejs#6482 Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>

It is no longer necessary now that libplatform/libplatform.h uses proper includes. PR-URL: nodejs#7016 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

The removal of the promise debug event is an API/ABI breaking change. Ref: https://codereview.chromium.org/1833563002 Ref: #23 PR-URL: nodejs#7016 Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

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

Remove the `_malloced_memory` field from the `HeapStatistics` class to achieve full ABI compatibility with V8 5.0. Ref: nodejs#7016 PR-URL: nodejs#7526 Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

Original commit message: Quit creating array literal boilerplates from Crankshaft. It's such a corner case. BUG= Review URL: https://codereview.chromium.org/1865013002 Cr-Commit-Position: refs/heads/master@{nodejs#35346} Fixes: nodejs#7454 PR-URL: nodejs#7632 Reviewed-By: Michaël Zasso <mic.besace@gmail.com>

Original commit message: Handle symbols in FrameMirror#invocationText(). Fix a TypeError when putting together the invocationText for a symbol method's stack frame. See nodejs#7536. Review-Url: https://codereview.chromium.org/2122793003 Cr-Commit-Position: refs/heads/master@{nodejs#37597} Fixes: nodejs#7536 PR-URL: nodejs#7612 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Michaël Zasso <mic.besace@gmail.com>

Original commit message: S390:Update inline asm constraint in test-platform The GetStackPointer() routine in test-platform uses an inline assembly code to store the current stack pointer value into a static variable sp_addr. The existing asm code for S390 uses an ST/STG instruction, with the memory operand associated with the general ('=g') constraint to sp_addr. On GCC 4.8.5, the GCC compiler got confused and treated sp_addr as an integer operand instead of memory operand, resulting in a store being emitted that writes to an invalid meory location. Given the specific store instructions being inlined here, we should restict the sp_addr operand to explicitly be a memory operand using '=m' instead of '=g'. R=bmeurer@chromium.org,jkummerow@chormium.org,rmcilroy@chromium.org,yangguo@chromium.org BUG= Review-Url: https://codereview.chromium.org/2158523002 Cr-Commit-Position: refs/heads/master@{nodejs#37809} Fixes: nodejs#7659 PR-URL: nodejs#7771 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>

Original commit message: If we can't rehash the backing store for weak sets & maps, do a last resort GC BUG=v8:4909 R=hpayer@chromium.org Committed: https://crrev.com/b93c80a6039c757723e70420ae73375b5d277814 Cr-Commit-Position: refs/heads/master@{#37591} Fixes: nodejs#6180 PR-URL: nodejs#7689 Reviewed-By: Matt Loring <mattloring@google.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

Original commit message: Fix incorrect parameter to HasSufficientCapacity It takes the number of additional elements, not the total target capacity. Also, avoid right-shifting a negative integer as this is undefined in general BUG=v8:4909 R=verwaest@chromium.org Review-Url: https://codereview.chromium.org/2162333002 Cr-Commit-Position: refs/heads/master@{nodejs#37901} Fixes: nodejs#6180 PR-URL: nodejs#7689 Reviewed-By: Matt Loring <mattloring@google.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

Original commit message: [regexp] Fix case-insensitive matching for one-byte subjects. The bug occurs because we do not canonicalize character class ranges before adding case equivalents. While adding case equivalents, we abort early for one-byte subject strings, assuming that the ranges are sorted. Which they are not. R=marja@chromium.org BUG=v8:5199 Review-Url: https://codereview.chromium.org/2159683002 Cr-Commit-Position: refs/heads/master@{nodejs#37833} Fixes: nodejs#7708 PR-URL: nodejs#7833 Reviewed-By: targos - Michaël Zasso <mic.besace@gmail.com> Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: ofrobots - Ali Ijaz Sheikh <ofrobots@google.com>

Original commit message: [build] Add force_dynamic_crt option to build a static library with /… …MD on windows Adds option to build a V8 library statically, but with the options on windows that allows it to be subsequently included in another DLL. On Windows this is required for it to correclty link against the correct C++ runtime. Require for our Node.js shared library build. Reference: nodejs#7487 BUG= R=machenbach@chromium.org, michael_dawson@ca.ibm.com Committed: https://crrev.com/9cf88c1c364cf76c1e745aa63196768435e8ef5d Review-Url: https://codereview.chromium.org/2149963002 Cr-Original-Commit-Position: refs/heads/master@{nodejs#37814} Cr-Commit-Position: refs/heads/master@{nodejs#37856} PR-URL: nodejs#7802 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

Pick up an upstream bugfix for https://crbug.com/621926 and bump V8 version to 5.1.281.80. Original commit message for 588e15c: Fixes a bug in cmpw. The opcodes for 'cmpw r/m16, r16' and 'cmpw r16, r/m16' were swapped, causing a few issues when less than/greater than comparison were performed. Adds a regression test. BUG=621926 Committed: https://crrev.com/efa7095e3e360fbadbe909d831ac11b268ca26b0 Review-Url: https://codereview.chromium.org/2103713003 Cr-Original-Commit-Position: refs/heads/master@{nodejs#37339} Cr-Commit-Position: refs/heads/master@{nodejs#37345} Original commit message for c0d4bb8: Fixes a wrong use of Operand in a test. Operand(reg) -> reg Operand(reg, 0) -> [reg] BUG= Review-Url: https://codereview.chromium.org/2111503002 Cr-Commit-Position: refs/heads/master@{nodejs#37370} PR-URL: nodejs#8038 Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: ofrobots - Ali Ijaz Sheikh <ofrobots@google.com> Reviewed-By: mhdawson - Michael Dawson <michael_dawson@ca.ibm.com>

Original commit message: [Debugger] Fix StepNext over function with caught exception Without CL debugger on StepNext adds breakpoint to function where throw instruction is located. In case of StepNext we will skip pause in this function because StepNext shouldn't break in a deeper frame. BUG=chromium:604495 R=yangguo@chromium.org LOG=N Review URL: https://codereview.chromium.org/1894263002 Cr-Commit-Position: refs/heads/master@{nodejs#35627} Fixes: nodejs#7219 PR-URL: nodejs#8099 Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>

ofrobots · 2016-08-25T17:45:14Z

Landed as 92ecbc4...723fa96.

Notable changes: * **buffer**: Fix regression introduced in v6.4.0 that prevented .write() at buffer end (Anna Henningsen) #8154 * **deps**: update V8 to 5.1.281.75 (Ali Ijaz Sheikh) #8054 * **inspector**: * fix inspector hang while disconnecting (Aleksei Koziatinskii) #8021 * add support for uncaught exception (Aleksei Koziatinskii) #8043 * **repl**: Fix saving editor mode text in `.save` (Prince J Wesley) #8145 * ***Revert*** "**repl,util**: insert carriage returns in output" (Evan Lucas) #8143 PR-URL: #8253

Fishrock123 · 2016-08-29T15:12:53Z

@nodejs/v8 Did this introduce any new compiler requirements?

Our builds are failing on debian wheezy using clang-3.4.

Fishrock123 · 2016-09-06T21:17:40Z

Retroactively marking as dont-land-on-v6.x so that it does not conflict with our release tooling (branch-diff)

nodejs-github-bot added the v8 engine Issues and PRs related to the V8 dependency. label Aug 10, 2016

ofrobots mentioned this pull request Aug 10, 2016

deps: update V8 to 5.1.281.69 #7016

Closed

2 tasks

ofrobots added the semver-minor PRs that contain new features and should be released in the next minor version. label Aug 10, 2016

mscdex added the v6.x label Aug 10, 2016

ofrobots mentioned this pull request Aug 11, 2016

Cherry pick 588e15c and c0d4bb8 from upstream V8 #8038

Closed

3 tasks

ofrobots mentioned this pull request Aug 11, 2016

deps: cherry-pick 6f68f30 from v8 upstream #7802

Closed

2 tasks

matthewloring mentioned this pull request Aug 17, 2016

v6 version of V8 has bad for-in implementation #8133

Closed

ofrobots force-pushed the v6.x-switch-to-V8-5.1 branch from 67bed8e to 53da7cd Compare August 17, 2016 17:22

ofrobots mentioned this pull request Aug 17, 2016

deps: cherry-pick de5aaad from V8's upstream #8099

Closed

2 tasks

targos mentioned this pull request Aug 22, 2016

REPL debugging Proxy bug #8224

Closed

ofrobots force-pushed the v6.x-switch-to-V8-5.1 branch from 53da7cd to c8981e8 Compare August 25, 2016 17:29

ofrobots force-pushed the v6.x-switch-to-V8-5.1 branch from c8981e8 to 723fa96 Compare August 25, 2016 17:33

targos and others added 17 commits August 25, 2016 10:33

repl: fix repl after V8 upgrade

66e66e5

V8 improved the error message for illegal token in v8/v8@879b617b. This breaks the recoverable error handling in repl. Ref: nodejs#6482 PR-URL: nodejs#7016 Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>

test: fix tests after V8 upgrade

cda8bfc

* Changes to messages. * V8 enabled proxy support by default. The --harmony_proxies flag is now gone. PR-URL: nodejs#6482 Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>

dtrace: fix ustack helper for V8 5.1

b1922e7

V8 5.1 changes the layout of stack frames. PR-URL: nodejs#6482 Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>

build: don't include V8 from node.gyp

dcd0655

It is no longer necessary now that libplatform/libplatform.h uses proper includes. PR-URL: nodejs#7016 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

deps: revert removal of V8::PromiseEvent

06a6c03

The removal of the promise debug event is an API/ABI breaking change. Ref: https://codereview.chromium.org/1833563002 Ref: #23 PR-URL: nodejs#7016 Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

deps: bring in V8 5.1 - 5.0 ABI compatibility

2c46e23

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

ofrobots merged commit 723fa96 into nodejs:v6.x Aug 25, 2016

richardlau mentioned this pull request Aug 25, 2016

2016-08-26, Version 6.5.0 (Current) #8253

Merged

Fishrock123 mentioned this pull request Aug 29, 2016

v6.5.0 fails to build using clang-3.4. #8323

Closed

Fishrock123 added the dont-land-on-v6.x label Sep 6, 2016

christophehurpeau mentioned this pull request Sep 15, 2016

Node 6 support destructuring and function name jhen0409/babel-preset-es2015-node6#4

Open

MylesBorins mentioned this pull request Nov 15, 2016

Fix V8 patch #9610

Merged

papandreou mentioned this pull request Mar 3, 2017

V8 crash during MarkCompact GC #7259

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Backport V8 5.1 to v6.x #8054

Backport V8 5.1 to v6.x #8054

ofrobots commented Aug 10, 2016

addaleax commented Aug 10, 2016

MylesBorins commented Aug 10, 2016

kibertoad commented Aug 10, 2016

addaleax commented Aug 10, 2016 •

edited

Loading

ofrobots commented Aug 11, 2016

MylesBorins commented Aug 11, 2016

MylesBorins commented Aug 17, 2016

ofrobots commented Aug 17, 2016

MylesBorins commented Aug 18, 2016 •

edited

Loading

MylesBorins commented Aug 19, 2016

ofrobots commented Aug 22, 2016

mhdawson commented Aug 22, 2016

evanlucas commented Aug 23, 2016

bnoordhuis commented Aug 23, 2016

ofrobots commented Aug 24, 2016

ofrobots commented Aug 25, 2016

Fishrock123 commented Aug 29, 2016

Fishrock123 commented Sep 6, 2016

Backport V8 5.1 to v6.x #8054

Backport V8 5.1 to v6.x #8054

Conversation

ofrobots commented Aug 10, 2016

Checklist

Affected core subsystem(s)

Description of change

addaleax commented Aug 10, 2016

MylesBorins commented Aug 10, 2016

kibertoad commented Aug 10, 2016

addaleax commented Aug 10, 2016 • edited Loading

ofrobots commented Aug 11, 2016

MylesBorins commented Aug 11, 2016

MylesBorins commented Aug 17, 2016

ofrobots commented Aug 17, 2016

MylesBorins commented Aug 18, 2016 • edited Loading

MylesBorins commented Aug 19, 2016

ofrobots commented Aug 22, 2016

mhdawson commented Aug 22, 2016

evanlucas commented Aug 23, 2016

bnoordhuis commented Aug 23, 2016

ofrobots commented Aug 24, 2016

ofrobots commented Aug 25, 2016

Fishrock123 commented Aug 29, 2016

Fishrock123 commented Sep 6, 2016

addaleax commented Aug 10, 2016 •

edited

Loading

MylesBorins commented Aug 18, 2016 •

edited

Loading