Skip to content

DFG: LiveCatchVariablePreservationPhase flushed the wrong handler's locals when one try range leads directly into another #417

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

Open
robobun wants to merge 1 commit into
base: main
Choose a base branch
from
+91 −10
Open

DFG: LiveCatchVariablePreservationPhase flushed the wrong handler's locals when one try range leads directly into another #417

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
16 changes: 16 additions & 0 deletions JSTests/stress/for-using-dispose-call-live-catch-locals-ftl-validation.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
//@ requireOptions("--useExplicitResourceManagement=1", "--useConcurrentJIT=0", "--validateGraph=1")

// Found by fuzzing. The FTL compile of this eval code used to fail OSR
// availability validation ("Live bytecode local not available") on the local
// that the synthesized catch handler around the dispose call reads: the try
// range of that handler ends at a block boundary inside the enclosing for-of
// handler, and LiveCatchVariablePreservationPhase flushed the outer handler's
// locals instead of the inner handler's when it crossed from one to the other.

(0, eval)(`
const resource = { [Symbol.dispose]() { } };
for (using r of [resource]) {
try { r(); } catch (e) { }
}
for (let i = 0; i < 2000000; ++i) { }
`);
53 changes: 53 additions & 0 deletions JSTests/stress/using-dispose-throw-after-body-throw-in-jit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
//@ requireOptions("--useExplicitResourceManagement=1", "--useConcurrentJIT=0")

// The disposal code emitted for a `using` block keeps "did the body throw" in a
// local that is only read by the synthesized catch handler wrapped around the
// dispose call. That handler has never run by the time the function is
// optimized, so nothing in the compiled code reads the local and it is only
// kept alive for the exception exit of the dispose call. The try range of the
// synthesized catch ends right at that call, inside the range of the enclosing
// handler, so LiveCatchVariablePreservationPhase used to switch to the outer
// handler's liveness before flushing for the inner one and dropped the local.
// The exit then restored it as undefined and the body's error was lost instead
// of being reported through a SuppressedError.

function shouldBe(actual, expected) {
if (actual !== expected)
throw new Error(`Expected ${expected} but got ${actual}`);
}

const state = { throwOnDispose: false };

// Distinct executables so the dispose call site becomes megamorphic and stays a
// real call instead of being inlined (inlining splits the block and hides the bug).
const resources = [];
for (let i = 0; i < 16; ++i) {
resources.push({
state,
[Symbol.dispose]: new Function(`if (this.state.throwOnDispose) throw new Error("dispose ${i}");`),
});
}

function run(resource, bodyShouldThrow) {
try {
{
using r = resource;
if (bodyShouldThrow)
throw new Error("body");
}
} catch (e) {
return e;
}
return null;
}

for (let i = 0; i < 20000; ++i) {

Check warning on line 44 in JSTests/stress/using-dispose-throw-after-body-throw-in-jit.js

View check run for this annotation

Claude / Claude Code Review

New stress tests hardcode iteration counts instead of using testLoopCount 

Both new stress tests hardcode their warmup iteration counts (`20000` here and `2000000` at `for-using-dispose-call-live-catch-locals-ftl-validation.js:15`) instead of using `testLoopCount`. `JSTests/README.md` requires new tests to use `testLoopCount` so they tier up where that matters and stay under the 200ms budget in no-JIT configurations; it is a global on the jsc shell's GlobalObject, so it is visible inside indirect eval (or can be interpolated via `${testLoopCount}` since the eval string

@claude claude Bot Aug 18, 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.

🟡 Both new stress tests hardcode their warmup iteration counts (20000 here and 2000000 at for-using-dispose-call-live-catch-locals-ftl-validation.js:15) instead of using testLoopCount. JSTests/README.md requires new tests to use testLoopCount so they tier up where that matters and stay under the 200ms budget in no-JIT configurations; it is a global on the jsc shell's GlobalObject, so it is visible inside indirect eval (or can be interpolated via ${testLoopCount} since the eval string is already a template literal).

Extended reasoning...

What the issue is

JSTests/README.md (pulled into JSTests/CLAUDE.md via @README.md) states, under "Adding Tests":

New tests are required to adhere to the following rules:

  1. Tests must run in under 200ms in all configurations. …
  2. Use testLoopCount or wasmTestLoopCount to control how many iterations a test runs. The jsc CLI sets these based on the configuration of the test, so tests iterate enough to tier up where that matters and exit early where it doesn't.

Both new stress tests in this PR hardcode their tier-up iteration counts instead:

  • JSTests/stress/using-dispose-throw-after-body-throw-in-jit.js:44 — for (let i = 0; i < 20000; ++i)
  • JSTests/stress/for-using-dispose-call-live-catch-locals-ftl-validation.js:15 — for (let i = 0; i < 2000000; ++i) { } inside the eval string

Why testLoopCount applies here

testLoopCount is set by the jsc shell as a direct property on the global object (jsc.cpp:674, via putDirect) and is scaled per configuration (jsc.cpp:662-666): it defaults to max(10000, thresholdForFTLOptimizeAfterWarmUp() * 3) when the FTL is on, drops to ~1000 with --useDFGJIT=0, and to ~100 with --useBaselineJIT=0. Both tests already pass --useConcurrentJIT=0, so testLoopCount iterations are sufficient to reach the FTL in the configurations where that is possible.

For the eval test specifically: the loop lives inside (0, eval)(...), but indirect eval executes in the global scope, so the testLoopCount global is visible there. The eval string is also already a template literal, so ${testLoopCount} interpolation works if you prefer to bake the number in.

Step-by-step: why the hardcoded counts are a problem

Take for-using-dispose-call-live-catch-locals-ftl-validation.js under the no-jit variant that run-jsc-stress-tests runs for every stress test:

  1. run-jsc-stress-tests launches the test with --useBaselineJIT=0 (among other flags). The jsc shell computes testLoopCount = clampLoopCount(100, thresholdForJITAfterWarmUp() * 3) — on the order of 100.
  2. The test ignores that and runs for (let i = 0; i < 2000000; ++i) { } in the LLInt. Two million empty iterations in the interpreter is well over the 200ms budget from rule maybe upgrade #1, and none of it is useful because there is no FTL to compile the eval code in this configuration.
  3. Multiply across the several no-JIT / no-LLInt variants and this adds measurable dead time to every stress run.

using-dispose-throw-after-body-throw-in-jit.js is less extreme (20k iterations of a real function call), but the same reasoning applies: in the no-jit variant those 20k warmup calls never tier run() up, so the loop is doing nothing the test needs while still counting against the 200ms budget.

Why nothing else prevents this

Neither file has a //@ skip if … or //@ runDefault directive that would exclude the no-JIT variants, so run-jsc-stress-tests runs them in every configuration. The --useConcurrentJIT=0 / --validateGraph=1 options passed via //@ requireOptions do not change which variants run.

Impact

This is a test-hygiene / repository-convention issue, not a correctness bug in the DFG fix. The tests still reproduce the bug and still pass with the fix; they just spend more wall-clock than necessary in configurations where the JIT is disabled and diverge from the ~2200 existing stress tests that use testLoopCount.

Fix

// using-dispose-throw-after-body-throw-in-jit.js:44
for (let i = 0; i < testLoopCount; ++i) {

// for-using-dispose-call-live-catch-locals-ftl-validation.js:15
    for (let i = 0; i < ${testLoopCount}; ++i) { }
// or, since indirect eval sees globals:
    for (let i = 0; i < testLoopCount; ++i) { }

const error = run(resources[i % resources.length], i & 1);
shouldBe(error === null, !(i & 1));
}

state.throwOnDispose = true;
const error = run(resources[0], 1);
shouldBe(error instanceof SuppressedError, true);
shouldBe(error.error.message, "dispose 0");
shouldBe(error.suppressed.message, "body");
32 changes: 22 additions & 10 deletions Source/JavaScriptCore/dfg/DFGLiveCatchVariablePreservationPhase.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,7 @@ class LiveCatchVariablePreservationPhase {

HandlerInfo* cachedHandlerResult;
CodeOrigin cachedCodeOrigin;
CodeOrigin cachedCatchOrigin;
auto catchHandler = [&] (CodeOrigin origin) -> HandlerInfo* {
ASSERT(origin);
if (origin == cachedCodeOrigin)
Expand All @@ -133,13 +134,7 @@ class LiveCatchVariablePreservationPhase {
InlineCallFrame* inlineCallFrame = origin.inlineCallFrame();
CodeBlock* codeBlock = m_graph.baselineCodeBlockFor(inlineCallFrame);
if (HandlerInfo* handler = codeBlock->handlerForBytecodeIndex(bytecodeIndexToCheck)) {
liveAtCatchHead.fill(false);

BytecodeIndex catchBytecodeIndex = BytecodeIndex(handler->target);
m_graph.forAllLocalsAndTmpsLiveInBytecode(CodeOrigin(catchBytecodeIndex, inlineCallFrame), [&] (Operand operand) {
liveAtCatchHead.operand(operand) = true;
});

cachedCatchOrigin = CodeOrigin(BytecodeIndex(handler->target), inlineCallFrame);
cachedHandlerResult = handler;
break;
}
Expand All @@ -156,6 +151,14 @@ class LiveCatchVariablePreservationPhase {
return cachedHandlerResult;
};

// Liveness at the head of the handler most recently returned by catchHandler().
auto computeLiveAtCatchHead = [&] {
liveAtCatchHead.fill(false);
m_graph.forAllLocalsAndTmpsLiveInBytecode(cachedCatchOrigin, [&] (Operand operand) {
liveAtCatchHead.operand(operand) = true;
});
};

Operands<VariableAccessData*> currentBlockAccessData(OperandsLike, block->variablesAtTail, nullptr);

auto flushEverything = [&] (NodeOrigin origin, unsigned index) {
Expand Down Expand Up @@ -188,9 +191,18 @@ class LiveCatchVariablePreservationPhase {

{
HandlerInfo* newHandler = catchHandler(node->origin.semantic);
if (newHandler != currentExceptionHandler && currentExceptionHandler)
flushEverything(node->origin, nodeIndex);
currentExceptionHandler = newHandler;
if (newHandler != currentExceptionHandler) {
// liveAtCatchHead still describes the handler we are leaving. Flush for it before
// switching over to the liveness of the handler we are entering, otherwise a
// transition straight from one handler into another (e.g. leaving a try range that
// is nested inside another one) flushes the outer handler's locals instead of the
// inner handler's.
if (currentExceptionHandler)
flushEverything(node->origin, nodeIndex);
currentExceptionHandler = newHandler;
if (newHandler)
computeLiveAtCatchHead();
}
Comment on lines +194 to +205

@claude claude Bot Aug 17, 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.

🔴 The transition is now keyed on the HandlerInfo* pointer alone, so computeLiveAtCatchHead() is skipped when the same handler entry is reached via a different inlineCallFrame — recursive inlining puts the same baseline CodeBlock at two depths, so the same try range returns the identical HandlerInfo* for both, but the catch head's CodeOrigin (and hence its operand liveness) differs. The pre-PR code refreshed liveAtCatchHead on every cache-miss lookup so it stayed in sync for this case; keying the transition on cachedCatchOrigin (or the (HandlerInfo*, InlineCallFrame*) pair) instead of the pointer alone restores that and also makes the transition flush fire, which is what you want anyway.

Extended reasoning...

What changed. Before this PR, catchHandler() refilled liveAtCatchHead inline on every cache-miss lookup that found a handler. After this PR the refill is factored into computeLiveAtCatchHead() and gated on newHandler != currentExceptionHandler — a HandlerInfo* pointer comparison. cachedCatchOrigin is still updated inside catchHandler() on every cache miss, but liveAtCatchHead is only recomputed when the caller sees the pointer change.

Why the pointer can stay the same while the catch origin changes. handlerForBytecodeIndex() returns a pointer into the baseline CodeBlock's m_rareData->m_exceptionHandlers. With recursive inlining (maximumInliningRecursion defaults to 2, and functions containing op_catch are inlinable — DFGByteCodeParser handles inlined op_catch at ~line 9774 rather than refusing), the outer and inner inline frames share the same baseline CodeBlock*, so the same try range yields the identical HandlerInfo* at both depths. But the catch head's CodeOrigin differs by InlineCallFrame*, and forAllLocalsAndTmpsLiveInBytecode() remaps each local through inlineCallFrame->stackOffset and walks the caller chain, so {handler->target, ICF_outer} and {handler->target, ICF_inner} produce different operand bitmaps.

Step-by-step trace. Take function f() { try { f(); } catch {} } with the recursive call inlined once. inlineCall() does not allocate a new block at callee entry (parseCodeBlock() reuses m_currentBlock on its first iteration), and a try-range start is not a jump target, so one DFG block can contain, in order:

  1. Outer-frame try-body nodes at {bc#X, ICF=null}. catchHandler finds H in the root baseline block; cachedCatchOrigin = {H->target, null}. newHandler != currentExceptionHandler (null → H), so computeLiveAtCatchHead() runs and liveAtCatchHead describes the outer frame's locals.
  2. Inlined prologue / op_enter at {bc#0, innerICF}. bc#0 is outside every try range, so catchHandler walks up to the direct caller and again finds H with cachedCatchOrigin = {H->target, null}. Same pointer, no recompute — still correct.
  3. Inner-frame try-body nodes at {bc#Y, innerICF}. Cache miss; catchHandler finds H directly in the inner frame's baseline block (same CodeBlock* as the root) and sets cachedCatchOrigin = {H->target, innerICF}. But it returns the same H, so newHandler == currentExceptionHandler and computeLiveAtCatchHead() is skipped. liveAtCatchHead is still keyed to {H->target, null}; the inner frame's locals (mapped to higher machine-local indices via innerICF->stackOffset) are all false in it.

From step 3 onward, the SetLocal check at line 210 and the block-end flushEverything at line 228 both consult the stale outer-frame bitmap, so inner-frame locals live at the inner catch head get no Flush. An exception from the inner try body OSR-exits to the inner catch with those locals unavailable — exactly the failure class this PR is fixing (DFGOSRAvailabilityAnalysisPhase.cpp(198) assertion in debug, wrong restored value in release).

Regression vs. pre-PR. The old code recomputed liveAtCatchHead inside the lambda at step 3 regardless of whether the returned pointer changed, so the SetLocal path and the block-end flush saw the correct inner-frame bitmap. The pre-PR code already failed to flushEverything at a same-pointer transition — that gap is pre-existing and not what this comment is about — but it did keep liveAtCatchHead in sync for everything after the transition; the new code no longer does.

Fix. Key the transition on the catch CodeOrigin rather than the HandlerInfo* alone: e.g. remember cachedCatchOrigin before calling catchHandler() and compare the before/after values (treating the null-handler case as an invalid origin), or compare the (HandlerInfo*, InlineCallFrame*) pair. That also makes the mid-block flushEverything fire at the outer→inner boundary, which is the desired behaviour since the inner catch's live set is what the exception exit needs from that point on.

}

if (currentExceptionHandler && (node->op() == SetLocal || node->op() == SetArgumentDefinitely || node->op() == SetArgumentMaybe)) {
Expand Down
Loading