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 11.2 #46815

Closed
wants to merge 8 commits into from
Closed

deps: update V8 to 11.2 #46815

wants to merge 8 commits into from

Conversation

targos
Copy link
Member

@targos targos commented Feb 24, 2023

No description provided.

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

Review requested:

  • @nodejs/gyp
  • @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 Feb 24, 2023
@targos targos added request-ci Add this label to start a Jenkins CI on a PR. and removed build Issues and PRs related to build files or the CI. v8 engine Issues and PRs related to the V8 dependency. needs-ci PRs that need a full CI run. labels Feb 24, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 24, 2023
@nodejs-github-bot

This comment was marked as outdated.

@targos targos added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 24, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 24, 2023
@nodejs-github-bot
Copy link
Collaborator

@targos
Copy link
Member Author

targos commented Feb 24, 2023

There's still the Windows arm64 failure we saw recently on canary:

15:19:53   handler-outside-simulator.cc
15:19:53 C:\workspace\node-compile-windows\node\deps\v8\src\trap-handler\trap-handler-simulator.h(38,5): error C2143: syntax error: missing ';' before 'asm' [C:\workspace\node-compile-windows\node\tools\v8_gypfiles\v8_base_without_compiler_host.vcxproj]
15:19:53 C:\workspace\node-compile-windows\node\deps\v8\src\trap-handler\trap-handler-simulator.h(38,44): error C2290: C++ 'asm' syntax ignored. Use __asm. [C:\workspace\node-compile-windows\node\tools\v8_gypfiles\v8_base_without_compiler_host.vcxproj]
15:19:53   simulator-arm64.cc
15:19:53 C:\workspace\node-compile-windows\node\deps\v8\src\trap-handler\trap-handler-simulator.h(38,5): error C2143: syntax error: missing ';' before 'asm' [C:\workspace\node-compile-windows\node\tools\v8_gypfiles\v8_base_without_compiler_host.vcxproj]
15:19:53 C:\workspace\node-compile-windows\node\deps\v8\src\trap-handler\trap-handler-simulator.h(38,44): error C2290: C++ 'asm' syntax ignored. Use __asm. [C:\workspace\node-compile-windows\node\tools\v8_gypfiles\v8_base_without_compiler_host.vcxproj]

@nodejs/platform-windows-arm PTAL

@pbo-linaro
Copy link
Contributor

@StefanStojanovic This would need you, as this is gonna block updating v8 later. An upstream patch, directly at v8, would be the best approach.

@targos targos added build Issues and PRs related to build files or the CI. v8 engine Issues and PRs related to the V8 dependency. labels Feb 24, 2023
@nodejs-github-bot
Copy link
Collaborator

@targos
Copy link
Member Author

targos commented Feb 26, 2023

There's also test.wpt/test-wasm-webapi that fails on ARM.

@StefanStojanovic
Copy link
Contributor

@StefanStojanovic This would need you, as this is gonna block updating v8 later. An upstream patch, directly at v8, would be the best approach.

Thanks for letting me know @pbo-linaro, I'll take a look after I'm finished with the tasks I'm working on currently.

@targos targos added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 8, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 8, 2023
@nodejs-github-bot
Copy link
Collaborator

@targos targos added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 21, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 21, 2023
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@StefanStojanovic
Copy link
Contributor

Good 👍. It's a bit dirty, and probably break "ProbeMemory" function, but from what I understood, nodeJS does not use all this "simulator" stuff, so it should not impact any user (or test).

For upstreaming that to V8, it would be needed to clean things a bit:

  • use __declspec(align) to align variables declared
  • declare symbol alias for Probe Memory

FYI, I'm working on this in V8 to open a CL upstream. I agree with all said here and I'll update this PR with a CL link when opened.

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@targos
Copy link
Member Author

targos commented Mar 22, 2023

test.wpt/test-wasm-webapi Still fails a lot because of timeouts.

@targos
Copy link
Member Author

targos commented Mar 22, 2023

There's definitely something wrong. I'm currently on test-equinix-debian10_container-armv7l-1.
Sometimes the test suite passes in less than two seconds, other times it hangs.

@targos
Copy link
Member Author

targos commented Mar 22, 2023

For now I've reduced the hang to:

$ out/Release/node test/wpt/test-wasm-webapi.js instantiateStreaming-bad-imports.any.js
---- Non-object imports argument: null ----
[PASS] Non-object imports argument: null
---- Non-object imports argument: true ----
[PASS] Non-object imports argument: true
---- Non-object imports argument: "" ----
[PASS] Non-object imports argument: ""
---- Non-object imports argument: symbol "Symbol()" ----
[PASS] Non-object imports argument: symbol "Symbol()"

@targos
Copy link
Member Author

targos commented Mar 22, 2023

/cc @tniessen since you implemented instantiateStreaming. Any idea how I could debug it further?

@StefanStojanovic
Copy link
Contributor

A quick update, I've opened CL with ARM64 fixes upstream https://chromium-review.googlesource.com/c/v8/v8/+/4359712

@pbo-linaro
Copy link
Contributor

Good work @StefanStojanovic, hope you'll get it accepted 👍

@targos
Copy link
Member Author

targos commented Mar 25, 2023

I opened #47251 in parallel. Maybe we're lucky and these wasm issues are fixed.

@targos
Copy link
Member Author

targos commented Mar 26, 2023

@bnoordhuis any idea?

nodejs-github-bot pushed a commit that referenced this pull request Mar 29, 2023
This test is flaky on ARM with V8 >= 11.2.
Skip it so we can update V8 before the release of Nodejs 20.0.0.

PR-URL: #47292
Refs: #46815
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Debadree Chatterjee <[email protected]>
Reviewed-By: Filip Skokan <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
@targos
Copy link
Member Author

targos commented Mar 31, 2023

Working on landing #47251

@targos targos closed this Mar 31, 2023
RafaelGSS pushed a commit that referenced this pull request Apr 5, 2023
This test is flaky on ARM with V8 >= 11.2.
Skip it so we can update V8 before the release of Nodejs 20.0.0.

PR-URL: #47292
Refs: #46815
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Debadree Chatterjee <[email protected]>
Reviewed-By: Filip Skokan <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
RafaelGSS pushed a commit that referenced this pull request Apr 6, 2023
This test is flaky on ARM with V8 >= 11.2.
Skip it so we can update V8 before the release of Nodejs 20.0.0.

PR-URL: #47292
Refs: #46815
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Debadree Chatterjee <[email protected]>
Reviewed-By: Filip Skokan <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
RafaelGSS pushed a commit that referenced this pull request Apr 7, 2023
This test is flaky on ARM with V8 >= 11.2.
Skip it so we can update V8 before the release of Nodejs 20.0.0.

PR-URL: #47292
Refs: #46815
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Debadree Chatterjee <[email protected]>
Reviewed-By: Filip Skokan <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
danielleadams pushed a commit that referenced this pull request Jul 6, 2023
This test is flaky on ARM with V8 >= 11.2.
Skip it so we can update V8 before the release of Nodejs 20.0.0.

PR-URL: #47292
Refs: #46815
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Debadree Chatterjee <[email protected]>
Reviewed-By: Filip Skokan <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
@targos targos deleted the v8-112 branch April 5, 2024 09:54
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. help wanted Issues that need assistance from volunteers or PRs that need help to proceed. 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.

4 participants