From e05f2eaff610e3bbbdc9695b27ecc8ac347f9bd6 Mon Sep 17 00:00:00 2001 From: Dmitry Petrov Date: Tue, 24 Jan 2017 16:46:52 +0300 Subject: [PATCH] KT-15017 Throwing exception in the end of inline suspend-functions lead to internal compiler error Suspend function call with a reachable (alive) begin marker and unreachable (dead) end marker is an exit point for the corresponding coroutine. It isn't a suspension point, and doesn't introduce a new state in the coroutine FSM. --- .../CoroutineTransformationClassBuilder.kt | 38 +++++++++-------- .../DeadCodeEliminationMethodTransformer.kt | 25 +++++++++-- .../codegen/optimization/common/Util.kt | 5 +++ .../codegen/box/coroutines/kt15017.kt | 25 +++++++++++ .../light-analysis/coroutines/kt15017.txt | 42 +++++++++++++++++++ .../ir/IrBlackBoxCodegenTestGenerated.java | 6 +++ .../codegen/BlackBoxCodegenTestGenerated.java | 6 +++ ...LightAnalysisModeCodegenTestGenerated.java | 6 +++ .../semantics/JsCodegenBoxTestGenerated.java | 6 +++ 9 files changed, 137 insertions(+), 22 deletions(-) create mode 100644 compiler/testData/codegen/box/coroutines/kt15017.kt create mode 100644 compiler/testData/codegen/light-analysis/coroutines/kt15017.txt diff --git a/compiler/backend/src/org/jetbrains/kotlin/codegen/coroutines/CoroutineTransformationClassBuilder.kt b/compiler/backend/src/org/jetbrains/kotlin/codegen/coroutines/CoroutineTransformationClassBuilder.kt index 60a53921040..74df9591615 100644 --- a/compiler/backend/src/org/jetbrains/kotlin/codegen/coroutines/CoroutineTransformationClassBuilder.kt +++ b/compiler/backend/src/org/jetbrains/kotlin/codegen/coroutines/CoroutineTransformationClassBuilder.kt @@ -83,10 +83,7 @@ class CoroutineTransformerMethodVisitor( // 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) - DeadCodeEliminationMethodTransformer().transform(classBuilder.thisName, methodNode) - suspensionPoints.removeAll { - it.suspensionCallBegin.next == null && it.suspensionCallBegin.previous == null - } + removeUnreachableSuspensionPointsAndExitPoints(methodNode, suspensionPoints) processUninitializedStores(methodNode) @@ -136,6 +133,22 @@ class CoroutineTransformerMethodVisitor( } + private fun removeUnreachableSuspensionPointsAndExitPoints(methodNode: MethodNode, suspensionPoints: MutableList) { + val dceResult = DeadCodeEliminationMethodTransformer().transformWithResult(classBuilder.thisName, methodNode) + + // 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)) { + methodNode.instructions.remove(it.suspensionCallBegin) + } + } + + suspensionPoints.removeAll { dceResult.isRemoved(it.suspensionCallBegin) || dceResult.isRemoved(it.suspensionCallEnd) } + } + private fun collectSuspensionPoints(methodNode: MethodNode): MutableList { val suspensionPoints = mutableListOf() val beforeSuspensionPointMarkerStack = Stack() @@ -149,12 +162,7 @@ class CoroutineTransformerMethodVisitor( } AFTER_SUSPENSION_POINT_MARKER_NAME -> { - suspensionPoints.add( - SuspensionPoint( - suspensionCallBegin = beforeSuspensionPointMarkerStack.pop(), - suspensionCallEnd = methodInsn - ) - ) + suspensionPoints.add(SuspensionPoint(beforeSuspensionPointMarkerStack.pop(), methodInsn)) } } } @@ -186,8 +194,8 @@ class CoroutineTransformerMethodVisitor( for (suspension in suspensionPoints) { val suspensionCallBegin = suspension.suspensionCallBegin - val suspensionCallEnd = suspension.suspensionCallEnd - assert(frames[suspensionCallEnd.next.index()]?.stackSize == 1) { + + assert(frames[suspension.suspensionCallEnd.next.index()]?.stackSize == 1) { "Stack should be spilled before suspension call" } @@ -430,9 +438,3 @@ private class SuspensionPoint( ) { lateinit var tryCatchBlocksContinuationLabel: LabelNode } - -private val DEFAULT_VALUE_OPCODES = - setOf(Opcodes.ICONST_0, Opcodes.LCONST_0, Opcodes.FCONST_0, Opcodes.DCONST_0, Opcodes.ACONST_NULL, - // GETSTATIC Unit.Instance - Opcodes.GETSTATIC) - diff --git a/compiler/backend/src/org/jetbrains/kotlin/codegen/optimization/DeadCodeEliminationMethodTransformer.kt b/compiler/backend/src/org/jetbrains/kotlin/codegen/optimization/DeadCodeEliminationMethodTransformer.kt index 57b0ea2feb4..f11e5a10f08 100644 --- a/compiler/backend/src/org/jetbrains/kotlin/codegen/optimization/DeadCodeEliminationMethodTransformer.kt +++ b/compiler/backend/src/org/jetbrains/kotlin/codegen/optimization/DeadCodeEliminationMethodTransformer.kt @@ -16,15 +16,22 @@ package org.jetbrains.kotlin.codegen.optimization -import org.jetbrains.org.objectweb.asm.tree.MethodNode -import org.jetbrains.kotlin.codegen.optimization.transformer.MethodTransformer import org.jetbrains.kotlin.codegen.optimization.common.OptimizationBasicInterpreter import org.jetbrains.kotlin.codegen.optimization.common.isMeaningful import org.jetbrains.kotlin.codegen.optimization.common.removeEmptyCatchBlocks +import org.jetbrains.kotlin.codegen.optimization.transformer.MethodTransformer +import org.jetbrains.org.objectweb.asm.tree.AbstractInsnNode +import org.jetbrains.org.objectweb.asm.tree.MethodNode class DeadCodeEliminationMethodTransformer : MethodTransformer() { override fun transform(internalClassName: String, methodNode: MethodNode) { - val frames = MethodTransformer.analyze(internalClassName, methodNode, OptimizationBasicInterpreter()) + transformWithResult(internalClassName, methodNode) + } + + fun transformWithResult(internalClassName: String, methodNode: MethodNode): Result { + val removedNodes = HashSet() + + val frames = analyze(internalClassName, methodNode, OptimizationBasicInterpreter()) val insnList = methodNode.instructions val insnsArray = insnList.toArray() @@ -32,9 +39,19 @@ class DeadCodeEliminationMethodTransformer : MethodTransformer() { // by try/catch blocks or local variables table. insnsArray.zip(frames).filter { it.second == null && it.first.isMeaningful - }.forEach { insnList.remove(it.first) } + }.forEach { + insnList.remove(it.first) + removedNodes.add(it.first) + } // Remove empty try-catch blocks to make sure we don't break data flow analysis invariants by dead code elimination. methodNode.removeEmptyCatchBlocks() + + return Result(removedNodes) + } + + class Result(val removedNodes: Set) { + fun isRemoved(node: AbstractInsnNode) = removedNodes.contains(node) + fun isAlive(node: AbstractInsnNode) = !isRemoved(node) } } diff --git a/compiler/backend/src/org/jetbrains/kotlin/codegen/optimization/common/Util.kt b/compiler/backend/src/org/jetbrains/kotlin/codegen/optimization/common/Util.kt index c8c57ed2d0d..2dc04c11880 100644 --- a/compiler/backend/src/org/jetbrains/kotlin/codegen/optimization/common/Util.kt +++ b/compiler/backend/src/org/jetbrains/kotlin/codegen/optimization/common/Util.kt @@ -16,6 +16,7 @@ package org.jetbrains.kotlin.codegen.optimization.common +import org.jetbrains.kotlin.codegen.inline.InlineCodegenUtil import org.jetbrains.org.objectweb.asm.Opcodes import org.jetbrains.org.objectweb.asm.Opcodes.* import org.jetbrains.org.objectweb.asm.tree.* @@ -120,3 +121,7 @@ fun insnListOf(vararg insns: AbstractInsnNode) = InsnList().apply { insns.forEac fun AbstractInsnNode.isStoreOperation(): Boolean = getOpcode() in Opcodes.ISTORE..Opcodes.ASTORE fun AbstractInsnNode.isLoadOperation(): Boolean = getOpcode() in Opcodes.ILOAD..Opcodes.ALOAD + +val AbstractInsnNode?.insnText get() = InlineCodegenUtil.getInsnText(this) +val AbstractInsnNode?.debugText get() = + if (this == null) "" else "${this.javaClass.simpleName}: $insnText" diff --git a/compiler/testData/codegen/box/coroutines/kt15017.kt b/compiler/testData/codegen/box/coroutines/kt15017.kt new file mode 100644 index 00000000000..ca00897a991 --- /dev/null +++ b/compiler/testData/codegen/box/coroutines/kt15017.kt @@ -0,0 +1,25 @@ +// WITH_RUNTIME +// WITH_COROUTINES +import kotlin.coroutines.startCoroutine + +class Controller { + suspend inline fun suspendInlineThrow(v: String): String = throw RuntimeException(v) + suspend inline fun suspendInline(v: String) = v +} + +fun builder(c: suspend Controller.() -> Unit) { + c.startCoroutine(Controller(), EmptyContinuation) +} + +class OK + +fun box(): String { + var result = "" + + builder { + result = try { suspendInlineThrow("OK") } catch (e: RuntimeException) { e.message!! } +// result = suspendInline("OK") + } + + return result +} \ No newline at end of file diff --git a/compiler/testData/codegen/light-analysis/coroutines/kt15017.txt b/compiler/testData/codegen/light-analysis/coroutines/kt15017.txt new file mode 100644 index 00000000000..ac4cceedaeb --- /dev/null +++ b/compiler/testData/codegen/light-analysis/coroutines/kt15017.txt @@ -0,0 +1,42 @@ +@kotlin.Metadata +public final class Controller { + public method (): void + public final @org.jetbrains.annotations.Nullable method suspendInline(@org.jetbrains.annotations.NotNull p0: java.lang.String, @org.jetbrains.annotations.NotNull p1: kotlin.coroutines.Continuation): java.lang.Object + public final @org.jetbrains.annotations.Nullable method suspendInlineThrow(@org.jetbrains.annotations.NotNull p0: java.lang.String, @org.jetbrains.annotations.NotNull p1: kotlin.coroutines.Continuation): java.lang.Object +} + +@kotlin.Metadata +public final class CoroutineUtilKt { + public final static @org.jetbrains.annotations.NotNull method handleExceptionContinuation(@org.jetbrains.annotations.NotNull p0: kotlin.jvm.functions.Function1): kotlin.coroutines.Continuation + public final static @org.jetbrains.annotations.NotNull method handleResultContinuation(@org.jetbrains.annotations.NotNull p0: kotlin.jvm.functions.Function1): kotlin.coroutines.Continuation +} + +@kotlin.Metadata +public class EmptyContinuation { + public final static field Companion: EmptyContinuation.Companion + private final @org.jetbrains.annotations.NotNull field context: kotlin.coroutines.CoroutineContext + inner class EmptyContinuation/Companion + public @synthetic.kotlin.jvm.GeneratedByJvmOverloads method (): void + public method (@org.jetbrains.annotations.NotNull p0: kotlin.coroutines.CoroutineContext): void + public synthetic method (p0: kotlin.coroutines.CoroutineContext, p1: int, p2: kotlin.jvm.internal.DefaultConstructorMarker): void + public @org.jetbrains.annotations.NotNull method getContext(): kotlin.coroutines.CoroutineContext + public method resume(@org.jetbrains.annotations.Nullable p0: java.lang.Object): void + public method resumeWithException(@org.jetbrains.annotations.NotNull p0: java.lang.Throwable): void +} + +@kotlin.Metadata +public final static class EmptyContinuation/Companion { + inner class EmptyContinuation/Companion + private method (): void +} + +@kotlin.Metadata +public final class Kt15017Kt { + public final static @org.jetbrains.annotations.NotNull method box(): java.lang.String + public final static method builder(@org.jetbrains.annotations.NotNull p0: kotlin.jvm.functions.Function2): void +} + +@kotlin.Metadata +public final class OK { + public method (): void +} diff --git a/compiler/tests-ir-jvm/tests/org/jetbrains/kotlin/codegen/ir/IrBlackBoxCodegenTestGenerated.java b/compiler/tests-ir-jvm/tests/org/jetbrains/kotlin/codegen/ir/IrBlackBoxCodegenTestGenerated.java index be9d2019f66..2746dfb4d1a 100644 --- a/compiler/tests-ir-jvm/tests/org/jetbrains/kotlin/codegen/ir/IrBlackBoxCodegenTestGenerated.java +++ b/compiler/tests-ir-jvm/tests/org/jetbrains/kotlin/codegen/ir/IrBlackBoxCodegenTestGenerated.java @@ -4751,6 +4751,12 @@ public class IrBlackBoxCodegenTestGenerated extends AbstractIrBlackBoxCodegenTes doTest(fileName); } + @TestMetadata("kt15017.kt") + public void testKt15017() throws Exception { + String fileName = KotlinTestUtils.navigationMetadata("compiler/testData/codegen/box/coroutines/kt15017.kt"); + doTest(fileName); + } + @TestMetadata("lastExpressionIsLoop.kt") public void testLastExpressionIsLoop() throws Exception { String fileName = KotlinTestUtils.navigationMetadata("compiler/testData/codegen/box/coroutines/lastExpressionIsLoop.kt"); diff --git a/compiler/tests/org/jetbrains/kotlin/codegen/BlackBoxCodegenTestGenerated.java b/compiler/tests/org/jetbrains/kotlin/codegen/BlackBoxCodegenTestGenerated.java index ac09cb1160a..cd5727f6353 100644 --- a/compiler/tests/org/jetbrains/kotlin/codegen/BlackBoxCodegenTestGenerated.java +++ b/compiler/tests/org/jetbrains/kotlin/codegen/BlackBoxCodegenTestGenerated.java @@ -4751,6 +4751,12 @@ public class BlackBoxCodegenTestGenerated extends AbstractBlackBoxCodegenTest { doTest(fileName); } + @TestMetadata("kt15017.kt") + public void testKt15017() throws Exception { + String fileName = KotlinTestUtils.navigationMetadata("compiler/testData/codegen/box/coroutines/kt15017.kt"); + doTest(fileName); + } + @TestMetadata("lastExpressionIsLoop.kt") public void testLastExpressionIsLoop() throws Exception { String fileName = KotlinTestUtils.navigationMetadata("compiler/testData/codegen/box/coroutines/lastExpressionIsLoop.kt"); diff --git a/compiler/tests/org/jetbrains/kotlin/codegen/LightAnalysisModeCodegenTestGenerated.java b/compiler/tests/org/jetbrains/kotlin/codegen/LightAnalysisModeCodegenTestGenerated.java index 4c406aee6dd..529e35301da 100644 --- a/compiler/tests/org/jetbrains/kotlin/codegen/LightAnalysisModeCodegenTestGenerated.java +++ b/compiler/tests/org/jetbrains/kotlin/codegen/LightAnalysisModeCodegenTestGenerated.java @@ -4751,6 +4751,12 @@ public class LightAnalysisModeCodegenTestGenerated extends AbstractLightAnalysis doTest(fileName); } + @TestMetadata("kt15017.kt") + public void testKt15017() throws Exception { + String fileName = KotlinTestUtils.navigationMetadata("compiler/testData/codegen/box/coroutines/kt15017.kt"); + doTest(fileName); + } + @TestMetadata("lastExpressionIsLoop.kt") public void testLastExpressionIsLoop() throws Exception { String fileName = KotlinTestUtils.navigationMetadata("compiler/testData/codegen/box/coroutines/lastExpressionIsLoop.kt"); diff --git a/js/js.tests/test/org/jetbrains/kotlin/js/test/semantics/JsCodegenBoxTestGenerated.java b/js/js.tests/test/org/jetbrains/kotlin/js/test/semantics/JsCodegenBoxTestGenerated.java index e5d2eac03c9..bd83aa1fd49 100644 --- a/js/js.tests/test/org/jetbrains/kotlin/js/test/semantics/JsCodegenBoxTestGenerated.java +++ b/js/js.tests/test/org/jetbrains/kotlin/js/test/semantics/JsCodegenBoxTestGenerated.java @@ -5436,6 +5436,12 @@ public class JsCodegenBoxTestGenerated extends AbstractJsCodegenBoxTest { doTest(fileName); } + @TestMetadata("kt15017.kt") + public void testKt15017() throws Exception { + String fileName = KotlinTestUtils.navigationMetadata("compiler/testData/codegen/box/coroutines/kt15017.kt"); + doTest(fileName); + } + @TestMetadata("lastExpressionIsLoop.kt") public void testLastExpressionIsLoop() throws Exception { String fileName = KotlinTestUtils.navigationMetadata("compiler/testData/codegen/box/coroutines/lastExpressionIsLoop.kt");