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.

Sign up for GitHub

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

Jump to bottom

v7.x: backport update to V8 5.5 #11029

Merged
merged 8 commits into from
Feb 7, 2017
Merged

v7.x: backport update to V8 5.5 #11029

merged 8 commits into from
Feb 7, 2017

Conversation

targos
Copy link
Member

@targos targos commented Jan 26, 2017 •
edited
Loading

This is a backport of the update to V8 5.5 for v7.x.

Additions are:

  • Revert of the breaking UTF-8 decoder changes
  • Patches to recover ABI/API compatibility with V8 5.4
  • Patches to keep compatibility with all platforms supported by v7.x

/cc @nodejs/collaborators

CI: https://ci.nodejs.org/job/node-test-commit/7510
V8 tests: https://ci.nodejs.org/job/node-test-commit-v8-linux/543/

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

V8

@targos targos added semver-minor PRs that contain new features and should be released in the next minor version. v7.x v8 engine Issues and PRs related to the V8 dependency. labels Jan 26, 2017
@nodejs-github-bot nodejs-github-bot added v7.x v8 engine Issues and PRs related to the V8 dependency. labels Jan 26, 2017
@MylesBorins
Copy link
Contributor

MylesBorins commented Jan 26, 2017 via email

@ofrobots
Copy link
Contributor

This is heroic; kudos for putting it together!

@pi0 pi0 mentioned this pull request Jan 27, 2017
Closed
2 tasks
@misterdjules
Copy link

Thank you @targos!

@mcollina
Copy link
Member

Does this include #9730 as well? It might need to be reapplied.

@targos
Copy link
Member Author

targos commented Jan 27, 2017

@mcollina Yes it is included. Ref:v8/v8@a3197fa

@mcollina
Copy link
Member

Thanks!

@dnalborczyk dnalborczyk mentioned this pull request Jan 29, 2017
Merged
@stephank stephank mentioned this pull request Jan 29, 2017
Closed
@targos
Copy link
Member Author

targos commented Jan 31, 2017

@MylesBorins is https://ci.nodejs.org/view/Node.js-citgm/job/citgm-abi-smoker/ the job to launch?

@Trott
Copy link
Member

Trott commented Jan 31, 2017

Needs a rebase. Also, do any of the Collaborators giving this emoticon upvotes and positive comments feel comfortable enough approving it? 1500+ files changed. Personally, I wouldn't know where to begin to even spot-check this sort of thing before dropping in a rubber-stamp approval. Paging @nodejs/v8 for help.

@rom1504
Copy link

rom1504 commented Jan 31, 2017

@Trott if you look at all commits of this PR except "deps: update V8 to 5.5.372.40" then actually little changes were made ;)

@bricss
Copy link

bricss commented Jan 31, 2017

I believe that V8 dev team already approved it, also there is a pile of tests for this. Just merge it. 😃

@MylesBorins
Copy link
Contributor

MylesBorins commented Feb 1, 2017 •
edited
Loading

I'm going to spend some time tomorrow getting the abi smoker up and running again. Thanks for the patience everyone :D

edit: I totally said tomorrow 5 days ago... totally for serious mean it this time

@adnan-kamili adnan-kamili mentioned this pull request Feb 1, 2017
Closed
@MylesBorins
Copy link
Contributor

MylesBorins commented Feb 1, 2017 •
edited
Loading

Rebased the rebuild job and fingers crossed it should be working

--> https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker-rebuild/3/

Will dig into results tomorrow

edit: the above was just the job that forces a rebuild of native modules, below is a job that builds with v7.5.0 and then tests with the node built from this PR

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

edit:

citgm job of v7.5.0 for comparison

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

edit 2:

the rebuild job has some failures that are a bit concerning, the abi smoker has passed. Only failures are known flakes

edit 3:

citgm abi smoker with --rebuild: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-abi-smoker/30/

edit 4:

seems like running it with rebuild BROKE THE WORLD

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

edit 5:

NVM the above rebuild was accidentally run on master and everything breaking was due to module version mismatch.

citgm-abi-smoker with rebuild with 5.5: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-abi-smoker/31/

citgm-rebuild-smoker on v7.5.0 for comparison: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker-rebuild/4/

addaleax
addaleax approved these changes Feb 3, 2017
View reviewed changes
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

LABICTM¹

¹ looks abi compatible to me 😄

deps/v8/include/v8.h
/**
* Returns the number of wrappers that are still to be traced by the embedder.
*/
virtual size_t NumberOfWrappersToTrace() { return 0; }
Copy link
Member

Choose a reason for hiding this comment

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

Just for my own understanding – it doesn’t make sense for addons to set an EmbedderHeapTracer, which is why we don’t care about these breakages, right?

Copy link
Member

Choose a reason for hiding this comment

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

I suppose it is technically possible for an add-on to call Isolate::SetEmbedderHeapTracer() but I can't see that being useful: an add-on won't know how to trace objects it didn't create itself.

(Neither does core know about objects created by add-ons. This API doesn't seem all that usable inside the node ecosystem, it seems to assume the embedder is a single monolithic entity.)

Copy link
Member

Choose a reason for hiding this comment

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

@bnoordhuis Thanks for confirming!

deps/v8/include/v8.h
@@ -6857,8 +7460,6 @@ class V8_EXPORT V8 {
int* index);
static Local<Value> GetEternal(Isolate* isolate, int index);

static void RegisterExternallyReferencedObject(internal::Object** object,
internal::Isolate* isolate);
Copy link
Member

Choose a reason for hiding this comment

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

(ditto)

This was referenced Feb 5, 2017
Closed
Merged
@targos
repl: remove workaround for function redefinition
ce020fb 
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>
@targos
deps: ensure V8 5.4 ABI compatibility
85f45cc 
V8 5.5 is not ABI-compatible with 5.4.
Make the necessary changes to V8's header files so that native addons
compiled against a previous version of node can still be loaded..

* Remove new fields from {Indexed|Named}PropertyHandlerConfiguration.
* Revert "[tracing] Support ConvertableToTraceFormat argument type."
  * This reverts commit 4810f41a521227ccab4f231aa4a439d790428953 from V8.
* Remove tests for getOwnPropertyDescriptor and defineProperty interceptors.

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
deps: revert breaking UTF-8 decoder changes in V8
1d55b34 
Refs: v8/v8@7c46245
Refs: v8/v8@aadb1c8

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
deps: work around SmartOS 14 incompatibility
fd04af1 
Refs: nodejs/v8#1

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 targos merged commit fd04af1 into nodejs:v7.x-staging Feb 7, 2017
@lopezjurip lopezjurip mentioned this pull request Feb 7, 2017
Closed
@dead-horse dead-horse mentioned this pull request Feb 8, 2017
Closed
9 tasks
@jbajwa jbajwa mentioned this pull request Feb 9, 2017
Closed
3 tasks
italoacasas pushed a commit to italoacasas/node that referenced this pull request Feb 14, 2017
@targos @italoacasas
deps: update V8 to 5.5.372.40
61870b4 
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 pushed a commit to italoacasas/node that referenced this pull request Feb 14, 2017
@targos @italoacasas
deps: cherry-pick workaround for clang-3.4 ICE
7fea966 
Fixes: nodejs#8323
Refs: nodejs#8343
Refs: nodejs#8317

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 pushed a commit to italoacasas/node that referenced this pull request Feb 14, 2017
@mhdawson @italoacasas
deps: limit regress/regress-crbug-514081 v8 test
53e00e1 
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: nodejs#6340
Refs: nodejs#6678

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 pushed a commit to italoacasas/node that referenced this pull request Feb 14, 2017
@fhinkel @italoacasas
test: move test-vm-function-redefinition to parallel
8612a00 
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 pushed a commit to italoacasas/node that referenced this pull request Feb 14, 2017
@targos @italoacasas
repl: remove workaround for function redefinition
fe2f058 
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 pushed a commit to italoacasas/node that referenced this pull request Feb 14, 2017
@targos @italoacasas
deps: ensure V8 5.4 ABI compatibility
22e2288 
V8 5.5 is not ABI-compatible with 5.4.
Make the necessary changes to V8's header files so that native addons
compiled against a previous version of node can still be loaded..

* Remove new fields from {Indexed|Named}PropertyHandlerConfiguration.
* Revert "[tracing] Support ConvertableToTraceFormat argument type."
  * This reverts commit 4810f41a521227ccab4f231aa4a439d790428953 from V8.
* Remove tests for getOwnPropertyDescriptor and defineProperty interceptors.

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 pushed a commit to italoacasas/node that referenced this pull request Feb 14, 2017
@targos @italoacasas
deps: revert breaking UTF-8 decoder changes in V8
028bb63 
Refs: v8/v8@7c46245
Refs: v8/v8@aadb1c8

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 pushed a commit to italoacasas/node that referenced this pull request Feb 14, 2017
@targos @italoacasas
deps: work around SmartOS 14 incompatibility
5446fa7 
Refs: nodejs/v8#1

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 that referenced this pull request Feb 16, 2017
@italoacasas
2017-02-21, Version 7.6.0 (Current)
12ed929 
Notable changes:

* deps:
    * update V8 to 5.5 (Michaël Zasso) [#11029](#11029)
    * upgrade libuv to 1.11.0 (cjihrig) [#11094](#11094)
    * add node-inspect 1.10.2 (Jan Krems) [#10187](#10187)
* lib: build `node inspect` into `node` (Anna Henningsen) [#10187](#10187)
* crypto: Remove expired certs from CNNIC whitelist (Shigeki Ohtsu) [#9469](#9469)
* inspector: add --inspect-brk (Josh Gavant) [#11149](#11149)
* fs: allow WHATWG URL and file: URLs as paths (James M Snell) [#10739](#10739)
* src: support UTF-8 in compiled-in JS source files (Ben Noordhuis) [#11129](#11129)
* url: extend url.format to support WHATWG URL (James M Snell) [#10857](#10857)

PR-URL: #11185
italoacasas added a commit to italoacasas/node that referenced this pull request Feb 21, 2017
@italoacasas
2017-02-21, Version 7.6.0 (Current)
bebda6d 
Notable changes:

* deps:
    * update V8 to 5.5 (Michaël Zasso) [nodejs#11029](nodejs#11029)
    * upgrade libuv to 1.11.0 (cjihrig) [nodejs#11094](nodejs#11094)
    * add node-inspect 1.10.4 (Jan Krems) [nodejs#10187](nodejs#10187)
    * upgrade zlib to 1.2.11 (Sam Roberts) [nodejs#10980](nodejs#10980)
* lib: build `node inspect` into `node` (Anna Henningsen) [nodejs#10187](nodejs#10187)
* crypto: Remove expired certs from CNNIC whitelist (Shigeki Ohtsu) [nodejs#9469](nodejs#9469)
* inspector: add --inspect-brk (Josh Gavant) [nodejs#11149](nodejs#11149)
* fs: allow WHATWG URL objects as paths (James M Snell) [nodejs#10739](nodejs#10739)
* src: support UTF-8 in compiled-in JS source files (Ben Noordhuis) [nodejs#11129](nodejs#11129)
* url: extend url.format to support WHATWG URL (James M Snell) [nodejs#10857](nodejs#10857)

PR-URL: nodejs#11185
@misterdjules misterdjules mentioned this pull request Feb 21, 2017
Closed
italoacasas added a commit to italoacasas/node that referenced this pull request Feb 22, 2017
@italoacasas
2017-02-21, Version 7.6.0 (Current)
2b5d83d 
Notable changes:

* deps:
    * update V8 to 5.5 (Michaël Zasso) [nodejs#11029](nodejs#11029)
    * upgrade libuv to 1.11.0 (cjihrig) [nodejs#11094](nodejs#11094)
    * add node-inspect 1.10.4 (Jan Krems) [nodejs#10187](nodejs#10187)
    * upgrade zlib to 1.2.11 (Sam Roberts) [nodejs#10980](nodejs#10980)
* lib: build `node inspect` into `node` (Anna Henningsen) [nodejs#10187](nodejs#10187)
* crypto: Remove expired certs from CNNIC whitelist (Shigeki Ohtsu) [nodejs#9469](nodejs#9469)
* inspector: add --inspect-brk (Josh Gavant) [nodejs#11149](nodejs#11149)
* fs: allow WHATWG URL objects as paths (James M Snell) [nodejs#10739](nodejs#10739)
* src: support UTF-8 in compiled-in JS source files (Ben Noordhuis) [nodejs#11129](nodejs#11129)
* url: extend url.format to support WHATWG URL (James M Snell) [nodejs#10857](nodejs#10857)

PR-URL: nodejs#11185
imyller added a commit to imyller/meta-nodejs that referenced this pull request Mar 2, 2017
@imyller
2017-02-21, Version 7.6.0 (Current)
75b0a8a 
    Notable changes:

    * deps:
        * update V8 to 5.5 (Michaël Zasso) [#11029](nodejs/node#11029)
        * upgrade libuv to 1.11.0 (cjihrig) [#11094](nodejs/node#11094)
        * add node-inspect 1.10.4 (Jan Krems) [#10187](nodejs/node#10187)
        * upgrade zlib to 1.2.11 (Sam Roberts) [#10980](nodejs/node#10980)
    * lib: build `node inspect` into `node` (Anna Henningsen) [#10187](nodejs/node#10187)
    * crypto: Remove expired certs from CNNIC whitelist (Shigeki Ohtsu) [#9469](nodejs/node#9469)
    * inspector: add --inspect-brk (Josh Gavant) [#11149](nodejs/node#11149)
    * fs: allow WHATWG URL objects as paths (James M Snell) [#10739](nodejs/node#10739)
    * src: support UTF-8 in compiled-in JS source files (Ben Noordhuis) [#11129](nodejs/node#11129)
    * url: extend url.format to support WHATWG URL (James M Snell) [#10857](nodejs/node#10857)

    PR-URL: nodejs/node#11185

Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>
@gibfahn gibfahn mentioned this pull request Apr 24, 2017
Closed
31 tasks
@targos targos deleted the v8-5.5-v7 branch June 1, 2017 07:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@addaleax addaleax addaleax approved these changes

@bnoordhuis bnoordhuis bnoordhuis approved these changes

@MylesBorins MylesBorins MylesBorins approved these changes

Assignees
No one assigned
Labels
semver-minor PRs that contain new features and should be released in the next minor version. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.