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 9.4 #39945

Closed
wants to merge 15 commits into from
Closed

deps: update V8 to 9.4 #39945

wants to merge 15 commits into from

Conversation

targos
Copy link
Member

@targos targos commented Aug 30, 2021

ETA: Tue, Sep 21, 2021

Blog post: https://v8.dev/blog/v8-release-94

@targos targos added 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. blocked PRs that are blocked by other issues or PRs. labels Aug 30, 2021
@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. labels Aug 30, 2021
@targos targos added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 31, 2021
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 31, 2021
@nodejs-github-bot

This comment has been minimized.

@targos
Copy link
Member Author

targos commented Aug 31, 2021

Cherry-picked v8/v8@bdcda72 to fix the Windows error.

@targos targos added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 31, 2021
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 31, 2021
@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@targos
Copy link
Member Author

targos commented Sep 4, 2021

We have a lot of test crashes in debug mode: https://ci.nodejs.org/job/node-test-commit-linux-containered/28505/nodes=ubuntu1804_sharedlibs_debug_x64/consoleFull

Example:

13:01:28 not ok 86 async-hooks/test-async-exec-resource-http
13:01:28   ---
13:01:28   duration_ms: 0.409
13:01:28   severity: crashed
13:01:28   exitcode: -5
13:01:28   stack: |-
13:01:28     
13:01:28     
13:01:28     #
13:01:28     # Fatal error in ../deps/v8/src/objects/js-array-buffer.cc, line 78
13:01:28     # Debug check failed: !backing_store->is_wasm_memory() && !backing_store->is_resizable() implies backing_store->byte_length() == backing_store->max_byte_length().
13:01:28     #
13:01:28     #
13:01:28     #
13:01:28     #FailureMessage Object: 0x7fff693ae770
13:01:28      1: 0x557958e218cb node::DumpBacktrace(_IO_FILE*) [out/Debug/node]
13:01:28      2: 0x557958fde55b  [out/Debug/node]
13:01:28      3: 0x557958fde57b  [out/Debug/node]
13:01:28      4: 0x55795a709b76 V8_Fatal(char const*, int, char const*, ...) [out/Debug/node]
13:01:28      5: 0x55795a709ba5  [out/Debug/node]
13:01:28      6: 0x5579597ab02f v8::internal::JSArrayBuffer::Attach(std::shared_ptr<v8::internal::BackingStore>) [out/Debug/node]
13:01:28      7: 0x5579597ab3a5 v8::internal::JSArrayBuffer::Setup(v8::internal::SharedFlag, v8::internal::ResizableFlag, std::shared_ptr<v8::internal::BackingStore>) [out/Debug/node]
13:01:28      8: 0x5579594de380 v8::internal::Factory::NewJSArrayBuffer(std::shared_ptr<v8::internal::BackingStore>, v8::internal::AllocationType) [out/Debug/node]
13:01:28      9: 0x5579591d5d35 v8::ArrayBuffer::New(v8::Isolate*, std::shared_ptr<v8::BackingStore>) [out/Debug/node]
13:01:28     10: 0x557958ed9b19 node::AllocatedBuffer::ToArrayBuffer() [out/Debug/node]
13:01:28     11: 0x557959073bc5 node::EmitToJSStreamListener::OnStreamRead(long, uv_buf_t const&) [out/Debug/node]
13:01:28     12: 0x557958e7b621 node::StreamResource::EmitRead(long, uv_buf_t const&) [out/Debug/node]
13:01:28     13: 0x55795907bc70 node::LibuvStreamWrap::OnUvRead(long, uv_buf_t const*) [out/Debug/node]
13:01:28     14: 0x55795907b84e  [out/Debug/node]
13:01:28     15: 0x55795907b87e  [out/Debug/node]
13:01:28     16: 0x557959f39e1d  [out/Debug/node]
13:01:28     17: 0x557959f3a10f  [out/Debug/node]
13:01:28     18: 0x557959f41438  [out/Debug/node]
13:01:28     19: 0x557959f2857a uv_run [out/Debug/node]
13:01:28     20: 0x557958db738f node::SpinEventLoop(node::Environment*) [out/Debug/node]
13:01:28     21: 0x557958f868fa node::NodeMainInstance::Run(int*, node::Environment*) [out/Debug/node]
13:01:28     22: 0x557958f8681b node::NodeMainInstance::Run(node::EnvSerializeInfo const*) [out/Debug/node]
13:01:28     23: 0x557958eae176 node::Start(int, char**) [out/Debug/node]
13:01:28     24: 0x55795a33bb72 main [out/Debug/node]
13:01:28     25: 0x7fc90df22bf7 __libc_start_main [/lib/x86_64-linux-gnu/libc.so.6]
13:01:28     26: 0x557958db011a _start [out/Debug/node]

@targos
Copy link
Member Author

targos commented Sep 4, 2021

The check that fails is: https://github.com/targos/node/blob/bfbf0b64551ee22acc30d5cd66d7f6b5eae97c04/deps/v8/src/objects/js-array-buffer.cc#L76-L78

  DCHECK_IMPLIES(
      !backing_store->is_wasm_memory() && !backing_store->is_resizable(),
      backing_store->byte_length() == backing_store->max_byte_length());

It's a new check in this version.

/cc @addaleax ?

@targos
Copy link
Member Author

targos commented Sep 4, 2021

Added in v8/v8@1e7effd

@addaleax
Copy link
Member

addaleax commented Sep 5, 2021

@targos I think https://github.com/v8/v8/blob/1e7effd113d791f2272160e0e41e048b39892dc6/src/objects/backing-store.cc#L778 is just missing a line for max_byte_length_ = new_byte_length, i.e. it’s a bug in the commit you referenced.

@targos
Copy link
Member Author

targos commented Sep 6, 2021

/cc @marjakh ^

@marjakh
Copy link
Contributor

marjakh commented Sep 6, 2021

Right, that seems to be the case. I'll fix this upstream, thanks for pointing this out! (Slightly concerning that our tests didn't catch this...)I

@marjakh
Copy link
Contributor

marjakh commented Sep 6, 2021

Looks like you already found https://chromium-review.googlesource.com/c/v8/v8/+/3141581

pull bot pushed a commit to Alan-love/v8 that referenced this pull request Sep 6, 2021
See nodejs/node#39945

Bug: v8:11111
Change-Id: I7271a907bf036c18a014b2a13ff5746db622456a
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3141581
Reviewed-by: Leszek Swirski <[email protected]>
Commit-Queue: Marja Hölttä <[email protected]>
Cr-Commit-Position: refs/heads/main@{#76670}
targos added a commit to targos/node that referenced this pull request Sep 6, 2021
Original commit message:

    [rab/gsab] Fix missing assignment in BackingStore::Reallocate

    See nodejs#39945

    Bug: v8:11111
    Change-Id: I7271a907bf036c18a014b2a13ff5746db622456a
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3141581
    Reviewed-by: Leszek Swirski <[email protected]>
    Commit-Queue: Marja Hölttä <[email protected]>
    Cr-Commit-Position: refs/heads/main@{#76670}

Refs: v8/v8@5681a65
@targos
Copy link
Member Author

targos commented Sep 6, 2021

Thanks for the quick fix!

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@targos targos removed the blocked PRs that are blocked by other issues or PRs. label Sep 17, 2021
targos added a commit to targos/node that referenced this pull request Sep 17, 2021
Original commit message:

    [rab/gsab] Fix missing assignment in BackingStore::Reallocate

    See nodejs#39945

    Bug: v8:11111
    Change-Id: I7271a907bf036c18a014b2a13ff5746db622456a
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3141581
    Reviewed-by: Leszek Swirski <[email protected]>
    Commit-Queue: Marja Hölttä <[email protected]>
    Cr-Commit-Position: refs/heads/main@{#76670}

Refs: v8/v8@5681a65
@nodejs-github-bot
Copy link
Collaborator

@AshCripps
Copy link
Member

ok the failing one is 11.2.something and the nearform is 11.6 so I think it might be fixed with an update - cant confirm right now as the macstadium ones picked the moment I began looking into to fall off the network. Also the release machine is running 11.0 so that may need an update

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

targos added a commit that referenced this pull request Oct 2, 2021
PR-URL: #39945
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
targos added a commit that referenced this pull request Oct 2, 2021
PR-URL: #39945
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
targos added a commit that referenced this pull request Oct 2, 2021
Major V8 updates are usually API/ABI incompatible with previous
versions. This commit adapts NODE_MODULE_VERSION for V8 9.4.

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

PR-URL: #39945
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
targos added a commit that referenced this pull request Oct 2, 2021
Original commit message:

    [platform] Fix compilation on 32-bit Windows

    Use `extern "C"` to declare the __readfsdword function.
    Fixes error C2732 in the Node.js CI.

    Bug: chromium:796644
    Change-Id: If261985e65bfdade53ce06ff28afe0e2db402f7a
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3086457
    Reviewed-by: Clemens Backes <[email protected]>
    Commit-Queue: Michaël Zasso <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#76259}

Refs: v8/v8@bdcda72

PR-URL: #39945
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
targos added a commit that referenced this pull request Oct 2, 2021
Original commit message:

    [rab/gsab] Fix missing assignment in BackingStore::Reallocate

    See #39945

    Bug: v8:11111
    Change-Id: I7271a907bf036c18a014b2a13ff5746db622456a
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3141581
    Reviewed-by: Leszek Swirski <[email protected]>
    Commit-Queue: Marja Hölttä <[email protected]>
    Cr-Commit-Position: refs/heads/main@{#76670}

Refs: v8/v8@5681a65

PR-URL: #39945
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
targos added a commit that referenced this pull request Oct 2, 2021
PR-URL: #39945
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@targos
Copy link
Member Author

targos commented Oct 2, 2021

Landed in 606bb52..fd44fe5

@targos targos closed this Oct 2, 2021
@targos targos deleted the v8-94 branch October 2, 2021 06:45
targos added a commit to targos/node that referenced this pull request Oct 2, 2021
Original commit message:

    [platform] Fix compilation on 32-bit Windows

    Use `extern "C"` to declare the __readfsdword function.
    Fixes error C2732 in the Node.js CI.

    Bug: chromium:796644
    Change-Id: If261985e65bfdade53ce06ff28afe0e2db402f7a
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3086457
    Reviewed-by: Clemens Backes <[email protected]>
    Commit-Queue: Michaël Zasso <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#76259}

Refs: v8/v8@bdcda72

PR-URL: nodejs#39945
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
targos added a commit to targos/node that referenced this pull request Oct 2, 2021
Original commit message:

    [rab/gsab] Fix missing assignment in BackingStore::Reallocate

    See nodejs#39945

    Bug: v8:11111
    Change-Id: I7271a907bf036c18a014b2a13ff5746db622456a
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3141581
    Reviewed-by: Leszek Swirski <[email protected]>
    Commit-Queue: Marja Hölttä <[email protected]>
    Cr-Commit-Position: refs/heads/main@{#76670}

Refs: v8/v8@5681a65

PR-URL: nodejs#39945
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
targos added a commit to targos/node that referenced this pull request Oct 2, 2021
PR-URL: nodejs#39945
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
targos added a commit to targos/node that referenced this pull request Oct 3, 2021
PR-URL: nodejs#39945
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
targos added a commit to targos/node that referenced this pull request Oct 4, 2021
Original commit message:

    [platform] Fix compilation on 32-bit Windows

    Use `extern "C"` to declare the __readfsdword function.
    Fixes error C2732 in the Node.js CI.

    Bug: chromium:796644
    Change-Id: If261985e65bfdade53ce06ff28afe0e2db402f7a
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3086457
    Reviewed-by: Clemens Backes <[email protected]>
    Commit-Queue: Michaël Zasso <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#76259}

Refs: v8/v8@bdcda72

Backport-PR-URL: nodejs#40285
PR-URL: nodejs#39945
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
targos added a commit to targos/node that referenced this pull request Oct 4, 2021
Original commit message:

    [rab/gsab] Fix missing assignment in BackingStore::Reallocate

    See nodejs#39945

    Bug: v8:11111
    Change-Id: I7271a907bf036c18a014b2a13ff5746db622456a
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3141581
    Reviewed-by: Leszek Swirski <[email protected]>
    Commit-Queue: Marja Hölttä <[email protected]>
    Cr-Commit-Position: refs/heads/main@{#76670}

Refs: v8/v8@5681a65

Backport-PR-URL: nodejs#40285
PR-URL: nodejs#39945
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
targos added a commit to targos/node that referenced this pull request Oct 4, 2021
Backport-PR-URL: nodejs#40285
PR-URL: nodejs#39945
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Michael Dawson <[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.