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

deps: update V8 to 8.3 #32831

Closed
wants to merge 20 commits into from
Closed

deps: update V8 to 8.3 #32831

wants to merge 20 commits into from

Conversation

mmarchini
Copy link
Contributor

@mmarchini mmarchini commented Apr 13, 2020

Branch cutoff: April 2 2020
Chrome Beta coming Apr 16 - Apr 23
Chrome Stable May 19 2020

Checklist

@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 Apr 13, 2020
@nodejs-github-bot
Copy link
Collaborator

@mmarchini
Copy link
Contributor Author

Not running a V8 CI yet, because the V8 CI is currently broken.

@mmarchini
Copy link
Contributor Author

cc @nodejs/platform-freebsd @nodejs/platform-smartos build is failing on both platforms with similar error:

FreeBSD:

16:25:34 ../deps/v8/src/base/platform/platform-freebsd.cc:104:11: error: use of undeclared identifier 'pthread_attr_get_np'; did you mean 'pthread_attr_getscope'?
16:25:34   error = pthread_attr_get_np(pthread_self(), &attr);
16:25:34           ^~~~~~~~~~~~~~~~~~~
16:25:34           pthread_attr_getscope
16:25:34 /usr/include/pthread.h:312:6: note: 'pthread_attr_getscope' declared here
16:25:34 int             pthread_attr_getscope(const pthread_attr_t *, int *);
16:25:34                 ^
16:25:34 ../deps/v8/src/base/platform/platform-freebsd.cc:104:31: error: cannot initialize a parameter of type 'const pthread_attr_t *' (aka 'pthread_attr *const *') with an rvalue of type 'pthread_t' (aka 'pthread *')
16:25:34   error = pthread_attr_get_np(pthread_self(), &attr);
16:25:34                               ^~~~~~~~~~~~~~
16:25:34 /usr/include/pthread.h:312:50: note: passing argument to parameter here
16:25:34 int             pthread_attr_getscope(const pthread_attr_t *, int *);
16:25:34                                                             ^
16:25:34 2 errors generated.

SmartOS:

16:26:58 ../deps/v8/src/base/platform/platform-posix.cc: In static member function 'static void* v8::base::Stack::GetStackStart()':
16:26:58 ../deps/v8/src/base/platform/platform-posix.cc:978:15: error: 'pthread_getattr_np' was not declared in this scope
16:26:58    int error = pthread_getattr_np(pthread_self(), &attr);
16:26:58                ^~~~~~~~~~~~~~~~~~
16:26:58 ../deps/v8/src/base/platform/platform-posix.cc:978:15: note: suggested alternative: 'pthread_getname_np'
16:26:58    int error = pthread_getattr_np(pthread_self(), &attr);
16:26:58                ^~~~~~~~~~~~~~~~~~
16:26:58                pthread_getname_np
16:26:58 make[2]: *** [tools/v8_gypfiles/v8_libbase.target.mk:147: /home/iojs/build/workspace/node-test-commit-smartos/nodes/smartos18-64/out/Release/obj.target/v8_libbase/deps/v8/src/base/platform/platform-posix.o] Error 1

@targos
Copy link
Member

targos commented Apr 14, 2020

Diff in deps/v8/includes: https://gist.github.com/targos/196ca29dd29e719191fc70e02c5de68c

Is there anything breaking the ABI that we want to backport to 8.2 8.1 so it goes it out with Node.js 14?

@addaleax
Copy link
Member

@targos Yeah, there’s a number of ABI-breaking changes… do we ship 14.0.0 with V8 8.2?

@targos
Copy link
Member

targos commented Apr 14, 2020

@addaleax no, sorry I meant V8 8.1. The diff I posted is between current master (8.1) and the PR (8.3).

@targos
Copy link
Member

targos commented Apr 14, 2020

And my question is: are there ABI-breaking changes that we should rather somehow port to V8 8.1 before v14.0.0 is released because it will be cumbersome to revert them when we want to backport the upgrade to V8 8.3?

@mmarchini
Copy link
Contributor Author

If we get a forward patch for the ABI breaking changes this weekend, will there be enough time for it to make into v14?

@addaleax
Copy link
Member

And my question is: are there ABI-breaking changes that we should rather somehow port to V8 8.1 before v14.0.0 is released because it will be cumbersome to revert them when we want to backport the upgrade to V8 8.3?

Yes. Of the ABI-breaking changes, you’d want to patch them to be as close to V8 8.3 as possible, with as few exceptions as possible.

If we get a forward patch for the ABI breaking changes this weekend, will there be enough time for it to make into v14?

I should be able to put something like that together, if that’s desired. I guess whether that leaves enough time depends on what @BethGriggs says?

@BethGriggs
Copy link
Member

I can pull updates into the release up until Monday. I'd assume the update would be semver-major, so it'd also need no objections from TSC to land. I have slight concerns that this patch wouldn't have much time to live on master, but will defer judgement to those more familiar with V8 updates/internals. tldr; I can land the update if it's on master by Monday, assuming no objections from the TSC.

@addaleax
Copy link
Member

@BethGriggs That patch wouldn’t go on master, though (at least that’s what we’ve done in the past, I think). It would target v14.x-staging directly, and not be semver-major there, since no previous v14.x release exists.

@mmarchini
Copy link
Contributor Author

@addaleax let me know if there's anything I can help with the patch.

@cjihrig
Copy link
Contributor

cjihrig commented Apr 17, 2020

@mmarchini can you cherry-pick cjihrig@fb9e01d as a proposed fix for SmartOS. I started a CI run with that change, and the SmartOS build compiled and the tests passed.

@targos
Copy link
Member

targos commented May 4, 2020

@mmarchini V8 8.3 was officially announced today: https://v8.dev/blog/v8-release-83

Will you have time to update this? Otherwise I can try to do it tomorrow

@mmarchini
Copy link
Contributor Author

@targos I'll try to update it today, otherwise feel free to update tomorrow. (I definitely lost track of time and missed the start of May lol)

@mmarchini
Copy link
Contributor Author

@targos I won't have time to do it today. Feel free to do it tomorrow. If you don't have time I think I can get to it by the end of the week.

@targos
Copy link
Member

targos commented May 5, 2020

@nodejs/testing test.parallel/test-https-foafssl failed twice in a row. Is it a known flake?

Sorry, wrong thread

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented May 5, 2020

@targos
Copy link
Member

targos commented May 5, 2020

/cc @nodejs/platform-freebsd

09:46:16 ../deps/v8/src/base/platform/platform-freebsd.cc:104:11: error: use of undeclared identifier 'pthread_attr_get_np'; did you mean 'pthread_attr_getscope'?
09:46:16   error = pthread_attr_get_np(pthread_self(), &attr);
09:46:16           ^~~~~~~~~~~~~~~~~~~
09:46:16           pthread_attr_getscope
09:46:16 /usr/include/pthread.h:312:6: note: 'pthread_attr_getscope' declared here
09:46:16 int             pthread_attr_getscope(const pthread_attr_t *, int *);
09:46:16                 ^
09:46:16 ../deps/v8/src/base/platform/platform-freebsd.cc:104:31: error: cannot initialize a parameter of type 'const pthread_attr_t *' (aka 'pthread_attr *const *') with an rvalue of type 'pthread_t' (aka 'pthread *')
09:46:16   error = pthread_attr_get_np(pthread_self(), &attr);
09:46:16                               ^~~~~~~~~~~~~~
09:46:16 /usr/include/pthread.h:312:50: note: passing argument to parameter here
09:46:16 int             pthread_attr_getscope(const pthread_attr_t *, int *);
09:46:16                                                             ^
09:46:16 2 errors generated.
09:46:16 gmake[2]: *** [tools/v8_gypfiles/v8_libbase.target.mk:156: /usr/home/iojs/build/workspace/node-test-commit-freebsd/nodes/freebsd11-x64/out/Release/obj.target/v8_libbase/deps/v8/src/base/platform/platform-freebsd.o] Error 1

@targos
Copy link
Member

targos commented May 5, 2020

@nodejs/platform-ppc @nodejs/platform-ibmi @nodejs/platform-aix (I don't know which team is relevant to LinuxONE)

It looks like --interpreted-frames-native-stack is no longer supported by V8 on that platform: https://ci.nodejs.org/job/node-test-commit-linuxone/20771/nodes=rhel7-s390x/testReport/junit/(root)/test/parallel_test_cli_node_options/
Is it a known limitation?

@targos
Copy link
Member

targos commented May 5, 2020

Ah, I think that's @nodejs/platform-s390, sorry for the ping, other teams.

@targos
Copy link
Member

targos commented May 5, 2020

Refs: https://github.com/v8/v8/blob/843c8de8238184aa9fa856ca3673b457424c1457/src/flags/flag-definitions.h#L1641-L1651

The flag is now excluded for ARM and S390X.

Should I skip the test if process.arch === 's390x' ?

@miladfarca
Copy link
Contributor

miladfarca commented May 5, 2020

@targos yes s390 has the same issue Arm had experienced in which relative offsets in memory cannot exceed 32 bits, there are no instructions for supporting it.

targos added a commit that referenced this pull request May 26, 2020
Original commit message:

    [intl] Remove soon-to-be removed getAllFieldPositions

    Needed to land ICU67.1 soon.

    Bug: v8:10393
    Change-Id: I3c7737ca600d6ccfdc46ffaddfb318ce60bc7618
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2136489
    Reviewed-by: Jakob Kummerow <jkummerow@chromium.org>
    Commit-Queue: Frank Tang <ftang@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#67027}

Refs: v8/v8@3f8dc4b

Backport-PR-URL: #33376
PR-URL: #32831
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
targos added a commit that referenced this pull request May 26, 2020
Original commit message:

    [arraybuffer] Clean up BackingStore even if it pointer to nullptr

    For a zero-length BackingStore allocation, it is valid for the
    underlying memory to be a null pointer. However, some cleanup
    is still necessary, since the BackingStore may hold a reference
    to the allocator itself, which needs to be released when destroying
    the `BackingStore` instance.

    Change-Id: I1f168079d39e4592d2fde31fbe5f705586690e85
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2169646
    Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
    Commit-Queue: Ulan Degenbaev <ulan@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#67420}

Refs: v8/v8@e29c62b

Backport-PR-URL: #33376
PR-URL: #32831
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
targos added a commit that referenced this pull request May 26, 2020
Original commit message:

    FreeBSD: add missing include of pthread_np.h

    This is necessary for the pthread_attr_get_np function.

    Change-Id: I01cfe075a7c86909e8cf37eb7f7c5d44fa044975
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2188310
    Commit-Queue: Michaël Zasso <mic.besace@gmail.com>
    Commit-Queue: Ulan Degenbaev <ulan@chromium.org>
    Auto-Submit: Michaël Zasso <mic.besace@gmail.com>
    Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#67660}

Refs: v8/v8@74d50c5

Backport-PR-URL: #33376
PR-URL: #32831
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
targos pushed a commit that referenced this pull request May 26, 2020
Ref: https://chromium-review.googlesource.com/c/v8/v8/+/1997438
Ref: https://chromium-review.googlesource.com/c/v8/v8/+/2010107
Ref: nodejs/node-v8#144
Signed-off-by: Matheus Marchini <mmarchini@netflix.com>

Backport-PR-URL: #33376
PR-URL: #32831
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
targos added a commit that referenced this pull request May 26, 2020
V8 does not support the flag on this architecture anymore.

Backport-PR-URL: #33376
PR-URL: #32831
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@miladfarca
Copy link
Contributor

miladfarca commented Jun 1, 2020

@targos Hello, we need to re-enable the test skipped in this comment on s390, and to do so need to have these two CLs ported to nodejs:
https://chromium-review.googlesource.com/c/v8/v8/+/2196521
https://chromium-review.googlesource.com/c/v8/v8/+/2193911

Should I open a backport to nodejs master?

@targos
Copy link
Member

targos commented Jun 2, 2020

@miladfarca Hi. It depends on whether it's important to land the changes sooner on v14.x. #33579 already includes them but won't land in a release at least before July 14.

@miladfarca
Copy link
Contributor

@targos It think it would be safer to land them sooner, should I backport to master or v14.x-staging?

@targos
Copy link
Member

targos commented Jun 2, 2020

Master is fine

@miladfarca
Copy link
Contributor

@targos Thanks, here is the PR: #33702

AntonBikineev pushed a commit to AntonBikineev/cppgc that referenced this pull request Jun 23, 2020
This commit resolves compilation errors on SmartOS that
were found while upgrading Node.js.

See: nodejs/node#32831
Change-Id: Ia2a2e028ba4f5bfd69c050cab4fb4e13af5eefd9
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2191054
Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
Commit-Queue: Ulan Degenbaev <ulan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#67793}
AntonBikineev pushed a commit to AntonBikineev/cppgc that referenced this pull request Jun 24, 2020
This commit resolves compilation errors on SmartOS that
were found while upgrading Node.js.

See: nodejs/node#32831
Change-Id: Ia2a2e028ba4f5bfd69c050cab4fb4e13af5eefd9
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2191054
Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
Commit-Queue: Ulan Degenbaev <ulan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#67793}
@codebytere codebytere mentioned this pull request Jun 28, 2020
cjihrig pushed a commit to cjihrig/node that referenced this pull request Jul 4, 2020
Original commit message:

    Fix SmartOS compilation errors

    This commit resolves compilation errors on SmartOS that
    were found while upgrading Node.js.

    See: nodejs#32831
    Change-Id: Ia2a2e028ba4f5bfd69c050cab4fb4e13af5eefd9
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2191054
    Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
    Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
    Commit-Queue: Ulan Degenbaev <ulan@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#67793}

Refs: v8/v8@9868b2a
targos added a commit to targos/node that referenced this pull request Jul 13, 2020
Original commit message:

    Fix SmartOS compilation errors

    This commit resolves compilation errors on SmartOS that
    were found while upgrading Node.js.

    See: nodejs#32831
    Change-Id: Ia2a2e028ba4f5bfd69c050cab4fb4e13af5eefd9
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2191054
    Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
    Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
    Commit-Queue: Ulan Degenbaev <ulan@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#67793}

Refs: v8/v8@9868b2a
targos added a commit that referenced this pull request Jul 13, 2020
Original commit message:

    Fix SmartOS compilation errors

    This commit resolves compilation errors on SmartOS that
    were found while upgrading Node.js.

    See: #32831
    Change-Id: Ia2a2e028ba4f5bfd69c050cab4fb4e13af5eefd9
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2191054
    Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
    Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
    Commit-Queue: Ulan Degenbaev <ulan@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#67793}

Refs: v8/v8@9868b2a

PR-URL: #33579
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Shelley Vohr <codebytere@gmail.com>
targos added a commit to targos/node that referenced this pull request Jul 16, 2020
Original commit message:

    Fix SmartOS compilation errors

    This commit resolves compilation errors on SmartOS that
    were found while upgrading Node.js.

    See: nodejs#32831
    Change-Id: Ia2a2e028ba4f5bfd69c050cab4fb4e13af5eefd9
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2191054
    Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
    Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
    Commit-Queue: Ulan Degenbaev <ulan@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#67793}

Refs: v8/v8@9868b2a

PR-URL: nodejs#33579
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Shelley Vohr <codebytere@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jul 16, 2020
Original commit message:

    Fix SmartOS compilation errors

    This commit resolves compilation errors on SmartOS that
    were found while upgrading Node.js.

    See: #32831
    Change-Id: Ia2a2e028ba4f5bfd69c050cab4fb4e13af5eefd9
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2191054
    Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
    Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
    Commit-Queue: Ulan Degenbaev <ulan@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#67793}

Refs: v8/v8@9868b2a

Backport-PR-URL: #34356
PR-URL: #33579
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Shelley Vohr <codebytere@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jul 16, 2020
Original commit message:

    Fix SmartOS compilation errors

    This commit resolves compilation errors on SmartOS that
    were found while upgrading Node.js.

    See: #32831
    Change-Id: Ia2a2e028ba4f5bfd69c050cab4fb4e13af5eefd9
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2191054
    Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
    Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
    Commit-Queue: Ulan Degenbaev <ulan@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#67793}

Refs: v8/v8@9868b2a

Backport-PR-URL: #34356
PR-URL: #33579
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Shelley Vohr <codebytere@gmail.com>
targos pushed a commit to targos/abi-stable-v8 that referenced this pull request Apr 17, 2021
This commit resolves compilation errors on SmartOS that
were found while upgrading Node.js.

See: nodejs/node#32831
Change-Id: Ia2a2e028ba4f5bfd69c050cab4fb4e13af5eefd9
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2191054
Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
Commit-Queue: Ulan Degenbaev <ulan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#67793}
@targos targos removed their assignment Oct 31, 2021
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. semver-major PRs that contain breaking changes and should be released in the next major version. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

Successfully merging this pull request may close these issues.