...
<rdar://problem/53570112>
Created attachment 374932 [details] proposed patch. Let's get some testing on the EWS first. I'm still in the process of perf testing the patch locally.
Created attachment 374938 [details] proposed patch.
Created attachment 374944 [details] proposed patch.
Comment on attachment 374944 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=374944&action=review r=me with some nits > Source/JavaScriptCore/ChangeLog:10 > + SlotVisitor::visitChildren(). If a zapped cell is detected, we we'll crash with grammar *we well*. May *we will* or *we'll*. > Source/JavaScriptCore/heap/HeapCell.h:51 > + // the rest of the cell header bits in tact for crash analysis uses. *intact*
Comment on attachment 374944 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=374944&action=review > Source/JavaScriptCore/heap/HeapCell.h:55 > + uint32_t* cellWords = reinterpret_cast_ptr<uint32_t*>(this); bitwise_cast > Source/JavaScriptCore/heap/HeapCell.h:60 > + bool isZapped() const { return !*reinterpret_cast_ptr<const uint32_t*>(this); } ditto > Source/JavaScriptCore/heap/SlotVisitor.cpp:293 > +#if PLATFORM(MAC) why not X86_64? > Source/JavaScriptCore/heap/SlotVisitor.cpp:852 > +NEVER_INLINE void SlotVisitor::reportZappedCellAndCrash(JSCell* cell) > +{ > + MarkedBlock::Handle* foundBlock = nullptr; > + uint32_t* cellWords = reinterpret_cast_ptr<uint32_t*>(this); > + > + uintptr_t cellAddress = bitwise_cast<uintptr_t>(cell); > + uintptr_t headerWord = cellWords[1]; > + uintptr_t zapReason = cellWords[2]; > + unsigned subspaceHash = 0; > + size_t cellSize = 0; > + > + m_heap.objectSpace().forEachBlock([&] (MarkedBlock::Handle* block) { > + if (block->contains(cell)) { > + foundBlock = block; > + return IterationStatus::Done; > + } > + return IterationStatus::Continue; > + }); > + > + if (foundBlock) { > + subspaceHash = bmalloc::stringHash(foundBlock->subspace()->name()); > + cellSize = foundBlock->cellSize(); > + } > + > + RELEASE_ASSERT(!cell->isZapped(), cellAddress, headerWord, zapReason, subspaceHash, cellSize); > +} adding all of this worries more than just crashing. Do we not think the crash alone is good enough? At least in the appendToMarkedStack case. If you do keep it, please: - use bitwise_cast - Use WTF's string hash instead of bmalloc
I didn't mean to clear Michael's r+, though I do have some hesitation. Last time we added a bunch of debugging code, we ended up in a worse place than just crashing.
Comment on attachment 374944 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=374944&action=review > Source/JavaScriptCore/heap/SlotVisitor.cpp:851 > + RELEASE_ASSERT(!cell->isZapped(), cellAddress, headerWord, zapReason, subspaceHash, cellSize); please make this CRASH_WITH_INFO instead. Just to avoid any chance someone raced and allocated over this.
Created attachment 374997 [details] proposed patch. I've applied the feedback from Michael and Saam, and added some additional tooling to dump subspace hashes.
Comment on attachment 374997 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=374997&action=review > Source/JavaScriptCore/heap/SlotVisitor.cpp:849 > +#endif // PLATFORM(MAC) I'll fix this comment before landing. Should be CPU(X86_64).
Comment on attachment 374997 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=374997&action=review r=me with a nit. > Source/JavaScriptCore/heap/SlotVisitor.cpp:834 > + m_heap.objectSpace().forEachBlock([&] (MarkedBlock::Handle* block) { This is potentially dangerous because m_blocks can be added by the main thread while the concurrent marker is working. But maybe, it does not matter much since the crash trace itself offers much more profitable information already. Can you add a comment about it with FIXME or invent something safer?
Thanks for the reviews. Preliminary perf testing results looks promising i.e. no significant change. I'm going to go ahead and land this. We can roll it out later if we find that it does regress performance.
Landed in r247886: <http://trac.webkit.org/r247886>.
[975/2287] Building CXX object Source/...sources/UnifiedSource-ee8a7a7a-7.cpp.o In file included from DerivedSources/JavaScriptCore/unified-sources/UnifiedSource-ee8a7a7a-7.cpp:2: /home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/heap/SlotVisitor.cpp: In member function ‘void JSC::SlotVisitor::reportZappedCellAndCrash(JSC::JSCell*)’: /home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/heap/SlotVisitor.cpp:828:15: warning: unused variable ‘cellAddress’ [-Wunused-variable] 828 | uintptr_t cellAddress = bitwise_cast<uintptr_t>(cell); | ^~~~~~~~~~~ /home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/heap/SlotVisitor.cpp:829:15: warning: unused variable ‘headerWord’ [-Wunused-variable] 829 | uintptr_t headerWord = cellWords[1]; | ^~~~~~~~~~ /home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/heap/SlotVisitor.cpp:830:15: warning: unused variable ‘zapReason’ [-Wunused-variable] 830 | uintptr_t zapReason = cellWords[2]; | ^~~~~~~~~ /home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/heap/SlotVisitor.cpp:831:14: warning: variable ‘subspaceHash’ set but not used [-Wunused-but-set-variable] 831 | unsigned subspaceHash = 0; | ^~~~~~~~~~~~ /home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/heap/SlotVisitor.cpp:832:12: warning: variable ‘cellSize’ set but not used [-Wunused-but-set-variable] 832 | size_t cellSize = 0; | ^~~~~~~~ So this is kinda annoying to fix. The obvious solution is to just use the UNUSED_VARIABLE() macro for each of these in SlotVisitor.cpp. But that's annoying because the variables are passed to CRASH_WITH_INFO(), so one would expect CRASH_WITH_INFO() to use them. It does for Clang and MSVCC, but not for the fallback GCC implementation. And writing a macro that passes the __VA_ARGS__ to UNUSED_VARIABLE() doesn't seem simple. But I think we could create a macro that calls a variadic function and just does nothing with its va_list.
Re-opened since this is blocked by bug 200214
FYI, this patch has been rolled out in r247900: <https://trac.webkit.org/changeset/247900> because it regresses PLT5.
(In reply to Michael Catanzaro from comment #14) > The obvious solution is to just use the UNUSED_VARIABLE() macro for each of > these in SlotVisitor.cpp. But that's annoying because the variables are > passed to CRASH_WITH_INFO(), so one would expect CRASH_WITH_INFO() to use > them. It does for Clang and MSVCC, but not for the fallback GCC > implementation. And writing a macro that passes the __VA_ARGS__ to > UNUSED_VARIABLE() doesn't seem simple. But I think we could create a macro > that calls a variadic function and just does nothing with its va_list. FWIW, there's `WTF_LAZY_FOR_EACH_TERM` in wtf/Forward.h. I believe you could use `WTF_LAZY_FOR_EACH_TERM(UNUSED_VARIABLE, __VA_ARGS__)`
That seems good, thanks!
I'll fix the CRASH_WITH_INFO unused parameters issue separately in https://bugs.webkit.org/show_bug.cgi?id=200243.
Created attachment 375343 [details] proposed patch.
Attachment 375343 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/assembler/CPU.cpp:74: Use nullptr instead of NULL. [readability/null] [5] ERROR: Source/JavaScriptCore/assembler/CPU.cpp:84: Use nullptr instead of NULL. [readability/null] [5] Total errors found: 2 in 20 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 375343 [details] proposed patch. r=me
Created attachment 375346 [details] patch for landing.
Comment on attachment 375346 [details] patch for landing. Attachment 375346 [details] did not pass jsc-ews (mac): Output: https://webkit-queues.webkit.org/results/12849056 New failing tests: mozilla-tests.yaml/js1_5/Array/regress-101964.js.mozilla-ftl-eager-no-cjit-validate-phases
(In reply to Build Bot from comment #24) > Comment on attachment 375346 [details] > patch for landing. > > Attachment 375346 [details] did not pass jsc-ews (mac): > Output: https://webkit-queues.webkit.org/results/12849056 > > New failing tests: > mozilla-tests.yaml/js1_5/Array/regress-101964.js.mozilla-ftl-eager-no-cjit- > validate-phases This is unrelated. I ran this test on my slowest MacBookAir, and it runs just fine.
Landed in r248143: <http://trac.webkit.org/r248143>.
+ Build fix in r248162: <http://trac.webkit.org/r248162>.