[help wanted] deps: update V8 to 13.4

[targos]

Summary of current issues (I need help for all of them):

• SmartOS build failures: https://ci.nodejs.org/job/node-test-commit-smartos/59914/
• Problem with --shared build: https://ci.nodejs.org/job/node-test-commit-linux-containered/49785/nodes=ubuntu2204_sharedlibs_shared_x64/console
• zlib/brotli test crashes on armv7l: https://ci.nodejs.org/job/node-test-commit-arm/57881/nodes=ubuntu2204-armv7l/
• armv7 cross-compiler build error: https://ci.nodejs.org/job/node-cross-compile/52730/nodes=cross-compiler-rhel9-armv7-gcc-12-glibc-2.28/console
• Test crashes on Windows Clang builds: https://ci.nodejs.org/job/node-test-binary-windows-js-suites/33347/RUN_SUBSET=0,nodes=win11-COMPILED_BY-vs2022_clang/testReport/junit/(root)/parallel/test_abortcontroller/
• V8 CI broken on x64: V8 CI broken on x64 build#4058

Notable changes:

• Atomics.pause
• WebAssembly Memory64
• Explicit resource management
• Error.isError

@nodejs/v8-update @nodejs/tsc

Supersedes #56959

[nodejs-github-bot]

Review requested:

• @nodejs/gyp
• @nodejs/security-wg
• @nodejs/tsc
• @nodejs/v8-update

[anonrig]

PS: @targos we need https://chromium-review.googlesource.com/c/v8/v8/+/6276706 to land to get rid of deprecated string WriteUtf8 function.

[targos]

@anonrig Since you told me that you wanted to exercise contributing to V8, I pushed a PoC of getting the V8 hash seed using an API instead of the guessing script that doesn't work anymore: f545e47
Do you think you can try to contribute something similar upstream?

[jasnell]

It is worth noting that while there are still a few outstanding bugs to deal with, the v8::IsolateGroup stuff should be available in 13.4. There is a memory leak that is being investigated now so it will likely need another patch or two or three before it is ready for use but my hope is that those are ready to go by the time this lands and we can start experimenting with v8::IsolateGroup and with enabling pointer compression by default.

[anonrig]

@anonrig Since you told me that you wanted to exercise contributing to V8, I pushed a PoC of getting the V8 hash seed using an API instead of the guessing script that doesn't work anymore: f545e47 Do you think you can try to contribute something similar upstream?

Thanks for the heads up, I've sent a changeset to v8: https://chromium-review.googlesource.com/c/v8/v8/+/6286748

[richardlau]

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

FWIW the mksnapshot crash on AIX is known and there's some discussion in https://chromium-review.googlesource.com/c/v8/v8/+/6261390 (that change was originally to try to get the builds building again on AIX while the cause of the crash is investigated).

[targos]

#57727 does not fix the zlib-brotli crashes

[targos]

I have updated the list in the OP: #57114 (comment)

• SmartOS builds are still failing
• We still have two issues with armv7
• There seems to be at least one consistent crash with a test on Windows compiled by Clang
• The --shared build fails to link libnode.so.136

@nodejs/platform-smartos @nodejs/platform-arm @nodejs/platform-windows @nodejs/build @nodejs/tsc

[danmcd]

@targos -- Reproduced illumos/SmartOS build failure locally, and it's nearly-or-exactly the same failure you're seeing in Jenkins, which I documented specifically earlier in this PR. The generation of node_snapshot.cc is failing due to an assertion check deep in the heart of node_mksnapshot (!isFreelistEntry() fails, so whatever it is, it IS in the free-list, I guess).

The resultant coredumps of last-time and now have slightly different stacks, but perhaps that's explained by code-changes between last time and now:

[root@nuc /zones/c5fc4aa5-3fac-455f-a95e-b685be69a78f/cores]# diff /tmp/{old,new}
3c3
< 0x30cd7e1()
---
> 0x30ce7e1()
18c18
< node::InitializePrimordials+0x120()
---
> node::InitializePrimordials+0x160()
20,22c20,24
< node::InitializePrimordials+0x90()
< node::NewContext+0xa0()
< node::CommonEnvironmentSetup::CommonEnvironmentSetup+0x634()
---
> node::Realm::CreateProperties+0x46()
> node::Environment::InitializeMainContext+0x11d()
> node::CreateEnvironment+0x8f()
> std::_Function_handler<node::Environment* +0x74()
> node::CommonEnvironmentSetup::CommonEnvironmentSetup+0x6a3()
[root@nuc /zones/c5fc4aa5-3fac-455f-a95e-b685be69a78f/cores]# 

I am happy to deep-dive assist in fixing this starting next week Monday. Can't offer earlier because it's both a Triton and SmartOS release week here.

[danmcd]

Tonight's $DAYJOB activities went well enough where I compiled with some v8 debugging enabled, mostly around symbol visibility (I should add CTF into the mix, but that's a bit difficult right this moment).

ANYWAY, I fond out that the local build (and likely Jenkins) is tripping the assertion at the very end of SetCodeAndEntrypointPointer. This is a function that used to not assert() at the end before this changeset, and also seems to weaken a guarantee about cas ordering.

I don't know enough about v8 itself to know if I'm missing a forest for the trees, however. Stay tuned!

[StefanStojanovic]

• There seems to be at least one consistent crash with a test on Windows compiled by Clang

About this - I've just rerun the failed test job, and it passed, plus I cannot reproduce it locally. It is potentially a flaky test with a higher chance for flakiness than usual, but it doesn't; look like a constant crashing test.

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[danmcd]

illumos/SmartOS update. It's kinda long.

TL;DR ==> Found a spot with DCHECK(!isFreelistEntry()) FAILING in void JSDispatchEntry::SetCodeAndEntrypointPointer, i.e. it apparently IS a Free List entry when it shouldn't be. Gonna keep pounding my head, but if folks with deep-v8-knowledge ( @targos ?) could dispense some clues, it'll reduce the head injuries.

. . .

So if I configure the build with:

./configure --v8-non-optimized-debug --v8-with-dchecks --v8-enable-object-print --gdb

I get a coredump of node_mksnapshot in this function:

void JSDispatchEntry::SetCodeAndEntrypointPointer(Address new_object,
                                                  Address new_entrypoint) {
  Address old_payload = encoded_word_.load(std::memory_order_relaxed);
  Address marking_bit = old_payload & kMarkingBit;
  Address parameter_count = old_payload & kParameterCountMask;
  // We want to preserve the marking bit of the entry. Since that happens to
  // be the tag bit of the pointer, we need to explicitly clear it here.
  Address object = (new_object << kObjectPointerShift) & ~kMarkingBit;
  Address new_payload = object | marking_bit | parameter_count;
  encoded_word_.store(new_payload, std::memory_order_relaxed);
  entrypoint_.store(new_entrypoint, std::memory_order_relaxed);
  DCHECK(!IsFreelistEntry()); // XXX KEBE SAYS RIGHT HERE, DCHECK().
}

I should probably remove --v8-with-dchecks and try again to see where vanilla CHECK() fails (likely same as Jenkins).

Due to multiple reasons my usual suite of debugging tools still fail me here. I'm gonna see if I can attach CTF data to this build and then I can unleash mdb on it. Speaking of debugging tools and mdb, I HAVE run this with illumos-inherited-from-Solaris libumem(3lib) with full debugging and didn't find anything leaky, use-after-free, or other such memory issues. I may use it with full LOGGING to follow all of the memory allocations in node_mksnapshot to narrow down things further (can do that on stock non-DEBUG builds too).

ALSO of interest in the build configured per above, I found a link-time warning in the build of node_mksnapshot:

  g++ -o /home/danmcd/targos-node-debug/out/Release/node_mksnapshot -rdynamic -pthreads -Wl,--whole-archive /home/danmcd/targos-node-debug/out/Release/obj.target/deps/zlib/libzlib.a -Wl,--no-whole-archive -Wl,--whole-archive /home/danmcd/targos-node-debug/out/Release/obj.target/deps/uv/libuv.a -Wl,--no-whole-archive -pthreads -m64 -Wl,-z,allextract  -Wl,--start-group /home/danmcd/targos-node-debug/out/Release/obj.target/node_mksnapshot/src/node_snapshot_stub.o /home/danmcd/targos-node-debug/out/Release/obj.target/node_mksnapshot/tools/snapshot/node_mksnapshot.o /home/danmcd/targos-node-debug/out/Release/obj.target/libnode.a /home/danmcd/targos-node-debug/out/Release/obj.target/deps/histogram/libhistogram.a /home/danmcd/targos-node-debug/out/Release/obj.target/deps/nbytes/libnbytes.a /home/danmcd/targos-node-debug/out/Release/obj.target/deps/ncrypto/libncrypto.a /home/danmcd/targos-node-debug/out/Release/obj.target/tools/v8_gypfiles/libv8_snapshot.a /home/danmcd/targos-node-debug/out/Release/obj.target/tools/v8_gypfiles/libv8_libplatform.a /home/danmcd/targos-node-debug/out/Release/obj.target/tools/icu/libicui18n.a /home/danmcd/targos-node-debug/out/Release/obj.target/deps/zlib/libzlib.a /home/danmcd/targos-node-debug/out/Release/obj.target/deps/llhttp/libllhttp.a /home/danmcd/targos-node-debug/out/Release/obj.target/deps/cares/libcares.a /home/danmcd/targos-node-debug/out/Release/obj.target/deps/uv/libuv.a /home/danmcd/targos-node-debug/out/Release/obj.target/deps/uvwasi/libuvwasi.a /home/danmcd/targos-node-debug/out/Release/obj.target/deps/nghttp2/libnghttp2.a /home/danmcd/targos-node-debug/out/Release/obj.target/deps/ada/libada.a /home/danmcd/targos-node-debug/out/Release/obj.target/deps/simdjson/libsimdjson.a /home/danmcd/targos-node-debug/out/Release/obj.target/tools/v8_gypfiles/libsimdutf.a /home/danmcd/targos-node-debug/out/Release/obj.target/deps/brotli/libbrotli.a /home/danmcd/targos-node-debug/out/Release/obj.target/deps/sqlite/libsqlite.a /home/danmcd/targos-node-debug/out/Release/obj.target/deps/zstd/libzstd.a /home/danmcd/targos-node-debug/out/Release/obj.target/deps/openssl/libopenssl.a /home/danmcd/targos-node-debug/out/Release/obj.target/tools/v8_gypfiles/libabseil.a /home/danmcd/targos-node-debug/out/Release/obj.target/deps/inspector_protocol/libcrdtp.a /home/danmcd/targos-node-debug/out/Release/obj.target/tools/icu/libicuucx.a /home/danmcd/targos-node-debug/out/Release/obj.target/tools/icu/libicudata.a /home/danmcd/targos-node-debug/out/Release/obj.target/tools/v8_gypfiles/libv8_base_without_compiler.a /home/danmcd/targos-node-debug/out/Release/obj.target/tools/v8_gypfiles/libv8_libbase.a /home/danmcd/targos-node-debug/out/Release/obj.target/tools/v8_gypfiles/libv8_zlib.a /home/danmcd/targos-node-debug/out/Release/obj.target/tools/v8_gypfiles/libhighway.a /home/danmcd/targos-node-debug/out/Release/obj.target/tools/v8_gypfiles/libv8_compiler.a /home/danmcd/targos-node-debug/out/Release/obj.target/tools/v8_gypfiles/libv8_initializers.a /home/danmcd/targos-node-debug/out/Release/obj.target/tools/v8_gypfiles/libv8_initializers_slow.a /home/danmcd/targos-node-debug/out/Release/obj.target/deps/zlib/libzlib_inflate_chunk_simd.a /home/danmcd/targos-node-debug/out/Release/obj.target/deps/zlib/libzlib_adler32_simd.a -lkstat -lumem -lsocket -lnsl -lrt -lm -lsendfile -Wl,--end-group
ld: warning: relocation warning: R_AMD64_64: file /home/danmcd/targos-node-debug/out/Release/obj.target/tools/v8_gypfiles/libv8_base_without_compiler.a(maglev-ir-x64.o): section [220].rela.rodata.cst8: symbol _ZNSt17_Function_handlerIFbN2v88internal11interpreter8RegisterEENS1_6maglev6detailUlS3_E0_EE9_M_invokeERKSt9_Any_dataOS3_: relocation against discarded COMDAT section [62].text._ZNSt17_Function_handlerIFbN2v88internal11interpreter8RegisterEENS1_6maglev6detailUlS3_E0_EE9_M_invokeERKSt9_Any_dataOS3_: symbol not found, relocation ignored

My demanglement on the coredump shows:

> ::dem _ZNSt17_Function_handlerIFbN2v88internal11interpreter8RegisterEENS1_6maglev6detailUlS3_E0_EE9_M_invokeERKSt9_Any_dataOS3_
_ZNSt17_Function_handlerIFbN2v88internal11interpreter8RegisterEENS1_6maglev6detailUlS3_E0_EE9_M_invokeERKSt9_Any_dataOS3_ == std::_Function_handler<bool 

This is likely a side-effect of one of the configuration options I picked above; the stock build shows no such link-time warning at the same place.

I'll continue with whatever trick I deem appropriate, and report back.

[danmcd]

I think I understand why we might have a problem in illumos.

I put some printfs in the failing function:

--- a/deps/v8/src/sandbox/js-dispatch-table-inl.h
+++ b/deps/v8/src/sandbox/js-dispatch-table-inl.h
@@ -176,6 +176,18 @@ void JSDispatchEntry::SetCodeAndEntrypointPointer(Address new_object,
   Address new_payload = object | marking_bit | parameter_count;
   encoded_word_.store(new_payload, std::memory_order_relaxed);
   entrypoint_.store(new_entrypoint, std::memory_order_relaxed);
+  // XXX KEBE SAYS this is where we fail.  Let's print a bunch of things?
+  if (IsFreelistEntry()) {
+         // XXX KEBE HOPES this doesn't mess anything up...
+         auto entrypoint = entrypoint_.load(std::memory_order_relaxed);
+         bool flret = (entrypoint & kFreeEntryTag) == kFreeEntryTag;
+
+         (void) printf("ABOUT TO CRASH!\n");
+         (void) printf("this = 0x%llx, entrypoint = 0x%llx, flret %d\n"
+             "kFreeEntryTag = 0x%x/0x%llx, andcheck 0x%x/0x%llx\n", this,
+             entrypoint, (int)flret, kFreeEntryTag, kFreeEntryTag,
+             (entrypoint & kFreeEntryTag), (entrypoint & kFreeEntryTag));
+  }
   DCHECK(!IsFreelistEntry());
 }

So now I can understand why IsFreelistEntry() succeeds, marking the entrypoint as a failure.

this = 0xfffffc7fe3e22c70, entrypoint = 0xfffffc7fbba00040, flret 1
kFreeEntryTag = 0x0/0xffff000000000000, andcheck 0x0/0xffff000000000000

Was it the intention to assume that the high 16 bits were always 0? That's not the case in the illumos heap allocator for 64-bit processes. We start at the high end of the virutal address space.

This code, BTW, is new with this PR. Some diffs showed me this bit in deps/v8/src/sandbox/js-dispatch-table.h
(I've annotated some of it inline. Look for KEBE speaking...)

#if defined(V8_TARGET_ARCH_64_BIT)
  // Freelist entries contain the index of the next free entry in their lower 32
  // bits and are tagged with this tag.
  // XXX KEBE ASKS DID YOU MEAN 0xffffULL ?!?
  static constexpr Address kFreeEntryTag = 0xffff000000000000ull;
#ifdef V8_TARGET_BIG_ENDIAN
  // 2-byte parameter count is on the least significant side of encoded_word_.
  static constexpr int kBigEndianParamCountOffset =
      sizeof(Address) - sizeof(uint16_t);
  static constexpr uintptr_t kParameterCountOffset =
      kCodeObjectOffset + kBigEndianParamCountOffset;
#else
  // XXX KEBE THINKS, maybe it was a typo above for "lower 32 bits"?
  static constexpr uintptr_t kParameterCountOffset = kCodeObjectOffset;
#endif  // V8_TARGET_BIG_ENDIAN
  static constexpr uint32_t kObjectPointerShift = 16;
  static constexpr uint32_t kParameterCountMask = 0xffff; 
#elif defined(V8_TARGET_ARCH_32_BIT)
  static constexpr uintptr_t kParameterCountOffset =
      kCodeObjectOffset + kSystemPointerSize;
  static constexpr uint32_t kObjectPointerShift = 0;
  static constexpr uint32_t kParameterCountMask = 0x0;
#else
#error "Unsupported Architecture"
#endif

I'll need to consult some other illumos folks to make sure I understand things, but I believe we allocate heap from the _userlimit/USERLIMIT downward, and USERLIMIT is set to 0xfffffc7fffe00000.

See https://github.com/illumos/illumos-gate/blob/master/usr/src/uts/i86pc/sys/machparam.h

I believe the current system in this PR won't work on illumos.

[bnoordhuis]

Was it the intention to assume that the high 16 bits were always 0?

Yes. I don't think V8 supports any architectures/platforms where that isn't currently the case.

I think you could change JSDispatchEntry::kFreeEntryTag to something like that 0xDEADBEEFull << 32 because the only requirement is that it's > 2**32 - or was when https://chromium-review.googlesource.com/c/v8/v8/+/5672594 was landed, but things may have changed since then.

[legendecas]

I think the convention is using node_shared=="true".

[danmcd]

Was it the intention to assume that the high 16 bits were always 0?

Yes. I don't think V8 supports any architectures/platforms where that isn't currently the case.

I think you could change JSDispatchEntry::kFreeEntryTag to something like that 0xDEADBEEFull << 32 because the only requirement is that it's > 2**32 - or was when https://chromium-review.googlesource.com/c/v8/v8/+/5672594 was landed, but things may have changed since then.

Yeah... I'd noticed in the main branch that kFreeEntryTag = 0xffff000000000000 was there, but gmake... on the main branch generates things Just Fine and goes all the way to completion.

Look at #57753 as I have more details there. Unless the 13.6 PR is not superceding this one?

[targos]

I would like the 13.6 PR to supersede this one but I don't know if it will be ready soon enough

[anonrig]

I would like the 13.6 PR to supersede this one but I don't know if it will be ready soon enough

Can you run the CI on that so we can have a similar todo list like this PR?

[targos]

I could but I expect the CI to fail all across the board because of the error in node_mksnapshot

[danmcd]

I would like the 13.6 PR to supersede this one but I don't know if it will be ready soon enough

IN THE MEANTIME I will see if my specially-modified illumos parameters (which made it to the same failure place in the 13.6 branch as @targos 's non-illumos builds did) work better with THIS branch. Stay tuned.

[danmcd]

IN THE MEANTIME I will see if my specially-modified illumos parameters (which made it to the same failure place in the 13.6 branch as @targos 's non-illumos builds did) work better with THIS branch. Stay tuned.

Well well well... on an illumos (yes specifically SmartOS) system with this kernel parameter changed:

* Keep top-16-bits of the available address space zeroed
set _userlimit=0x7fffc0000000

I managed to build this branch successfully. I'm going to focus on how to get v8's tagging to work in a world where most/all pointers have 0xffff as their high bits.

[danmcd]

Until the 13.6 bits start working for non-illumos folks, I've gone ahead with THIS branch to run two concurrent experiments.

Note both machines are running 20250403 SmartOS, and both are running the 24.4.1 pkgsrc LTS. These are ahead of the old-enough-to-not-have-modern-madvice() PI/kernel change and the 22.4 or 23.4 pkgsrc LTS the agents we currently offer this project (for older node builds) do.

The two experiments are:

1.) Using kernel tuning to blank-out the high 16 bits on userland pointers for amd64. This current succeeds in building node, but gmake test is producing coredumps, which I will drill into in a future post upon narrowing down to the first failure by not using parallel gmake.

2.) Using a stock kernel with USERLIMIT set to 0xfffffc7fffe00000, but with changes in V8 under review from the AIX team here: https://chromium-review.googlesource.com/c/v8/v8/+/6320599

with a couple of extra illumos mods. I'm still working out the kinks in these illumos mods, but my hope is to get it to as far as the kernel-tuned one, if not even further.

As I told the V8 folks, clean documentation of what their expectations of pointers are will help all OS vendors either solve them with runtime changes (there are ld(1) tricks that could be used, and if we don't have them yet, we could stand them up), or with feedback to any project (like I'm trying to do now).

Will report back with coredump information from the first experiment's gmake test, and once I get experiment 2 past gmake to build node, any different-from-experiment-1 results.

[danmcd]

Test 1 generated a lot of corefiles, but I suspect those were negative-tests of failure paths.

The summary of gmake test in the OS-hacked-address-range zone was:

[07:03|% 100|+ 4372|-   3]: Done                                              

Failed tests:
out/Release/node --test-reporter=./test/common/test-error-reporter.js --test-reporter-destination=stdout /home/danmcd/targos-v8-134/test/es-module/test-esm-detect-ambiguous.mjs
out/Release/node --test-reporter=./test/common/test-error-reporter.js --test-reporter-destination=stdout /home/danmcd/targos-v8-134/test/parallel/test-node-run.js
out/Release/node --test-reporter=./test/common/test-error-reporter.js --test-reporter-destination=stdout /home/danmcd/targos-v8-134/test/parallel/test-child-process-stdio-reuse-readable-stdio.js
gmake[1]: *** [Makefile:315: jstest] Error 1
gmake: *** [Makefile:342: test] Error 2

[targos]

#57753 is now in a similar state.