diff --git a/JSTests/stress/for-using-dispose-call-live-catch-locals-ftl-validation.js b/JSTests/stress/for-using-dispose-call-live-catch-locals-ftl-validation.js new file mode 100644 index 000000000000..bafb1cd08c88 --- /dev/null +++ b/JSTests/stress/for-using-dispose-call-live-catch-locals-ftl-validation.js @@ -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) { } +`); diff --git a/JSTests/stress/using-dispose-throw-after-body-throw-in-jit.js b/JSTests/stress/using-dispose-throw-after-body-throw-in-jit.js new file mode 100644 index 000000000000..88b729c15619 --- /dev/null +++ b/JSTests/stress/using-dispose-throw-after-body-throw-in-jit.js @@ -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) { + 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"); diff --git a/Source/JavaScriptCore/dfg/DFGLiveCatchVariablePreservationPhase.cpp b/Source/JavaScriptCore/dfg/DFGLiveCatchVariablePreservationPhase.cpp index afff25864867..dd82fe5554e7 100644 --- a/Source/JavaScriptCore/dfg/DFGLiveCatchVariablePreservationPhase.cpp +++ b/Source/JavaScriptCore/dfg/DFGLiveCatchVariablePreservationPhase.cpp @@ -120,6 +120,7 @@ class LiveCatchVariablePreservationPhase { HandlerInfo* cachedHandlerResult; CodeOrigin cachedCodeOrigin; + CodeOrigin cachedCatchOrigin; auto catchHandler = [&] (CodeOrigin origin) -> HandlerInfo* { ASSERT(origin); if (origin == cachedCodeOrigin) @@ -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; } @@ -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 currentBlockAccessData(OperandsLike, block->variablesAtTail, nullptr); auto flushEverything = [&] (NodeOrigin origin, unsigned index) { @@ -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(); + } } if (currentExceptionHandler && (node->op() == SetLocal || node->op() == SetArgumentDefinitely || node->op() == SetArgumentMaybe)) {