Skip to content

Conversation

@targos
Copy link
Member

No description provided.

@targostargos 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-botnodejs-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
@targostargos 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-actionsgithub-actionsbot 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.

@targostargos added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 24, 2023
@github-actionsgithub-actionsbot 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
MemberAuthor

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.

@targostargos 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
MemberAuthor

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.

@targostargos added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 8, 2023
@github-actionsgithub-actionsbot 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

@targostargos added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 21, 2023
@github-actionsgithub-actionsbot 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
MemberAuthor

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

@targos
Copy link
MemberAuthor

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
MemberAuthor

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
MemberAuthor

/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
MemberAuthor

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

@targos
Copy link
MemberAuthor

@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
MemberAuthor

Working on landing #47251

@targostargos 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]>
@targostargos deleted the v8-112 branch April 5, 2024 09:54
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

buildIssues and PRs related to build files or the CI.help wantedIssues that need assistance from volunteers or PRs that need help to proceed.semver-majorPRs that contain breaking changes and should be released in the next major version.v8 engineIssues and PRs related to the V8 dependency.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@targos@nodejs-github-bot@pbo-linaro@StefanStojanovic