| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
|
Review requested:
|
Sorry, something went wrong.
|
PS: @targos we need https://chromium-review.googlesource.com/c/v8/v8/+/6276706 to land to get rid of deprecated string WriteUtf8 function. |
Sorry, something went wrong.
|
@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 |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
Thanks for the heads up, I've sent a changeset to v8: https://chromium-review.googlesource.com/c/v8/v8/+/6286748 |
Sorry, something went wrong.
|
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). |
Sorry, something went wrong.
|
I have updated the list in the OP: #57114 (comment)
@nodejs/platform-smartos @nodejs/platform-arm @nodejs/platform-windows @nodejs/build @nodejs/tsc |
Sorry, something went wrong.
|
@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. |
Sorry, something went wrong.
|
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! |
Sorry, something went wrong.
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. |
Sorry, something went wrong.
The function AllocForBrotli prefixes the allocated memory with its size, and returns a pointer to the region after it. This pointer can however no longer be suitably aligned. Correct this by allocating the maximum of the the size of the size_t and the max alignment. On Arm 32bits the size_t is 4 bytes long, but the alignment is 8 for some NEON instructions. When Brotli is compiled with optimizations enabled newer GCC versions will use the NEON instructions and trigger a bus error killing node. see google/brotli#1159
|
CI: https://ci.nodejs.org/job/node-test-pull-request/66010/ |
Sorry, something went wrong.
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
Original commit message:
[execution] Respect isolate stack limit in GetCentralStackView
A stack limit can be set for each v8::Isolate. The limit size can be
greater than the one specified with --stack-size. `Heap::CollectGarbage`
should not crash due to a `CHECK` on `Isolate::IsOnCentralStack()` with
an isolate stack limit.
Refs: nodejs#57114
Fixed: 400996806
Bug: 42202153
Change-Id: I80d0826fcd6a64261b8d745f8f47aa096bc83fb8
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/6329659
Commit-Queue: Chengzhong Wu <cwu631@bloomberg.net>
Reviewed-by: Jakob Kummerow <jkummerow@chromium.org>
Cr-Commit-Position: refs/heads/main@{#99228}
Refs: v8/v8@c24df23
Co-authored-by: Michaël Zasso <targos@protonmail.com>
|
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 #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. |
Sorry, something went wrong.
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. |
Sorry, something went wrong.
| 'BUILDING_V8_SHARED', # Make V8_EXPORT visible. | ||
| ], | ||
| }], | ||
| ['node_target_type=="shared_library"', { |
There was a problem hiding this comment.
I think the convention is using node_shared=="true".
Sorry, something went wrong.
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? |
Sorry, something went wrong.
|
I would like the 13.6 PR to supersede this one but I don't know if it will be ready soon enough |
Sorry, something went wrong.
Can you run the CI on that so we can have a similar todo list like this PR? |
Sorry, something went wrong.
|
I could but I expect the CI to fail all across the board because of the error in node_mksnapshot |
Sorry, something went wrong.
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. |
Sorry, something went wrong.
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. |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
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 |
Sorry, something went wrong.
|
#57753 is now in a similar state. |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
Summary of current issues (I need help for all of them):
Notable changes:
@nodejs/v8-update @nodejs/tsc
Supersedes #56959