diff --git a/compiler/backend/src/org/jetbrains/kotlin/codegen/coroutines/CoroutineTransformerMethodVisitor.kt b/compiler/backend/src/org/jetbrains/kotlin/codegen/coroutines/CoroutineTransformerMethodVisitor.kt index 530698279d2..52049ef44cc 100644 --- a/compiler/backend/src/org/jetbrains/kotlin/codegen/coroutines/CoroutineTransformerMethodVisitor.kt +++ b/compiler/backend/src/org/jetbrains/kotlin/codegen/coroutines/CoroutineTransformerMethodVisitor.kt @@ -5,7 +5,6 @@ package org.jetbrains.kotlin.codegen.coroutines -import com.intellij.util.containers.Stack import org.jetbrains.kotlin.backend.common.CodegenUtil import org.jetbrains.kotlin.codegen.AsmUtil import org.jetbrains.kotlin.codegen.ClassBuilder @@ -118,7 +117,7 @@ class CoroutineTransformerMethodVisitor( ) if (examiner.allSuspensionPointsAreTailCalls(suspensionPoints)) { examiner.replacePopsBeforeSafeUnitInstancesWithCoroutineSuspendedChecks() - dropSuspensionMarkers(methodNode, suspensionPoints) + dropSuspensionMarkers(methodNode) return } @@ -138,10 +137,6 @@ class CoroutineTransformerMethodVisitor( // Actual max stack might be increased during the previous phases updateMaxStack(methodNode) - // Remove unreachable suspension points - // If we don't do this, then relevant frames will not be analyzed, that is unexpected from point of view of next steps (e.g. variable spilling) - removeUnreachableSuspensionPointsAndExitPoints(methodNode, suspensionPoints) - UninitializedStoresProcessor(methodNode, shouldPreserveClassInitialization).run() val spilledToVariableMapping = spillVariables(suspensionPoints, methodNode) @@ -191,7 +186,7 @@ class CoroutineTransformerMethodVisitor( }) } - dropSuspensionMarkers(methodNode, suspensionPoints) + dropSuspensionMarkers(methodNode) methodNode.removeEmptyCatchBlocks() // The parameters (and 'this') shall live throughout the method, otherwise, d8 emits warning about invalid debug info @@ -498,48 +493,65 @@ class CoroutineTransformerMethodVisitor( }) } - private fun removeUnreachableSuspensionPointsAndExitPoints(methodNode: MethodNode, suspensionPoints: MutableList) { - val dceResult = DeadCodeEliminationMethodTransformer().transformWithResult(containingClassInternalName, methodNode) + /* + * Every suspension point should be surrounded by two markers: before suspension point marker (start marker) + * and after suspension point marker (end marker) + * + * However, if suspension point comes from inline function and its end marker is unreachable, the end marker is removed by + * either inliner or bytecode optimization. + * + * If this happens, we should restore end marker. + * + * Since in both cases (when end marker is reachable and when it is not) all paths should lead to + * either a single end marker or to ATHROWs and ARETURNs, we just compute all paths from start marker until they reach + * these instructions. + */ + private fun collectSuspensionPoints(methodNode: MethodNode): List { + // Exception paths lead outside suspension points, thus we should ignore them + val cfg = ControlFlowGraph.build(methodNode, followExceptions = false) - // If the suspension call begin is alive and suspension call end is dead - // (e.g., an inlined suspend function call ends with throwing a exception -- see KT-15017), - // this is an exit point for the corresponding coroutine. - // It doesn't introduce an additional state to the corresponding coroutine's FSM. - suspensionPoints.forEach { - if (dceResult.isAlive(it.suspensionCallBegin) && dceResult.isRemoved(it.suspensionCallEnd)) { - it.removeBeforeSuspendMarker(methodNode) - } - } - - suspensionPoints.removeAll { dceResult.isRemoved(it.suspensionCallBegin) || dceResult.isRemoved(it.suspensionCallEnd) } - } - - private fun collectSuspensionPoints(methodNode: MethodNode): MutableList { - val suspensionPoints = mutableListOf() - val beforeSuspensionPointMarkerStack = Stack() - - for (methodInsn in methodNode.instructions.toArray().filterIsInstance()) { - when { - isBeforeSuspendMarker(methodInsn) -> { - beforeSuspensionPointMarkerStack.add(methodInsn.previous) - } - - isAfterSuspendMarker(methodInsn) -> { - suspensionPoints.add(SuspensionPoint(beforeSuspensionPointMarkerStack.pop(), methodInsn)) + // DFS until end marker or ATHROW or ARETURN. + // return true if it contains nested suspension points, which happens when we inline suspend lambda + // with multiple suspension points via several inlines. See boxInline/state/stateMachine/passLambda.kt as an example. + // In this case we simply ignore them. + fun collectSuspensionPointEnds( + insn: AbstractInsnNode, + visited: MutableSet, + ends: MutableSet + ): Boolean { + if (!visited.add(insn)) return false + if (insn.opcode == Opcodes.ARETURN || insn.opcode == Opcodes.ATHROW || isAfterSuspendMarker(insn)) { + ends.add(insn) + } else { + for (index in cfg.getSuccessorsIndices(insn)) { + val succ = methodNode.instructions[index] + if (isBeforeSuspendMarker(succ)) return true + if (collectSuspensionPointEnds(succ, visited, ends)) return true } } + return false } - assert(beforeSuspensionPointMarkerStack.isEmpty()) { "Unbalanced suspension markers stack" } - - return suspensionPoints + val starts = methodNode.instructions.asSequence().filter { + isBeforeSuspendMarker(it) && + cfg.getPredecessorsIndices(it).isNotEmpty() // Ignore unreachable start markers + }.toList() + return starts.mapNotNull { start -> + val ends = mutableSetOf() + if (collectSuspensionPointEnds(start, mutableSetOf(), ends)) return@mapNotNull null + // Ignore suspension points, if the suspension call begin is alive and suspension call end is dead + // (e.g., an inlined suspend function call ends with throwing a exception -- see KT-15017), + // (also see boxInline/suspend/stateMachine/unreachableSuspendMarker.kt) + // this is an exit point for the corresponding coroutine. + val end = ends.find { isAfterSuspendMarker(it) } ?: return@mapNotNull null + SuspensionPoint(start.previous, end) + } } - private fun dropSuspensionMarkers(methodNode: MethodNode, suspensionPoints: List) { - // Drop markers - suspensionPoints.forEach { - it.removeBeforeSuspendMarker(methodNode) - it.removeAfterSuspendMarker(methodNode) + private fun dropSuspensionMarkers(methodNode: MethodNode) { + // Drop markers, including ones, which we ignored in recognizing phase + for (marker in methodNode.instructions.asSequence().filter { isBeforeSuspendMarker(it) || isAfterSuspendMarker(it) }.toList()) { + methodNode.instructions.removeAll(listOf(marker.previous, marker)) } } @@ -796,21 +808,22 @@ class CoroutineTransformerMethodVisitor( instructions.insert(firstLabel.next, secondLabel) methodNode.tryCatchBlocks = - methodNode.tryCatchBlocks.flatMap { - val isContainingSuspensionPoint = - instructions.indexOf(it.start) < beginIndex && beginIndex < instructions.indexOf(it.end) + methodNode.tryCatchBlocks.flatMap { + val isContainingSuspensionPoint = + instructions.indexOf(it.start) < beginIndex && beginIndex < instructions.indexOf(it.end) - if (isContainingSuspensionPoint) { - assert(instructions.indexOf(it.start) < endIndex && endIndex < instructions.indexOf(it.end)) { - "Try catch block containing marker before suspension point should also contain the marker after suspension point" - } - listOf( - TryCatchBlockNode(it.start, firstLabel, it.handler, it.type), - TryCatchBlockNode(secondLabel, it.end, it.handler, it.type) - ) - } else - listOf(it) - } + if (isContainingSuspensionPoint) { + assert(instructions.indexOf(it.start) < endIndex && endIndex < instructions.indexOf(it.end)) { + "Try catch block ${instructions.indexOf(it.start)}:${instructions.indexOf(it.end)} containing marker before " + + "suspension point $beginIndex should also contain the marker after suspension point $endIndex" + } + listOf( + TryCatchBlockNode(it.start, firstLabel, it.handler, it.type), + TryCatchBlockNode(secondLabel, it.end, it.handler, it.type) + ) + } else + listOf(it) + } suspensionPoint.tryCatchBlocksContinuationLabel = secondLabel @@ -1095,16 +1108,6 @@ private class SuspensionPoint( val suspensionCallEnd: AbstractInsnNode ) { lateinit var tryCatchBlocksContinuationLabel: LabelNode - - fun removeBeforeSuspendMarker(methodNode: MethodNode) { - methodNode.instructions.remove(suspensionCallBegin.next) - methodNode.instructions.remove(suspensionCallBegin) - } - - fun removeAfterSuspendMarker(methodNode: MethodNode) { - methodNode.instructions.remove(suspensionCallEnd.previous) - methodNode.instructions.remove(suspensionCallEnd) - } } internal fun getLastParameterIndex(desc: String, access: Int) = @@ -1142,7 +1145,7 @@ internal class IgnoringCopyOperationSourceInterpreter : SourceInterpreter(Opcode } // Check whether this instruction is unreachable, i.e. there is no path leading to this instruction -internal fun isUnreachable(index: Int, sourceFrames: Array?>): Boolean = +internal fun isUnreachable(index: Int, sourceFrames: Array?>): Boolean = sourceFrames.size <= index || sourceFrames[index] == null private fun AbstractInsnNode?.isInvisibleInDebugVarInsn(methodNode: MethodNode): Boolean { diff --git a/compiler/backend/src/org/jetbrains/kotlin/codegen/optimization/common/ControlFlowGraph.kt b/compiler/backend/src/org/jetbrains/kotlin/codegen/optimization/common/ControlFlowGraph.kt index 89d50eeb3e2..d52e67726d2 100644 --- a/compiler/backend/src/org/jetbrains/kotlin/codegen/optimization/common/ControlFlowGraph.kt +++ b/compiler/backend/src/org/jetbrains/kotlin/codegen/optimization/common/ControlFlowGraph.kt @@ -22,7 +22,7 @@ class ControlFlowGraph private constructor(private val insns: InsnList) { companion object { @JvmStatic - fun build(node: MethodNode): ControlFlowGraph { + fun build(node: MethodNode, followExceptions: Boolean = true): ControlFlowGraph { val graph = ControlFlowGraph(node.instructions) fun addEdge(from: Int, to: Int) { @@ -37,7 +37,9 @@ class ControlFlowGraph private constructor(private val insns: InsnList) { } override fun visitControlFlowExceptionEdge(insn: Int, successor: Int): Boolean { - addEdge(insn, successor) + if (followExceptions) { + addEdge(insn, successor) + } return true } }.analyze() diff --git a/compiler/testData/codegen/boxInline/suspend/stateMachine/unreachableSuspendMarker.kt b/compiler/testData/codegen/boxInline/suspend/stateMachine/unreachableSuspendMarker.kt new file mode 100644 index 00000000000..652496eebf9 --- /dev/null +++ b/compiler/testData/codegen/boxInline/suspend/stateMachine/unreachableSuspendMarker.kt @@ -0,0 +1,56 @@ +// IGNORE_BACKEND: JVM_IR +// FILE: inline.kt +// COMMON_COROUTINES_TEST +// WITH_RUNTIME +// WITH_COROUTINES +// FULL_JDK +// NO_CHECK_LAMBDA_INLINING +// CHECK_STATE_MACHINE + +import helpers.* +import COROUTINES_PACKAGE.intrinsics.* + +fun check() = true + +inline suspend fun inlineMe(): Unit { + suspendCoroutineUninterceptedOrReturn { + if (check()) error("O") else error("Not this one") + } +} + +// FILE: box.kt +// WITH_COROUTINES + +import COROUTINES_PACKAGE.* +import helpers.* + +fun builder(c: suspend () -> Unit) { + c.startCoroutine(CheckStateMachineContinuation) +} + +suspend fun withoutTryCatch(): String { + StateMachineChecker.suspendHere() + inlineMe() + return "" // To prevent tail-call optimization +} + +fun box(): String { + var result = "FAIL 0" + builder { + result = try { + StateMachineChecker.suspendHere() + inlineMe() + "FAIL 1" + } catch (e: IllegalStateException) { + e.message!! + } + try { + withoutTryCatch() + result += "FAIL 2" + } catch (e: IllegalStateException) { + result += "K" + } + } + StateMachineChecker.check(numberOfSuspensions = 2) + return result +} diff --git a/compiler/tests/org/jetbrains/kotlin/codegen/BlackBoxInlineCodegenTestGenerated.java b/compiler/tests/org/jetbrains/kotlin/codegen/BlackBoxInlineCodegenTestGenerated.java index 89b81e5aa34..28f7bebc080 100644 --- a/compiler/tests/org/jetbrains/kotlin/codegen/BlackBoxInlineCodegenTestGenerated.java +++ b/compiler/tests/org/jetbrains/kotlin/codegen/BlackBoxInlineCodegenTestGenerated.java @@ -4040,6 +4040,16 @@ public class BlackBoxInlineCodegenTestGenerated extends AbstractBlackBoxInlineCo public void testPassParameter_1_3() throws Exception { runTestWithPackageReplacement("compiler/testData/codegen/boxInline/suspend/stateMachine/passParameter.kt", "kotlin.coroutines"); } + + @TestMetadata("unreachableSuspendMarker.kt") + public void testUnreachableSuspendMarker_1_2() throws Exception { + runTestWithPackageReplacement("compiler/testData/codegen/boxInline/suspend/stateMachine/unreachableSuspendMarker.kt", "kotlin.coroutines.experimental"); + } + + @TestMetadata("unreachableSuspendMarker.kt") + public void testUnreachableSuspendMarker_1_3() throws Exception { + runTestWithPackageReplacement("compiler/testData/codegen/boxInline/suspend/stateMachine/unreachableSuspendMarker.kt", "kotlin.coroutines"); + } } } diff --git a/compiler/tests/org/jetbrains/kotlin/codegen/CompileKotlinAgainstInlineKotlinTestGenerated.java b/compiler/tests/org/jetbrains/kotlin/codegen/CompileKotlinAgainstInlineKotlinTestGenerated.java index 785b7d7f797..7bd6e224712 100644 --- a/compiler/tests/org/jetbrains/kotlin/codegen/CompileKotlinAgainstInlineKotlinTestGenerated.java +++ b/compiler/tests/org/jetbrains/kotlin/codegen/CompileKotlinAgainstInlineKotlinTestGenerated.java @@ -4040,6 +4040,16 @@ public class CompileKotlinAgainstInlineKotlinTestGenerated extends AbstractCompi public void testPassParameter_1_3() throws Exception { runTestWithPackageReplacement("compiler/testData/codegen/boxInline/suspend/stateMachine/passParameter.kt", "kotlin.coroutines"); } + + @TestMetadata("unreachableSuspendMarker.kt") + public void testUnreachableSuspendMarker_1_2() throws Exception { + runTestWithPackageReplacement("compiler/testData/codegen/boxInline/suspend/stateMachine/unreachableSuspendMarker.kt", "kotlin.coroutines.experimental"); + } + + @TestMetadata("unreachableSuspendMarker.kt") + public void testUnreachableSuspendMarker_1_3() throws Exception { + runTestWithPackageReplacement("compiler/testData/codegen/boxInline/suspend/stateMachine/unreachableSuspendMarker.kt", "kotlin.coroutines"); + } } } diff --git a/compiler/tests/org/jetbrains/kotlin/codegen/ir/IrBlackBoxInlineCodegenTestGenerated.java b/compiler/tests/org/jetbrains/kotlin/codegen/ir/IrBlackBoxInlineCodegenTestGenerated.java index 06336551a69..fb5cb57f982 100644 --- a/compiler/tests/org/jetbrains/kotlin/codegen/ir/IrBlackBoxInlineCodegenTestGenerated.java +++ b/compiler/tests/org/jetbrains/kotlin/codegen/ir/IrBlackBoxInlineCodegenTestGenerated.java @@ -3800,6 +3800,11 @@ public class IrBlackBoxInlineCodegenTestGenerated extends AbstractIrBlackBoxInli public void testPassParameter_1_3() throws Exception { runTestWithPackageReplacement("compiler/testData/codegen/boxInline/suspend/stateMachine/passParameter.kt", "kotlin.coroutines"); } + + @TestMetadata("unreachableSuspendMarker.kt") + public void testUnreachableSuspendMarker_1_3() throws Exception { + runTestWithPackageReplacement("compiler/testData/codegen/boxInline/suspend/stateMachine/unreachableSuspendMarker.kt", "kotlin.coroutines"); + } } } diff --git a/js/js.tests/test/org/jetbrains/kotlin/js/test/ir/semantics/IrInlineSuspendTestsGenerated.java b/js/js.tests/test/org/jetbrains/kotlin/js/test/ir/semantics/IrInlineSuspendTestsGenerated.java index d13e5a91d7d..46cc0a81f82 100644 --- a/js/js.tests/test/org/jetbrains/kotlin/js/test/ir/semantics/IrInlineSuspendTestsGenerated.java +++ b/js/js.tests/test/org/jetbrains/kotlin/js/test/ir/semantics/IrInlineSuspendTestsGenerated.java @@ -358,5 +358,10 @@ public class IrInlineSuspendTestsGenerated extends AbstractIrInlineSuspendTests public void testPassParameter_1_3() throws Exception { runTestWithPackageReplacement("compiler/testData/codegen/boxInline/suspend/stateMachine/passParameter.kt", "kotlin.coroutines"); } + + @TestMetadata("unreachableSuspendMarker.kt") + public void testUnreachableSuspendMarker_1_3() throws Exception { + runTestWithPackageReplacement("compiler/testData/codegen/boxInline/suspend/stateMachine/unreachableSuspendMarker.kt", "kotlin.coroutines"); + } } } diff --git a/js/js.tests/test/org/jetbrains/kotlin/js/test/semantics/InlineSuspendTestsGenerated.java b/js/js.tests/test/org/jetbrains/kotlin/js/test/semantics/InlineSuspendTestsGenerated.java index 5356dc15881..18e1664aef1 100644 --- a/js/js.tests/test/org/jetbrains/kotlin/js/test/semantics/InlineSuspendTestsGenerated.java +++ b/js/js.tests/test/org/jetbrains/kotlin/js/test/semantics/InlineSuspendTestsGenerated.java @@ -588,5 +588,15 @@ public class InlineSuspendTestsGenerated extends AbstractInlineSuspendTests { public void testPassParameter_1_3() throws Exception { runTestWithPackageReplacement("compiler/testData/codegen/boxInline/suspend/stateMachine/passParameter.kt", "kotlin.coroutines"); } + + @TestMetadata("unreachableSuspendMarker.kt") + public void testUnreachableSuspendMarker_1_2() throws Exception { + runTestWithPackageReplacement("compiler/testData/codegen/boxInline/suspend/stateMachine/unreachableSuspendMarker.kt", "kotlin.coroutines.experimental"); + } + + @TestMetadata("unreachableSuspendMarker.kt") + public void testUnreachableSuspendMarker_1_3() throws Exception { + runTestWithPackageReplacement("compiler/testData/codegen/boxInline/suspend/stateMachine/unreachableSuspendMarker.kt", "kotlin.coroutines"); + } } }