[webkit-reviews] review requested: [Bug 200149] Add crash diagnostics for debugging unexpected zapped cells. : [Attachment 374944] proposed patch.

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has asked  for review:
Bug 200149: Add crash diagnostics for debugging unexpected zapped cells.
https://bugs.webkit.org/show_bug.cgi?id=200149

Attachment 374944: proposed patch.

https://bugs.webkit.org/attachment.cgi?id=374944&action=review




--- Comment #6 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 374944
  --> https://bugs.webkit.org/attachment.cgi?id=374944
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