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 12.7 #53651

Closed
wants to merge 22 commits into from
Closed

deps: update V8 to 12.7 #53651

wants to merge 22 commits into from

Conversation

targos
Copy link
Member

@targos targos commented Jun 30, 2024

@targos targos added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jun 30, 2024
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/gyp
  • @nodejs/security-wg
  • @nodejs/v8-update

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. needs-ci PRs that need a full CI run. v8 engine Issues and PRs related to the V8 dependency. labels Jun 30, 2024
@targos
Copy link
Member Author

targos commented Jun 30, 2024

ASan build error:

../deps/v8/src/common/code-memory-access.h:548:1: error: declaration of anonymous class must be a definition
class V8_NODISCARD V8_ALLOW_UNUSED NopRwxMemoryWriteScope final {
^
../deps/v8/src/common/code-memory-access.h:548:1: warning: declaration does not declare anything [-Wmissing-declarations]
1 warning and 1 error generated.

@anonrig
Copy link
Member

anonrig commented Jun 30, 2024

@targos where can i find the changelog for this release?

@gengjiawen
Copy link
Member

@targos where can i find the changelog for this release?

V8 no longer give release date anymore https://v8.dev/blog/discontinuing-release-posts

@gengjiawen
Copy link
Member

ASan build error:

../deps/v8/src/common/code-memory-access.h:548:1: error: declaration of anonymous class must be a definition
class V8_NODISCARD V8_ALLOW_UNUSED NopRwxMemoryWriteScope final {
^
../deps/v8/src/common/code-memory-access.h:548:1: warning: declaration does not declare anything [-Wmissing-declarations]
1 warning and 1 error generated.

Maybe give ubuntu 24 a try ? Clang 11 (latest is 18) maybe have compatible issues for new grammer.

@targos
Copy link
Member Author

targos commented Jul 1, 2024

@gengjiawen See #52374. TBH I will probably open a PR to disable test-asan as I'm not getting any help.

targos added a commit to targos/node that referenced this pull request Jul 14, 2024
It is running on ubuntu-20.04, which will inevitably be removed from
GitHub actions at some point. Attempts to upgrade it to ubuntu-22.04
and ubuntu-24.04 have failed.

It is now blocking V8 updates because of errors that happen only with
the `test-asan` job.

Refs: nodejs#52374
Refs: nodejs#53651 (comment)
nodejs-github-bot pushed a commit that referenced this pull request Jul 16, 2024
It is running on ubuntu-20.04, which will inevitably be removed from
GitHub actions at some point. Attempts to upgrade it to ubuntu-22.04
and ubuntu-24.04 have failed.

It is now blocking V8 updates because of errors that happen only with
the `test-asan` job.

Refs: #52374
Refs: #53651 (comment)
PR-URL: #53844
Reviewed-By: Marco Ippolito <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@targos targos added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 17, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 17, 2024
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@targos
Copy link
Member Author

targos commented Jul 17, 2024

Unfortunately this seems to be blocked on macOS system update:

10:00:55 In file included from Release/obj/gen/torque-generated/src/builtins/js-to-wasm-tq-csa.cc:1:
10:00:55 In file included from ../deps/v8/src/ast/ast.h:10:
10:00:55 In file included from ../deps/v8/src/ast/ast-value-factory.h:38:
10:00:55 In file included from ../deps/v8/src/objects/name.h:12:
10:00:55 In file included from ../deps/v8/src/objects/primitive-heap-object.h:8:
10:00:55 In file included from ../deps/v8/src/objects/heap-object.h:11:
10:00:55 In file included from ../deps/v8/src/objects/slots.h:11:
10:00:55 In file included from ../deps/v8/src/sandbox/external-pointer-table.h:13:
10:00:55 In file included from ../deps/v8/src/sandbox/compactible-external-entity-table.h:10:
10:00:55 In file included from ../deps/v8/src/sandbox/external-entity-table.h:15:
10:00:55 ../deps/v8/src/common/code-memory-access.h:548:1: error: declaration of anonymous class must be a definition
10:00:55 class V8_NODISCARD V8_ALLOW_UNUSED NopRwxMemoryWriteScope final {
10:00:55 ^
10:00:55 ../deps/v8/src/common/code-memory-access.h:548:1: warning: declaration does not declare anything [-Wmissing-declarations]
10:00:55 1 warning and 1 error generated.
10:00:55 make[2]: *** [/Users/iojs/build/workspace/node-test-commit-osx/nodes/osx11-x64/out/Release/obj.target/v8_initializers_slow/gen/torque-generated/src/builtins/js-to-wasm-tq-csa.o] Error 1

@targos
Copy link
Member Author

targos commented Jul 17, 2024

We have a lot of debug build crashes related to heap dump and heap snapshot: https://ci.nodejs.org/job/node-test-commit-arm-debug/13753/#showFailuresLink

@legendecas @joyeecheung

@targos
Copy link
Member Author

targos commented Jul 17, 2024

^ nodejs/node-v8#282

I will try to cherry-pick https://chromium-review.googlesource.com/c/v8/v8/+/5531355

Major V8 updates are usually API/ABI incompatible with previous
versions. This commit adapts NODE_MODULE_VERSION for V8 12.7.

Refs: https://github.com/nodejs/CTC/blob/master/meetings/2016-09-28.md
dllexport introduces issues when compiling with MSVC.

PR-URL: nodejs#47251
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
It introduces process hangs on some platforms because Node.js doesn't
tear down V8 correctly.
Disable it while we work on a solution.

Refs: nodejs#47297
Refs: https://bugs.chromium.org/p/v8/issues/detail?id=13902
PR-URL: nodejs#47450
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Refs: https://bugs.chromium.org/p/v8/issues/detail?id=14221
PR-URL: nodejs#49639
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
joyeecheung and others added 8 commits July 17, 2024 12:21
Original commit message:

    [arraybuffers] initialize max byte length of empty array buffers

    Without initializing the max byte length field, any empty array
    buffer captured in the snapshot can make the snapshot unreproducible.

    Refs: nodejs#53579

    Change-Id: I2489ab2e57ecbb405ec431a87d0acc92822b777c
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/5662576
    Reviewed-by: Marja Hölttä <[email protected]>
    Commit-Queue: Marja Hölttä <[email protected]>
    Cr-Commit-Position: refs/heads/main@{#94754}

Refs: v8/v8@e061cf9
PR-URL: nodejs#53755
Fixes: nodejs#53579
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Original commit message:

    [heap] Fix allocation tracker line end retrieval

    Tests in NodeJS with DCHECK enabled were failing because of two
    different problems:
    - One was that we were also disallowing heap allocations in the
    serialization stage. But NodeJS tests process the heap snapshot
    result in JS, so all those were broken.
    - But, also, the code that would retrieve from line ends would assume
    all the scripts populated line ends in the snapshot. But this was
    wrong. To fix it, this patch adds another storage of the line ends
    in the allocation tracker. This storage needs to keep weak references
    to the scripts so we do not leak the line ends data when scripts are
    disposed.

    This change keeps a weak reference to the scripts so, when they are
    freed, the line ends cache is also freed as it is not useful anymore.

    Node issue: nodejs/node-v8#282

    Change-Id: I4314d707903175f0005893e9aa06d3ae52fc57f8
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/5531355
    Commit-Queue: José Dapena Paz <[email protected]>
    Reviewed-by: Michael Lippautz <[email protected]>
    Reviewed-by: Simon Zünd <[email protected]>
    Cr-Commit-Position: refs/heads/main@{#94418}

Refs: v8/v8@058870c
As V8 has moved away from wrapper-descriptor-based CppHeap, this
patch:

1. Create the CppHeap without using wrapper descriptors.
2. Deprecates node::SetCppgcReference() in favor of
   v8::Object::Wrap() since the wrapper descriptor is no longer
   relevant. It is still kept as a compatibility layer for addons
   that need to also work on Node.js versions without
   v8::Object::Wrap().
@targos targos added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 17, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 17, 2024
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

ehsankhfr pushed a commit to ehsankhfr/node that referenced this pull request Jul 18, 2024
It is running on ubuntu-20.04, which will inevitably be removed from
GitHub actions at some point. Attempts to upgrade it to ubuntu-22.04
and ubuntu-24.04 have failed.

It is now blocking V8 updates because of errors that happen only with
the `test-asan` job.

Refs: nodejs#52374
Refs: nodejs#53651 (comment)
PR-URL: nodejs#53844
Reviewed-By: Marco Ippolito <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Add/remove abseil files introduced by V8 12.7 update found by:
```
git diff-tree --no-commit-id --name-status 0ec8f7e -r | grep '^[AD].*abseil.*'
```
@nodejs-github-bot
Copy link
Collaborator

@richardlau
Copy link
Member

richardlau commented Jul 18, 2024

I pushed a8deb42 which looks like it will fix the AIX build (missing abseil files from our gyp file). Feel free to squash/reword as necessary.

(I only looked at abseil as that was what was broken on AIX -- I haven't validated whether anything else is missing from the gypfile from the V8 update.)

@richardlau
Copy link
Member

CI: https://ci.nodejs.org/job/node-test-pull-request/60432/

https://ci.nodejs.org/job/node-test-commit-aix/52565/nodes=aix72-ppc64/ failed with lots of EMLINK errors -- this was because the temp dir used by tests had too many entries in it (I'm wondering if there's some failure path that doesn't clear the temp dir and we've just accumulated too much). I cleared out the directory and reran https://ci.nodejs.org/job/node-test-commit-aix/52567/ which now passes.

@targos
Copy link
Member Author

targos commented Jul 19, 2024

My last commit should fix the linker errors related to zlib. I expect the next CI to only fail because of macOS.

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

targos added a commit that referenced this pull request Jul 28, 2024
It is running on ubuntu-20.04, which will inevitably be removed from
GitHub actions at some point. Attempts to upgrade it to ubuntu-22.04
and ubuntu-24.04 have failed.

It is now blocking V8 updates because of errors that happen only with
the `test-asan` job.

Refs: #52374
Refs: #53651 (comment)
PR-URL: #53844
Reviewed-By: Marco Ippolito <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@targos
Copy link
Member Author

targos commented Jul 28, 2024

#54077

@targos targos closed this Jul 28, 2024
marco-ippolito pushed a commit that referenced this pull request Aug 19, 2024
It is running on ubuntu-20.04, which will inevitably be removed from
GitHub actions at some point. Attempts to upgrade it to ubuntu-22.04
and ubuntu-24.04 have failed.

It is now blocking V8 updates because of errors that happen only with
the `test-asan` job.

Refs: #52374
Refs: #53651 (comment)
PR-URL: #53844
Reviewed-By: Marco Ippolito <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: James M Snell <[email protected]>
marco-ippolito pushed a commit that referenced this pull request Aug 19, 2024
It is running on ubuntu-20.04, which will inevitably be removed from
GitHub actions at some point. Attempts to upgrade it to ubuntu-22.04
and ubuntu-24.04 have failed.

It is now blocking V8 updates because of errors that happen only with
the `test-asan` job.

Refs: #52374
Refs: #53651 (comment)
PR-URL: #53844
Reviewed-By: Marco Ippolito <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: James M Snell <[email protected]>
marco-ippolito pushed a commit that referenced this pull request Aug 19, 2024
It is running on ubuntu-20.04, which will inevitably be removed from
GitHub actions at some point. Attempts to upgrade it to ubuntu-22.04
and ubuntu-24.04 have failed.

It is now blocking V8 updates because of errors that happen only with
the `test-asan` job.

Refs: #52374
Refs: #53651 (comment)
PR-URL: #53844
Reviewed-By: Marco Ippolito <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: James M Snell <[email protected]>
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. needs-ci PRs that need a full CI run. 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.

10 participants