deps: update V8 to 11.0 by targos · Pull Request #46125 · nodejs/node

targos · 2023-01-07T09:32:10Z

No description provided.

nodejs-github-bot · 2023-01-07T09:32:15Z

Review requested:

@nodejs/gyp
@nodejs/v8-update

nodejs-github-bot · 2023-01-07T09:37:24Z

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

targos · 2023-01-07T09:37:28Z

CI: https://ci.nodejs.org/job/node-test-pull-request/48888/
V8: https://ci.nodejs.org/job/node-test-commit-v8-linux/5110/

nodejs-github-bot · 2023-01-07T09:38:44Z

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=rhel8-s390x,v8test=v8test/5110/

nodejs-github-bot · 2023-01-07T09:40:04Z

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=rhel8-ppc64le,v8test=v8test/5110/

nodejs-github-bot · 2023-01-07T09:45:20Z

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=benchmark-ubuntu1804-intel-64,v8test=v8test/5110/

targos · 2023-01-07T10:19:05Z

Build error on Windows (debug):

10:45:39 C:\workspace\node-compile-windows-debug\node\deps\v8\src\execution\isolate-data.h(279,58): error C2607: static assertion failed [C:\workspace\node-compile-windows-debug\node\tools\v8_gypfiles\v8_init.vcxproj]
10:45:39 C:\workspace\node-compile-windows-debug\node\deps\v8\src\execution\isolate-data.h(281,55): error C2607: static assertion failed [C:\workspace\node-compile-windows-debug\node\tools\v8_gypfiles\v8_init.vcxproj]
10:45:39 C:\workspace\node-compile-windows-debug\node\deps\v8\src\execution\isolate-data.h(284,3): error C2607: static assertion failed [C:\workspace\node-compile-windows-debug\node\tools\v8_gypfiles\v8_init.vcxproj]
10:45:39 C:\workspace\node-compile-windows-debug\node\deps\v8\src\execution\isolate-data.h(286,37): error C2607: static assertion failed [C:\workspace\node-compile-windows-debug\node\tools\v8_gypfiles\v8_init.vcxproj]

targos · 2023-01-07T10:22:37Z

@nodejs/platform-windows @nodejs/platform-windows-arm ^

targos · 2023-01-07T15:57:33Z

We also still have nodejs/node-v8#246, that I don't know how to fix.

p-b-o · 2023-01-09T13:52:24Z

@targos After investigation, I could reproduce this issue on my side, during an native compilation on an arm64 machine.

This issue is not reproducible when building directly v8 (either in release, or debug). For now, I would recommend to remove those static asserts on node.js side:

diff --git a/deps/v8/src/execution/isolate-data.h b/deps/v8/src/execution/isolate-data.h
index 9886287fe0..0a2ca12f58 100644
--- a/deps/v8/src/execution/isolate-data.h
+++ b/deps/v8/src/execution/isolate-data.h
@@ -275,15 +275,6 @@ class IsolateData final {
// issues because of different compilers used for snapshot generator and
// actual V8 code.
void IsolateData::AssertPredictableLayout() {
- static_assert(std::is_standard_layout<RootsTable>::value);
- static_assert(std::is_standard_layout<ThreadLocalTop>::value);
- static_assert(std::is_standard_layout<ExternalReferenceTable>::value);
- static_assert(std::is_standard_layout<IsolateData>::value);
-#define V(Offset, Size, Name) \
- static_assert(offsetof(IsolateData, Name##_) == Offset);
- ISOLATE_DATA_FIELDS(V)
-#undef V
- static_assert(sizeof(IsolateData) == IsolateData::kSize);
}
#undef ISOLATE_DATA_FIELDS_POINTER_COMPRESSION

For the root cause, it seems that some expected structure are not standard layout type, which I don't have clue why it's the case only when compiling for arm64. A comment says breaking this property could bring trouble when using v8 snapshots, which I'm not sure if Node.js is using this now. Whatever, we can always do something if a regression appear with unit tests later.

This static_assert stuff has been around for quite some time in v8 code, so it's not new. If you want to nail down the exact root cause, you'll need to bisect which commit exactly introduced this regression in one of those structures, something I alas don't have the time to do for now.

targos · 2023-01-09T15:31:58Z

It's not specific to arm64. The Windows debug build is done on x64 in our CI.

p-b-o · 2023-01-09T15:36:10Z

Sorry, I thought it was an arm64 specific issue. I think you can safely delete or comment those static assert, and run (x64) CI to check everything is fine.

addaleax · 2023-01-09T15:58:26Z

comment says breaking this property could bring trouble when using v8 snapshots, which I'm not sure if Node.js is using this now.

We are. Fwiw, the comments read as saying that this would be an issue for cross-compilation specifically, not snapshot usage in general, so I’m not sure to what degree this applies to Node.js.

Whatever, we can always do something if a regression appear with unit tests later.

It shouldn’t be too hard to figure out why exactly this assertion is failing, and I’d probably do that before just skipping the assertions.

targos · 2023-01-09T16:12:46Z

I'm currently doing a bisect using V8 canary branches to narrow down the list of commits.
I don't know how to figure out why the assertion is failing.

p-b-o · 2023-01-09T16:16:57Z

I didn't see anything obvious either...

addaleax · 2023-01-09T16:38:25Z

I don't know how to figure out why the assertion is failing.

Unfortunately, I couldn’t reproduce this by compiling natively on Ubuntu 20.04 arm64. I think one way to go about this would be to:

Look at what the smallest struct is that fails this assertion – if the line numbers above are correct, that’s ThreadLocalTop
Duplicate that struct, can even be in the same header, under a different name
Add static_assert(std::is_standard_layout<DuplicatedThreadLocalTop>::value);
Run a failing compile command, repeatedly removing member fields from DuplicatedThreadLocalTop until the error goes away

And that should give you the member that’s making ThreadLocalTop have a non-standard layout. This might sound tricky, but it shouldn’t be too complex, as long as you locally have a reproduction of the issue.

targos · 2023-01-09T17:39:06Z

The error appears somewhere between 11.0.193 and 11.0.194: v8/v8@11.0.193...11.0.194

addaleax · 2023-01-09T17:43:56Z

@targos That’s fairly certainly v8/v8@d9aa68e then, with its addition of v8::heap::base::Stack::inactive_stacks_. I’m guessing std::vector<> doesn’t necessarily have standard layout (and/or not consist of exactly three pointer-sized members) here.

addaleax · 2023-01-09T17:55:08Z

If I’m right, something like this might be a workaround? It’s a bit hacky and probably can’t be upstreamed as-is, but it should definitely not break anything.

diff in the fold

diff --git a/deps/v8/src/heap/base/stack.cc b/deps/v8/src/heap/base/stack.cc
index d8e618d0c964..d3ea7885ca7c 100644
--- a/deps/v8/src/heap/base/stack.cc+++ b/deps/v8/src/heap/base/stack.cc@@ -172,8 +172,10 @@ void Stack::IteratePointers(StackVisitor* visitor) const {
visitor, reinterpret_cast<const void* const*>(context_->stack_marker),
stack_start_, asan_fake_stack);
- for (const auto& stack : inactive_stacks_) {- IteratePointersInStack(visitor, stack.top, stack.start, asan_fake_stack);+ if (inactive_stacks_) {+ for (const auto& stack : *inactive_stacks_) {+ IteratePointersInStack(visitor, stack.top, stack.start, asan_fake_stack);+ }
}
IterateUnsafeStackIfNecessary(visitor);
@@ -234,9 +236,12 @@ void Stack::ClearContext(bool check_invariant) {
void Stack::AddStackSegment(const void* start, const void* top) {
DCHECK_LE(top, start);
- inactive_stacks_.push_back({start, top});+ if (!inactive_stacks_) {+ inactive_stacks_ = std::make_unique<std::remove_reference_t<decltype(*inactive_stacks_)>>();+ }+ inactive_stacks_->push_back({start, top});
}
-void Stack::ClearStackSegments() { inactive_stacks_.clear(); }+void Stack::ClearStackSegments() { if (inactive_stacks_) inactive_stacks_->clear(); }
} // namespace heap::base
diff --git a/deps/v8/src/heap/base/stack.h b/deps/v8/src/heap/base/stack.h
index 06edf4cc41b3..cda759de51da 100644
--- a/deps/v8/src/heap/base/stack.h+++ b/deps/v8/src/heap/base/stack.h@@ -124,7 +124,7 @@ class V8_EXPORT_PRIVATE Stack final {
const void* start;
const void* top;
};
- std::vector<StackSegments> inactive_stacks_;+ std::unique_ptr<std::vector<StackSegments>> inactive_stacks_;
};
} // namespace heap::base

addaleax · 2023-01-09T17:58:35Z

@targos Actually, I forgot to include it you’ll probably want to adjust https://github.com/v8/v8/blob/d9aa68e85079eeaf8d50442f3bb5b22af01a0500/src/execution/thread-local-top.h#L33 to be the right number as well, but that can happen once we know that this workaround actually works

targos · 2023-01-09T17:58:51Z

https://ci.nodejs.org/job/node-compile-windows-debug/15106/nodes=win-vs2019/console

targos · 2023-01-09T18:05:42Z

It looks like ee3b0e5 fixed 2/4 errors. Is that expected?

targos · 2023-01-09T18:08:12Z

I guess the other asserts fail because I haven't updated kSizeInBytes...

targos · 2023-01-09T18:10:39Z

https://ci.nodejs.org/job/node-compile-windows-debug/15107/nodes=win-vs2019/console

targos · 2023-01-09T18:24:28Z

Looks good now! How do we handle this?

addaleax · 2023-01-10T11:19:43Z

@targos Idk … how do we usually handle this type of issue? My patch isn’t one that is particularly risky or that I’d be scared to float on top of this V8 version indefinitely, but it would be nice if upstream V8 did something here so we could eventually get rid of it. Does opening an upstream ticket make sense? (also cc @thibaudmichaud as the original author of that change)

nickie · 2023-01-29T01:46:03Z

For the first conflict, definitely the bottom.
The second one is a bit trickier, as I removed the saved_isolate from there in some other CL.
I'll take a better look at that and come back.

nickie · 2023-01-29T17:20:29Z

The bottom change was enough for both conflicts but there was one more change that was not merged well automatically in thread-local-top.cc (one line with stack_ needed to be removed).

I have everything here: crrev.com/c/4200636.

I checked the tests locally and on our bots, plus our four "node_ci" bots. Three of them seem to fail for a long time, so I assume they have nothing to do with this change. It seems that this works OK, but please try your tests before merging it.

Major V8 updates are usually API/ABI incompatible with previous versions. This commit adapts NODE_MODULE_VERSION for V8 11.0. Refs: https://github.com/nodejs/CTC/blob/master/meetings/2016-09-28.md

dllexport introduces issues when compiling with MSVC.

PR-URL: nodejs#45579 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Original commit message: [cppgc]: Fix build on msvc Fixes compilation with msvc 2019 toolchain. See: nodejs#37330 (comment) Bug: v8:12661 Change-Id: I7cfd87a3dd531a2e4913d82b743fb8ecdfdb5ed8 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3533019 Reviewed-by: Michael Lippautz <mlippautz@chromium.org> Commit-Queue: Michael Lippautz <mlippautz@chromium.org> Cr-Commit-Position: refs/heads/main@{#85087} Refs: v8/v8@166fd2f

Original commit message: [heap] Move the Stack object from ThreadLocalTop to Isolate Stack information is thread-specific and, until now, it was stored in a field in ThreadLocalTop. This CL moves stack information to the isolate and makes sure to update the stack start whenever a main thread enters the isolate. At the same time, the Stack object is refactored and simplified. As a side effect, after removing the Stack object, ThreadLocalTop satisfies the std::standard_layout trait; this fixes some issues observed with different C++ compilers. Bug: v8:13630 Bug: v8:13257 Change-Id: I026a35af3bc6999a09b21f277756d4454c086343 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4152476 Reviewed-by: Michael Lippautz <mlippautz@chromium.org> Reviewed-by: Omer Katz <omerkatz@chromium.org> Commit-Queue: Nikolaos Papaspyrou <nikolaos@chromium.org> Cr-Commit-Position: refs/heads/main@{#85445} Refs: v8/v8@1e4b71d Refs: https://chromium-review.googlesource.com/c/v8/v8/+/4200636 Co-Authored-By: Nikolaos Papaspyrou <nikolaos@chromium.org>

targos · 2023-01-30T11:00:58Z

Thanks a lot @nickie, I backported your changes!

nodejs-github-bot · 2023-01-30T11:03:23Z

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

nodejs-github-bot · 2023-01-30T11:03:44Z

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=rhel8-ppc64le,v8test=v8test/5144/

nodejs-github-bot · 2023-01-30T11:05:08Z

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=rhel8-s390x,v8test=v8test/5144/

nodejs-github-bot · 2023-01-31T01:47:07Z

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

nodejs-github-bot · 2023-01-31T07:23:12Z

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

targos · 2023-01-31T07:25:31Z

We still have the problem with test-worker-init-failure

nodejs/node-v8#246
V8 bug: https://bugs.chromium.org/p/v8/issues/detail?id=13493

gengjiawen · 2023-01-31T07:38:37Z

We still have the problem with test-worker-init-failure
nodejs/node-v8#246 V8 bug: bugs.chromium.org/p/v8/issues/detail?id=13493

Is the related test case marked as flaky? I don't see failure on CI.

targos · 2023-01-31T07:56:07Z

I don't think it's flaky. It failed in all CI runs (e.g. node-test-commit-arm-debug)

targos · 2023-01-31T15:16:53Z

It seems that this crash doesn't happen with canary anymore (since last week, two runs didn't fail). Since there's no rush to land a V8 update at the moment, I'll try to open another PR for V8 11.1.

nodejs-github-bot · 2023-02-02T13:32:31Z

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=benchmark-ubuntu1804-intel-64,v8test=v8test/5144/

targos added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jan 7, 2023

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 Jan 7, 2023

targosand others added 8 commits January 30, 2023 11:40

deps: update V8 to 11.0.226.12
09de414

build: reset embedder string to "-node.0"
49b6e0f

src: update NODE_MODULE_VERSION to 115
fd8c0ad
Major V8 updates are usually API/ABI incompatible with previous versions. This commit adapts NODE_MODULE_VERSION for V8 11.0. Refs: https://github.com/nodejs/CTC/blob/master/meetings/2016-09-28.md

deps: always define V8_EXPORT_PRIVATE as no-op
4c47427
dllexport introduces issues when compiling with MSVC.

deps: silence irrelevant V8 warning
e17583a
PR-URL: nodejs#45579 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

tools: update V8 gypfiles for 11.0
74ba7bc

targos force-pushed the v8-110 branch from b79702d to 74ba7bcCompare January 30, 2023 10:59

targos added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 30, 2023

github-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 30, 2023

gengjiawen approved these changes Jan 31, 2023
View reviewed changes

This comment was marked as outdated.
Sign in to view

gengjiawen self-requested a review January 31, 2023 07:39

targos mentioned this pull request Jan 31, 2023
deps: update V8 to 11.1 #46445
Closed

targos closed this Feb 24, 2023

targos deleted the v8-110 branch April 5, 2024 09:54

Uh oh!

Uh oh!

Conversation

targos commented Jan 7, 2023

Uh oh!

nodejs-github-bot commented Jan 7, 2023

Uh oh!

nodejs-github-bot commented Jan 7, 2023

Uh oh!

targos commented Jan 7, 2023

Uh oh!

nodejs-github-bot commented Jan 7, 2023

Uh oh!

nodejs-github-bot commented Jan 7, 2023

Uh oh!

nodejs-github-bot commented Jan 7, 2023

Uh oh!

targos commented Jan 7, 2023

Uh oh!

targos commented Jan 7, 2023

Uh oh!

targos commented Jan 7, 2023

Uh oh!

p-b-o commented Jan 9, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

targos commented Jan 9, 2023

Uh oh!

p-b-o commented Jan 9, 2023

Uh oh!

addaleax commented Jan 9, 2023

Uh oh!

targos commented Jan 9, 2023

Uh oh!

p-b-o commented Jan 9, 2023

Uh oh!

addaleax commented Jan 9, 2023

Uh oh!

targos commented Jan 9, 2023

Uh oh!

addaleax commented Jan 9, 2023

Uh oh!

addaleax commented Jan 9, 2023

Uh oh!

addaleax commented Jan 9, 2023

Uh oh!

targos commented Jan 9, 2023

Uh oh!

targos commented Jan 9, 2023

Uh oh!

targos commented Jan 9, 2023

Uh oh!

targos commented Jan 9, 2023

Uh oh!

targos commented Jan 9, 2023

Uh oh!

addaleax commented Jan 10, 2023

Uh oh!

nickie commented Jan 29, 2023

Uh oh!

nickie commented Jan 29, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

targos commented Jan 30, 2023

Uh oh!

nodejs-github-bot commented Jan 30, 2023

Uh oh!

nodejs-github-bot commented Jan 30, 2023

Uh oh!

nodejs-github-bot commented Jan 30, 2023

Uh oh!

nodejs-github-bot commented Jan 31, 2023

Uh oh!

nodejs-github-bot commented Jan 31, 2023

Uh oh!

targos commented Jan 31, 2023

Uh oh!

gengjiawen commented Jan 31, 2023

Uh oh!

This comment was marked as outdated.

p-b-o commented Jan 9, 2023•
edited
Loading

nickie commented Jan 29, 2023•
edited
Loading