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

[WIP] deps: update V8 to 8.0 #32095

Closed
wants to merge 22 commits into from
Closed

[WIP] deps: update V8 to 8.0 #32095

wants to merge 22 commits into from

Conversation

mmarchini
Copy link
Contributor

Chrome release date: Feb 04th, 2020.

There's one outstanding issue on 8.0 (https://github.com/nodejs/node-v8/issues/120), but it might not be worth blocking the upgrade because of that (we're unlikely to see that issue outside very specific, synthetic test cases).

Also, build might be broken on Windows. I'll look into that before marking as ready for review.

mmarchini and others added 21 commits March 4, 2020 10:08
Major V8 updates are usually API/ABI incompatible with previous
versions. This commit adapts NODE_MODULE_VERSION for V8 8.0.

Refs: https://github.com/nodejs/CTC/blob/master/meetings/2016-09-28.md
Original commit message:

    [testrunner] delete ancient junit compatible format support

    Testrunner has ancient support for JUnit compatible XML output.

    This CL removes this old feature.

    R=mstarzinger@chromium.org,jgruber@chromium.org,jkummerow@chromium.org
    CC=​machenbach@chromium.org

    Bug: v8:8728
    Change-Id: I7e1beb011dbaec3aa1a27398a5c52abdd778eaf0
    Reviewed-on: https://chromium-review.googlesource.com/c/1430065
    Reviewed-by: Jakob Gruber <jgruber@chromium.org>
    Reviewed-by: Michael Starzinger <mstarzinger@chromium.org>
    Commit-Queue: Tamer Tas <tmrts@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#59045}

Refs: v8/v8@bd019bd

PR-URL: nodejs#26685
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
PR-URL: nodejs#26685
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Patch V8 (compiler/js-heap-broker.cc) to remove the use of an optional
property, which is a fairly new C++ feature, since that requires a newer
XCode version than the minimum requirement in BUILDING.md and thus
breaks CI.

PR-URL: nodejs#29694
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
Co-authored-by: Ujjwal Sharma <usharma1998@gmail.com>
These methods will be removed in V8 8.1, hence we need to stop
overriding them.
The following metadata has changed in V8:

- v8dbg_class_ConsString__first_offset__int
  - Use v8dbg_class_ConsString__first__String instead.
  - Refs: v8/v8@b38dfaf

- v8dbg_class_ConsString__second_offset__int
  - Use v8dbg_class_ConsString__second__String instead.
  - Refs: v8/v8@b38dfaf

- v8dbg_class_Map__constructor_or_backpointer__Object
  - This constant has not changed, but the postmortem metadata generation
    script required an update.

- v8dbg_class_SlicedString__offset_offset__int
  - Use v8dbg_class_SlicedString__offset__SMI instead.
  - Refs: v8/v8@b38dfaf

- v8dbg_class_ThinString__actual_offset__int
  - Use v8dbg_class_ThinString__actual__String instead.
  - Refs: v8/v8@b38dfaf
This commit updates v8abbr.h to use the updated metadata.
Bump minimum version of ICU needed to build node to 65.

Refs: v8/v8@74bf96e
This commit replaces Symbol::Name() with
Symbol::Description().

Fixes: nodejs#30916
V8 is about to increase the max TypedArray length to 2**32-1, which
Node inherits as Buffer.kMaxLength. Some tests relied on values greater
than the previous max length (2**31-1) to throw errors; this updates
those tests for the new max length.
Serialization support of WasmModuleObjects will be removed in
https://crrev.com/c/2013110, thus we need to stop testing for that.
The {SetExpectInlineWasm} method is deprecated and non-functional since
V8 v8.1.
Thus node should stop calling it, so that it can be fully removed in a
future v8 version.
This removes uses of the "IsWebAssemblyCompiledModule" method, which is
deprecated in V8 v8.1 and will be removed in v8.2.
We could replace it by "IsWasmModuleObject", but since it's unused in
node anyway, I just remove the definition.
@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. v8 engine Issues and PRs related to the V8 dependency. labels Mar 4, 2020
@mmarchini
Copy link
Contributor Author

@targos not sure if you was working on this already. If you were, I'm happy to close this PR.

I cherry-picked commits from canary-base, and removed the ones that looked 8.1+ specific.

There are some commits cherry-picked from v8/node (I assume) which do not follow our commit message convention. Should I rewrite those commits? How do we keep track of commits merged on upgrades to remove from canary-base later?

Do we have any way to ensure previously backported V8 commits are preserved when upgrading? I noticed #31957 was overridden, and I assume other backports were as well.

Original commit message:

    Fix MSVC component build

    Add the necessary V8_EXPORT_PRIVATE attributes and a few other minor
    changes to make building DLLs with MSVC happy. (Note: Debug builds still
    seem to be failing in Torque, but this fixes Release builds).

    Bug: v8:8791
    Change-Id: Ia4d5372fd1cb961e6268a2b5c089bcd17822f1e5
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1996157
    Reviewed-by: Jakob Kummerow <jkummerow@chromium.org>
    Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
    Reviewed-by: Tobias Tebbi <tebbi@chromium.org>
    Commit-Queue: Jakob Kummerow <jkummerow@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#65822}

Refs: v8/v8@e8e324a
@targos
Copy link
Member

targos commented Mar 5, 2020

@mmarchini TBH, I would like to skip 8.0 and upgrade directly to 8.1, but if you want to merge 8.0 first, I have no objections.

This is how I usually prepare an upgrade:

  • Update V8:
git checkout master
git pull upstream master
git checkout -b v8-8.0
git node v8 major --branch=8.0-lkgr
  • Cherry-pick commits from canary-base.

    • Ignore commits for later versions
    • Squash all commits related to gyp files
    • Rewrite commit messages for commits coming from v8/node
  • Check for existing backported commits on master since last V8 upgrade (this page is useful: https://github.com/nodejs/node/commits/master/deps/v8)

    • Go from oldest to newest
    • You can open the "Refs" link from the commit message to see if it already exists in the new V8 version or should be backported again
    • If a commit must be backported, use git node v8 backport SHA to backport it. If there are conflicts, try to cherry-pick the one from master.
    • Make sure the embedder string is incremented by one at each backported commit (that's one of the reasons why using git node backport is good, as it increments it automatically).
  • Open the PR

  • Update canary-base branch:

    • git checkout canary-base
    • git reset --hard upstream/master
    • git node v8 major
    • cherry-pick commits from the v8-8.0 update branch (from the first one after the embedder string is reset)
    • cherry-pick commits from upstream/canary-base (from the first one that wasn't included in v8-8.0.
    • git push --force

Feel free to ask if you have any question.

@mmarchini
Copy link
Contributor Author

Thank you for sharing! Should we document the steps? I can send a PR.

TBH, I would like to skip 8.0 and upgrade directly to 8.1, but if you want to merge 8.0 first, I have no objections.

I was thinking 8.0 could be backportable to v13, but with v14 one month away it might not be worth the effort (especially with that Windows build failure).

@gengjiawen
Copy link
Member

The windows build failure looks a lot like we deal in nodejs/node-v8#126 (comment). cc @targos

I was thinking 8.0 could be backportable to v13,

Any known problem for v8 8.1 backport to v13 ? I am inclined upgrade to v8 8.1 directly too.

@mmarchini
Copy link
Contributor Author

The windows build failure looks a lot like we deal in nodejs/node-v8#126 (comment).

Unfortunately we didn't fix it, it was fixed upstream and we would need to dig up the patch ourselves :/

Any known problem for v8 8.1 backport to v13

I haven't checked, but does it make sense to backport to v13 at this point? v14 is coming out next month.

@gengjiawen
Copy link
Member

@mmarchini The symbol seems related to zlib, Can you try import this by @targos
nodejs/node-v8@061db16#diff-30246878345a874b2105006560a03f27R1620

@mmarchini
Copy link
Contributor Author

Ok, so I opened #32116 to upgrade directly to 8.1. We might still be able to backport to v13, I'm not sure (someone would need to work on backwards-compatible patches, but that would happen for 8.0 as well).

I'll close this since that pull request is in way better shape (commits were properly squashes, reworded, etc.).

@mmarchini mmarchini closed this Mar 6, 2020
@mmarchini mmarchini mentioned this pull request Apr 13, 2020
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants