Review requested:

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

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

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

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

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

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

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]

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

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

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

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

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.

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.

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.

I didn't see anything obvious either...

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.

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

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

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

@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

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

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

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

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

Looks good now! How do we handle this?

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

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.

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.

Thanks a lot @nickie, I backported your changes!

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

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

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

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

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

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

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.

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

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.

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