Skip to content

[JSC] ConservativeRoots: no past-the-end butterfly slack for cells that cannot hold a butterfly (MarkedBlock rule + PreciseAllocation::contains) #398

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
dylan-conway merged 3 commits into from
Aug 9, 2026
+11 −4
Merged

[JSC] ConservativeRoots: no past-the-end butterfly slack for cells that cannot hold a butterfly (MarkedBlock rule + PreciseAllocation::contains) #398

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6cb6d08
[JSC] ConservativeRoots: only apply the past-the-end butterfly rule i…
dylan-conway Aug 9, 2026
e501c5c
[JSC] ConservativeRoots: no past-the-end slack for precise allocation…
dylan-conway Aug 9, 2026
9b999ae
[JSC] ConservativeRoots: keep the exact one-past-the-end pointer for …
dylan-conway Aug 9, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 11 additions & 4 deletions Source/JavaScriptCore/heap/ConservativeRoots.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,8 +117,14 @@
[] (PreciseAllocation** ptr) -> PreciseAllocation* { return *ptr; });
if (result) {
auto attemptLarge = [&] (PreciseAllocation* allocation) {
if (allocation->contains(pointer) && allocation->hasValidCell())
markFoundGCPointer(allocation->cell(), allocation->attributes().cellKind);
// contains() accepts up to sizeof(IndexingHeader) past the end for butterfly end
// pointers; for a cell that cannot carry an IndexingHeader accept at most one-past-the-end.
HeapCell::Kind kind = allocation->attributes().cellKind;
bool inBounds = mayHaveIndexingHeader(kind)
? allocation->contains(pointer)
: (allocation->aboveLowerBound(pointer) && pointer <= std::bit_cast<char*>(allocation->cell()) + allocation->cellSize());
if (inBounds && allocation->hasValidCell())
markFoundGCPointer(allocation->cell(), kind);
};

if (result > m_heap.objectSpace().preciseAllocationsForThisCollectionBegin())
Expand Down Expand Up @@ -186,9 +192,10 @@
return;

// Also, a butterfly could point at the end of an object plus sizeof(IndexingHeader). In that
// case, this is pointing to the object to the right of the one we should be marking.
if (candidate->candidateAtomNumber(alignedPointer) > 0 && pointer <= alignedPointer + sizeof(IndexingHeader))
// case, this is pointing to the object to the right of the one we should be marking. As with the
// previous-block case above, only blocks whose cells can carry an IndexingHeader have such pointers.
if (mayHaveIndexingHeader(cellKind) && candidate->candidateAtomNumber(alignedPointer) > 0 && pointer <= alignedPointer + sizeof(IndexingHeader))
tryPointer(alignedPointer - candidate->cellSize());

Check warning on line 198 in Source/JavaScriptCore/heap/ConservativeRoots.cpp

View check run for this annotation

Claude / Claude Code Review

MarkedBlock path drops exact one-past-the-end for non-butterfly cells; PreciseAllocation path keeps it 

Commit 9b999ae9d4 relaxed the PreciseAllocation branch to `<= end` because compiler-rewritten loops over trailing in-cell storage (DirectArguments, JSLexicalEnvironment, JSBigInt digits, large JSFinalObject inline storage) can materialise the exact one-past-the-end pointer as the only live reference — but that same rationale applies to those types when they live in a MarkedBlock with `objectSize == cellSize`, and here the new `mayHaveIndexingHeader(cellKind)` guard now drops that case (when the
Comment on lines +195 to 198

@claude claude Bot Aug 9, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Commit 9b999ae relaxed the PreciseAllocation branch to <= end because compiler-rewritten loops over trailing in-cell storage (DirectArguments, JSLexicalEnvironment, JSBigInt digits, large JSFinalObject inline storage) can materialise the exact one-past-the-end pointer as the only live reference — but that same rationale applies to those types when they live in a MarkedBlock with objectSize == cellSize, and here the new mayHaveIndexingHeader(cellKind) guard now drops that case (when the right neighbour is dead, tryPointer(alignedPointer - cellSize) used to mark the cell; now it's skipped). Consider allowing the exact-boundary case here too, e.g. (mayHaveIndexingHeader(cellKind) || pointer == alignedPointer) && ..., or add a comment explaining why the asymmetry vs. the PreciseAllocation path is intentional.

Extended reasoning...

What changed and where the asymmetry appears

Commit 9b999ae (the review-feedback follow-up on this PR) changed the PreciseAllocation non-butterfly bound from pointer < cell + cellSize to pointer <= cell + cellSize, with the explicit rationale:

compiler-rewritten loops over trailing in-cell storage (DirectArguments, JSLexicalEnvironment, JSBigInt digits, large JSFinalObject inline storage, Wasm instances) can legitimately materialise [the exact one-past-the-end pointer]. Only the +1..+8 IndexingHeader slack is butterfly-specific, so accept [cell, end] and reject (end, end+8].

That fix was applied only to the attemptLarge hunk. The MarkedBlock hunk at line 197 still gates the entire left-neighbour fallback on mayHaveIndexingHeader(cellKind), so for a non-butterfly block a stack word that lands exactly on cell + cellSize no longer marks cell.

Why the same rationale reaches the MarkedBlock path

The types listed in 9b999ae's message all live in MarkedBlocks when small enough. MarkedSpace's precise size classes cover every multiple of sizeStep = 16 up to preciseCutoff, so e.g. a DirectArguments or JSLexicalEnvironment whose allocationSize(N) is a multiple of 16 gets a block where objectSize == cellSize exactly. A C++-compiler-rewritten loop over its trailing storage can then hold only cell + cellSize — which is the start of the right-hand neighbour cell.

Step-by-step trace at line 197

Take a non-butterfly block (cellKind == JSCell), a live cell C at atom N, a dead cell at atom N+1, and a stack word pointer == start(N+1) == C + cellSize:

  1. isJSCellKind(cellKind) is true and pointer is atom-aligned, so tryPointer(pointer) runs on cell N+1. It's dead → isLive == false → returns false.
  2. alignedPointer = cellAlign(pointer) = start(N+1) = pointer. tryPointer(alignedPointer) again hits dead N+1 → returns false.
  3. Pre-PR: candidateAtomNumber(alignedPointer) > 0 && pointer <= alignedPointer + 8 → true → tryPointer(alignedPointer - cellSize) = tryPointer(C) → C is marked.
  4. Post-PR: mayHaveIndexingHeader(cellKind) is false → the whole condition short-circuits → C is not marked.

So this PR removes the case where the MarkedBlock path did handle the exact one-past-the-end pointer, while the review-feedback commit on the same PR asserts that case matters for the PreciseAllocation path.

Why the PR description's defence doesn't cover this

The PR description says "plain JSCells are never referenced by an end/past-the-end pointer as their only reference (JIT/LLInt keep the base)". That addresses JIT-generated code, but 9b999ae's rationale is about C++-compiler-rewritten loops over trailing in-cell storage — a different codepath, and one the author accepted as valid one commit later. Either that rationale applies to both allocation paths (in which case line 197 should also let pointer == alignedPointer through), or it applies to neither (in which case 9b999ae is unnecessary). Applying it to only one hunk looks like an oversight.

There is a plausible argument for keeping the asymmetry: accepting pointer == alignedPointer as a reference to the previous MarkedBlock cell reintroduces a false-positive path (a stale base pointer to a dead cell N+1 marks its live predecessor N) that PreciseAllocation doesn't have because precise cells have no adjacent neighbour. And the MarkedBlock one-past-the-end case was already unreliable pre-PR — a live right neighbour short-circuits at the isLive && !mayHaveIndexingHeader early-return before ever reaching the fallback. But if that tradeoff is the intent, it's worth a comment; "was already broken half the time" is a weak reason to remove the half that worked.

Suggested fix

if ((mayHaveIndexingHeader(cellKind) || pointer == alignedPointer)
    && candidate->candidateAtomNumber(alignedPointer) > 0
    && pointer <= alignedPointer + sizeof(IndexingHeader))
    tryPointer(alignedPointer - candidate->cellSize());

This mirrors the PreciseAllocation path's <= end: for non-butterfly blocks, only the exact boundary (pointer == alignedPointer, i.e. end of the left cell) triggers the fallback; the (end, end+8] IndexingHeader slack stays gated on mayHaveIndexingHeader.

Severity

Nit. The trigger is narrow (objectSize == cellSize exactly, C++ compiler drops the base pointer across a GC safepoint, right neighbour dead), there's no known repro (the Bun repro was end+8 in a PreciseAllocation), and the pre-PR MarkedBlock behaviour was already inconsistent. But it's exactly the class of feedback the author already acted on in 9b999ae for the other hunk, so worth either fixing symmetrically or documenting why not.

}

template<typename MarkHook>
Expand Down
Loading