Debug build is broken (node_mksnapshot) #252
Comments
|
Note that I'm trying to create a debug build because the release build, while it can be created, is also deeply broken: |
|
On the release build, I get the following stack trace. Note that I reverted 6a4952f before building locally. |
|
/cc @LeszekSwirski |
|
Do you have a V8 commit range between those dates? My suspicion would be crbug.com/v8/14247 |
|
I can retry locally with V8 11.8.14, which includes https://chromium-review.googlesource.com/c/v8/v8/+/4762666 I'm just not sure whether I should keep the default value of |
|
You should set it to |
|
Thanks, the build is fixed! There's only one test that fails: |
|
I pushed the current state to canary-base (main repo) and canary (this repo) |
|
Does nodejs/node#49099 help with the crash (when v8_enable_extensible_ro_snapshot is set to true)? |
|
It seems to help a bit with the release build (which doesn't crash anymore), but:
test failures
I applied nodejs/node#49099 and commented the 
|
|
Actually, |
|
Some notes of what I found so far: About the And after applying the patch above, the See stack traceThis was because we were compiling the code cache in an unfinalized isolate in the snapshot builder. nodejs/node#49099 updated the code cache compilation routine to create the code cache from a deserialized isolate in the second pass. With that patch the
It was actually crashing with a stack trace similar to the one folded above, just that the test doesn't log stderr when it fails (and instead go ahead parsing the stdout which is empty if the process crashes) |
|
I have a repro at this branch (with This crashes when trying to deserialize the freshly built snapshot in order to build the code cache in And this crashes when just trying to deserialize the snapshot (without the code cache): Curiously enough it doesn't always fail, for example this still works locally for me: (this is basically what |
|
I already see the crash from #252 (comment) at step: (Also, node takes forever to build locally. It seems to be stuck on I suspect this is related to ro-promotion creating forward-refs (from one RO page to the next). Deserialization can't handle these yet since RO pages are created in order as we deserialize. Looking.. |
|
Yes, confirmed: index 603010ed09..91f6ecfc81 100644
--- a/deps/v8/src/snapshot/read-only-serializer.cc
+++ b/deps/v8/src/snapshot/read-only-serializer.cc
@@ -231,6 +231,23 @@ class EncodeRelocationsVisitor final : public ObjectVisitor {
// Encode:
ro::EncodedTagged encoded = Encode(isolate_, o.GetHeapObject());
+
+ {
+ BasicMemoryChunk* host_chunk = BasicMemoryChunk::FromAddress(slot.address());
+ int host_page_index = 0;
+ for (ReadOnlyPage* page : isolate_->heap()->read_only_space()->pages()) {
+ if (host_chunk == page) break;
+ ++host_page_index;
+ }
+ BasicMemoryChunk* target_chunk = BasicMemoryChunk::FromAddress(o.GetHeapObject().address());
+ int target_page_index = 0;
+ for (ReadOnlyPage* page : isolate_->heap()->read_only_space()->pages()) {
+ if (target_chunk == page) break;
+ ++target_page_index;
+ }
+ CHECK_LE(target_page_index, host_page_index);
+ }
+
memcpy(segment_->contents.get() + slot_offset, &encoded,
ro::EncodedTagged::kSize);... |
|
@joyeecheung, please test the fix at crrev.com/c/4788992. What's your workflow to patch in some V8 version on top of node patches? |
See also: github.com/nodejs/node-v8/issues/252. Since adding RO promotion, it no longer holds that RO objects only point at heap objects with smaller addresses than themselves (that property used to hold because we were very careful about the allocation order). Support forward-pointers to following pages in the deserializer by changing deserialization order to allocate all pages first, before deserializing and potentially relocating page contents. Bug: v8:13789 Change-Id: I697d6f8fd237fbd63579679625aa0b20fb5bdd65 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4788992 Reviewed-by: Olivier Flückiger <[email protected]> Commit-Queue: Jakob Linke <[email protected]> Auto-Submit: Jakob Linke <[email protected]> Commit-Queue: Dominik Inführ <[email protected]> Reviewed-by: Dominik Inführ <[email protected]> Cr-Commit-Position: refs/heads/main@{#89545}
|
I just tried locally with the V8 It's almost all good. Only one test failure with the debug build: |
|
Ah, right, SEA needs to build the code cache from an deserialized isolate too (it does things separately)
@schuay We have a utility to backport a V8 commit to a Node.js checkout: https://github.com/nodejs/node-core-utils/blob/main/docs/git-node.md#git-node-v8 Normally if I want to backport it to Node.js with a proper backport commit to open PR with, I do something like this since I already have a local V8 check out Or, if I am just testing a V8 change locally without the intention of doing a backport PR, I do not use |
|
Locally nodejs/node#49226 fixes test-single-executable-application-use-code-cache.js for me, pushed it with #252 (comment) to https://github.com/joyeecheung/node/tree/ro-code-cache |
PR-URL: #49226 Refs: nodejs/node-v8#252 Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]>
V8 now requires code cache to be compiled from an isolate with the same RO space layout as the one that's going to deserialize the cache, so for a binary built with snapshot, we need to compile the code cache using a deserialized isolate. Drive-by: ignore "useCodeCache" when "useSnapshot" is true because the compilation would've been done during build time anyway in that case, and print a warning for it. PR-URL: #49226 Refs: nodejs/node-v8#252 Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]>
|
This is fixed. Thanks everyone! |
PR-URL: #49226 Refs: nodejs/node-v8#252 Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]>
V8 now requires code cache to be compiled from an isolate with the same RO space layout as the one that's going to deserialize the cache, so for a binary built with snapshot, we need to compile the code cache using a deserialized isolate. Drive-by: ignore "useCodeCache" when "useSnapshot" is true because the compilation would've been done during build time anyway in that case, and print a warning for it. PR-URL: #49226 Refs: nodejs/node-v8#252 Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]>
Diff to remove C++20 feature:
Error:
/cc @nodejs/v8 @joyeecheung does it ring a bell?
The text was updated successfully, but these errors were encountered: