From 17136257184420f7bc12878fa06b6386d351ff4c Mon Sep 17 00:00:00 2001 From: Mads Ager Date: Fri, 25 Oct 2019 14:17:42 +0200 Subject: [PATCH] JVM: Improve line number handling for suspend calls. Take branching and method calls into account when finding the line number of the continuation. If there is no line number before branching instructions or method calls, the following code is still on the line of the suspend call itself. This fixes a couple of issues with incorrect line numbers for multiple throws on the same line or multipe suspend calls on the same line. In addition, it avoids the need to spam the method node with repeated line number instructions in the IR backend. --- .../CoroutineTransformerMethodVisitor.kt | 50 ++++++---- .../codegen/optimization/common/Util.kt | 10 ++ .../backend/jvm/codegen/ExpressionCodegen.kt | 6 +- .../box/coroutines/debug/elvisLineNumber.kt | 2 +- .../debug/multipleSuspendCallsOnSameLine.kt | 94 +++++++++++++++++++ .../box/coroutines/debug/throwsOnSameLine.kt | 39 ++++++++ .../codegen/BlackBoxCodegenTestGenerated.java | 10 ++ .../LightAnalysisModeTestGenerated.java | 10 ++ .../ir/IrBlackBoxCodegenTestGenerated.java | 10 ++ 9 files changed, 209 insertions(+), 22 deletions(-) create mode 100644 compiler/testData/codegen/box/coroutines/debug/multipleSuspendCallsOnSameLine.kt create mode 100644 compiler/testData/codegen/box/coroutines/debug/throwsOnSameLine.kt 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 52049ef44cc..11fda09ef6a 100644 --- a/compiler/backend/src/org/jetbrains/kotlin/codegen/coroutines/CoroutineTransformerMethodVisitor.kt +++ b/compiler/backend/src/org/jetbrains/kotlin/codegen/coroutines/CoroutineTransformerMethodVisitor.kt @@ -11,7 +11,6 @@ import org.jetbrains.kotlin.codegen.ClassBuilder import org.jetbrains.kotlin.codegen.StackValue import org.jetbrains.kotlin.codegen.TransformationMethodVisitor import org.jetbrains.kotlin.codegen.inline.* -import org.jetbrains.kotlin.codegen.optimization.DeadCodeEliminationMethodTransformer import org.jetbrains.kotlin.codegen.optimization.boxing.isUnitInstance import org.jetbrains.kotlin.codegen.optimization.common.* import org.jetbrains.kotlin.codegen.optimization.fixStack.FixStackMethodTransformer @@ -146,7 +145,8 @@ class CoroutineTransformerMethodVisitor( val suspensionPointLineNumbers = suspensionPoints.map { findSuspensionPointLineNumber(it) } val continuationLabels = suspensionPoints.withIndex().map { - transformCallAndReturnContinuationLabel(it.index + 1, it.value, methodNode, suspendMarkerVarIndex) + transformCallAndReturnContinuationLabel( + it.index + 1, it.value, methodNode, suspendMarkerVarIndex, suspensionPointLineNumbers[it.index]) } methodNode.instructions.apply { @@ -702,12 +702,13 @@ class CoroutineTransformerMethodVisitor( id: Int, suspension: SuspensionPoint, methodNode: MethodNode, - suspendMarkerVarIndex: Int + suspendMarkerVarIndex: Int, + suspendPointLineNumber: LineNumberNode? ): LabelNode { val continuationLabel = LabelNode() val continuationLabelAfterLoadedResult = LabelNode() val suspendElementLineNumber = lineNumber - var nextLineNumberNode = suspension.suspensionCallEnd.findNextOrNull { it is LineNumberNode } as? LineNumberNode + var nextLineNumberNode = nextDefinitelyHitLineNumber(suspension) with(methodNode.instructions) { // Save state insertBefore( @@ -746,7 +747,6 @@ class CoroutineTransformerMethodVisitor( } remove(possibleTryCatchBlockStart.previous) - val afterSuspensionPointLineNumber = nextLineNumberNode?.line ?: suspendElementLineNumber insert(possibleTryCatchBlockStart, withInstructionAdapter { generateResumeWithExceptionCheck(languageVersionSettings.isReleaseCoroutines(), dataIndex, exceptionIndex) @@ -755,17 +755,23 @@ class CoroutineTransformerMethodVisitor( visitLabel(continuationLabelAfterLoadedResult.label) - // Extend next instruction linenumber. Can't use line number of suspension point here because both non-suspended execution - // and re-entering after suspension passes this label. - if (possibleTryCatchBlockStart.next?.opcode?.let { - it != Opcodes.ASTORE && it != Opcodes.CHECKCAST && it != Opcodes.INVOKESTATIC && - it != Opcodes.INVOKEVIRTUAL && it != Opcodes.INVOKEINTERFACE - } == true - ) { - visitLineNumber(afterSuspensionPointLineNumber, continuationLabelAfterLoadedResult.label) - } else { - // But keep the linenumber if the result of the call is is used afterwards - nextLineNumberNode = null + if (nextLineNumberNode != null) { + // If there is a clear next linenumber instruction, extend it. Can't use line number of suspension point + // here because both non-suspended execution and re-entering after suspension passes this label. + if (possibleTryCatchBlockStart.next?.opcode?.let { + it != Opcodes.ASTORE && it != Opcodes.CHECKCAST && it != Opcodes.INVOKESTATIC && + it != Opcodes.INVOKEVIRTUAL && it != Opcodes.INVOKEINTERFACE + } == true + ) { + visitLineNumber(nextLineNumberNode!!.line, continuationLabelAfterLoadedResult.label) + } else { + // But keep the linenumber if the result of the call is used afterwards + nextLineNumberNode = null + } + } else if (suspendPointLineNumber != null) { + // If there is no clear next linenumber instruction, the continuation is still on the + // same line as the suspend point. + visitLineNumber(suspendPointLineNumber.line, continuationLabelAfterLoadedResult.label) } }) @@ -779,6 +785,18 @@ class CoroutineTransformerMethodVisitor( return continuationLabel } + // Find the next line number instruction that is defintely hit. That is, a line number + // that comes before any branch or method call. + private fun nextDefinitelyHitLineNumber(suspension: SuspensionPoint): LineNumberNode? { + var next = suspension.suspensionCallEnd.next + while (next != null) { + if (next.isBranchOrCall) return null + else if (next is LineNumberNode) return next + else next = next.next + } + return next + } + // It's necessary to preserve some sensible invariants like there should be no jump in the middle of try-catch-block // Also it's important that spilled variables are being restored outside of TCB, // otherwise they would be treated as uninitialized within catch-block while they can be used there 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 f0434c4231b..7052f44419c 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 @@ -34,6 +34,16 @@ val AbstractInsnNode.isMeaningful: Boolean else -> true } +val AbstractInsnNode.isBranchOrCall: Boolean + get() = + when(this.type) { + AbstractInsnNode.JUMP_INSN, + AbstractInsnNode.TABLESWITCH_INSN, + AbstractInsnNode.LOOKUPSWITCH_INSN, + AbstractInsnNode.METHOD_INSN -> true + else -> false + } + class InsnSequence(val from: AbstractInsnNode, val to: AbstractInsnNode?) : Sequence { constructor(insnList: InsnList) : this(insnList.first, null) diff --git a/compiler/ir/backend.jvm/src/org/jetbrains/kotlin/backend/jvm/codegen/ExpressionCodegen.kt b/compiler/ir/backend.jvm/src/org/jetbrains/kotlin/backend/jvm/codegen/ExpressionCodegen.kt index d5c596f3cb9..c9a6d1385bc 100644 --- a/compiler/ir/backend.jvm/src/org/jetbrains/kotlin/backend/jvm/codegen/ExpressionCodegen.kt +++ b/compiler/ir/backend.jvm/src/org/jetbrains/kotlin/backend/jvm/codegen/ExpressionCodegen.kt @@ -150,11 +150,7 @@ class ExpressionCodegen( if (fileEntry != null) { val lineNumber = fileEntry.getLineNumber(offset) + 1 assert(lineNumber > 0) - // State-machine builder splits the sequence of instructions into states inside state-machine, adding additional LINENUMBERs - // between them for debugger to stop on suspension. Thus, it requires as much LINENUMBER information as possible to be present, - // otherwise, any exception will have incorrect line number. See elvisLineNumber.kt test. - // TODO: Remove unneeded LINENUMBERs after building the state-machine. - if (lastLineNumber != lineNumber || irFunction.isSuspend || irFunction.isInvokeSuspendOfLambda(context)) { + if (lastLineNumber != lineNumber) { lastLineNumber = lineNumber mv.visitLineNumber(lineNumber, markNewLabel()) } diff --git a/compiler/testData/codegen/box/coroutines/debug/elvisLineNumber.kt b/compiler/testData/codegen/box/coroutines/debug/elvisLineNumber.kt index 148c543450a..d6c409de5bc 100644 --- a/compiler/testData/codegen/box/coroutines/debug/elvisLineNumber.kt +++ b/compiler/testData/codegen/box/coroutines/debug/elvisLineNumber.kt @@ -1,9 +1,9 @@ - // TARGET_BACKEND: JVM // WITH_RUNTIME // WITH_COROUTINES // FULL_JDK + import helpers.* import kotlin.coroutines.* import kotlin.coroutines.intrinsics.* diff --git a/compiler/testData/codegen/box/coroutines/debug/multipleSuspendCallsOnSameLine.kt b/compiler/testData/codegen/box/coroutines/debug/multipleSuspendCallsOnSameLine.kt new file mode 100644 index 00000000000..bdf4a436142 --- /dev/null +++ b/compiler/testData/codegen/box/coroutines/debug/multipleSuspendCallsOnSameLine.kt @@ -0,0 +1,94 @@ +// TARGET_BACKEND: JVM +// WITH_RUNTIME +// WITH_COROUTINES +// FULL_JDK + +import helpers.* +import kotlin.coroutines.* +import kotlin.coroutines.intrinsics.* + +fun builder(c: suspend () -> Unit) { + c.startCoroutine(EmptyContinuation) +} + +suspend fun mightThrow(b: Boolean): Int { + if (b) throw RuntimeException() + return 1 +} + +fun multipleCalls(b: Boolean) = builder { + mightThrow(b) + mightThrow(!b) +} + +fun multipleCalls2(b: Boolean) = builder { + mightThrow(b) + mightThrow(!b) + throw RuntimeException() +} + +var i = 0 + +suspend fun throwEverySecondCall(): Int { + if ((i++ % 2) == 1) throw RuntimeException() + return 1 +} + +fun multipleCalls3() = builder { + throwEverySecondCall() + throwEverySecondCall() + throwEverySecondCall() +} + +fun multipleCalls4() = builder { + throwEverySecondCall() + throwEverySecondCall() +} + +fun box(): String { + try { + multipleCalls(true) + return "FAIL 0" + } catch (e: RuntimeException) { + if (e.stackTrace[0].lineNumber != 15) return "FAIL 1 ${e.stackTrace[0].lineNumber}" + if (e.stackTrace[1].lineNumber != 20) return "FAIL 2 ${e.stackTrace[1].lineNumber}" + } + + try { + multipleCalls(false) + return "FAIL 3" + } catch (e: RuntimeException) { + if (e.stackTrace[0].lineNumber != 15) return "FAIL 4 ${e.stackTrace[0].lineNumber}" + if (e.stackTrace[1].lineNumber != 20) return "FAIL 5 ${e.stackTrace[1].lineNumber}" + } + + try { + multipleCalls2(true) + return "FAIL 6" + } catch (e: RuntimeException) { + if (e.stackTrace[0].lineNumber != 15) return "FAIL 7 ${e.stackTrace[0].lineNumber}" + if (e.stackTrace[1].lineNumber != 24) return "FAIL 8 ${e.stackTrace[1].lineNumber}" + } + + try { + multipleCalls2(false) + return "FAIL 9" + } catch (e: RuntimeException) { + if (e.stackTrace[0].lineNumber != 15) return "FAIL 10 ${e.stackTrace[0].lineNumber}" + if (e.stackTrace[1].lineNumber != 24) return "FAIL 11 ${e.stackTrace[1].lineNumber}" + } + + try { + multipleCalls3() + return "FAIL 12" + } catch (e: RuntimeException) { + if (e.stackTrace[0].lineNumber != 31) return "FAIL 13 ${e.stackTrace[0].lineNumber}" + if (e.stackTrace[1].lineNumber != 36) return "FAIL 14 ${e.stackTrace[1].lineNumber}" + } + + try { + multipleCalls4() + return "FAIL 15" + } catch (e: RuntimeException) { + if (e.stackTrace[0].lineNumber != 31) return "FAIL 16 ${e.stackTrace[0].lineNumber}" + if (e.stackTrace[1].lineNumber != 41) return "FAIL 17 ${e.stackTrace[1].lineNumber}" + } + + return "OK" +} \ No newline at end of file diff --git a/compiler/testData/codegen/box/coroutines/debug/throwsOnSameLine.kt b/compiler/testData/codegen/box/coroutines/debug/throwsOnSameLine.kt new file mode 100644 index 00000000000..f7e61f41ac0 --- /dev/null +++ b/compiler/testData/codegen/box/coroutines/debug/throwsOnSameLine.kt @@ -0,0 +1,39 @@ +// TARGET_BACKEND: JVM +// WITH_RUNTIME +// WITH_COROUTINES +// FULL_JDK + +import helpers.* +import kotlin.coroutines.* +import kotlin.coroutines.intrinsics.* + +fun builder(c: suspend () -> Unit) { + c.startCoroutine(EmptyContinuation) +} + +suspend fun mightReturnNull(b: Boolean): String? { + return if (b) null else "asdf" +} + +fun throwOnSameLine(b: Boolean) = builder { + if (mightReturnNull(b) == null) throw RuntimeException() else throw RuntimeException() + throw RuntimeException() +} + +fun box(): String { + try { + throwOnSameLine(true) + return "FAIL 0" + } catch (e: RuntimeException) { + if (e.stackTrace[0].lineNumber != 19) return "FAIL 1 ${e.stackTrace[0].lineNumber}" + } + + try { + throwOnSameLine(false) + return "FAIL 2" + } catch (e: RuntimeException) { + if (e.stackTrace[0].lineNumber != 19) return "FAIL 3 ${e.stackTrace[0].lineNumber}" + } + + return "OK" +} \ No newline at end of file diff --git a/compiler/tests/org/jetbrains/kotlin/codegen/BlackBoxCodegenTestGenerated.java b/compiler/tests/org/jetbrains/kotlin/codegen/BlackBoxCodegenTestGenerated.java index 39e917e3d22..e7e71bfb0fc 100644 --- a/compiler/tests/org/jetbrains/kotlin/codegen/BlackBoxCodegenTestGenerated.java +++ b/compiler/tests/org/jetbrains/kotlin/codegen/BlackBoxCodegenTestGenerated.java @@ -6935,10 +6935,20 @@ public class BlackBoxCodegenTestGenerated extends AbstractBlackBoxCodegenTest { runTest("compiler/testData/codegen/box/coroutines/debug/fqName.kt"); } + @TestMetadata("multipleSuspendCallsOnSameLine.kt") + public void testMultipleSuspendCallsOnSameLine() throws Exception { + runTest("compiler/testData/codegen/box/coroutines/debug/multipleSuspendCallsOnSameLine.kt"); + } + @TestMetadata("runtimeDebugMetadata.kt") public void testRuntimeDebugMetadata() throws Exception { runTest("compiler/testData/codegen/box/coroutines/debug/runtimeDebugMetadata.kt"); } + + @TestMetadata("throwsOnSameLine.kt") + public void testThrowsOnSameLine() throws Exception { + runTest("compiler/testData/codegen/box/coroutines/debug/throwsOnSameLine.kt"); + } } @TestMetadata("compiler/testData/codegen/box/coroutines/featureIntersection") diff --git a/compiler/tests/org/jetbrains/kotlin/codegen/LightAnalysisModeTestGenerated.java b/compiler/tests/org/jetbrains/kotlin/codegen/LightAnalysisModeTestGenerated.java index 4ba11c3bad6..df891fa691a 100644 --- a/compiler/tests/org/jetbrains/kotlin/codegen/LightAnalysisModeTestGenerated.java +++ b/compiler/tests/org/jetbrains/kotlin/codegen/LightAnalysisModeTestGenerated.java @@ -6935,10 +6935,20 @@ public class LightAnalysisModeTestGenerated extends AbstractLightAnalysisModeTes runTest("compiler/testData/codegen/box/coroutines/debug/fqName.kt"); } + @TestMetadata("multipleSuspendCallsOnSameLine.kt") + public void testMultipleSuspendCallsOnSameLine() throws Exception { + runTest("compiler/testData/codegen/box/coroutines/debug/multipleSuspendCallsOnSameLine.kt"); + } + @TestMetadata("runtimeDebugMetadata.kt") public void testRuntimeDebugMetadata() throws Exception { runTest("compiler/testData/codegen/box/coroutines/debug/runtimeDebugMetadata.kt"); } + + @TestMetadata("throwsOnSameLine.kt") + public void testThrowsOnSameLine() throws Exception { + runTest("compiler/testData/codegen/box/coroutines/debug/throwsOnSameLine.kt"); + } } @TestMetadata("compiler/testData/codegen/box/coroutines/featureIntersection") diff --git a/compiler/tests/org/jetbrains/kotlin/codegen/ir/IrBlackBoxCodegenTestGenerated.java b/compiler/tests/org/jetbrains/kotlin/codegen/ir/IrBlackBoxCodegenTestGenerated.java index cd7d825ebf3..bd835a6a5ad 100644 --- a/compiler/tests/org/jetbrains/kotlin/codegen/ir/IrBlackBoxCodegenTestGenerated.java +++ b/compiler/tests/org/jetbrains/kotlin/codegen/ir/IrBlackBoxCodegenTestGenerated.java @@ -6385,10 +6385,20 @@ public class IrBlackBoxCodegenTestGenerated extends AbstractIrBlackBoxCodegenTes runTest("compiler/testData/codegen/box/coroutines/debug/fqName.kt"); } + @TestMetadata("multipleSuspendCallsOnSameLine.kt") + public void testMultipleSuspendCallsOnSameLine() throws Exception { + runTest("compiler/testData/codegen/box/coroutines/debug/multipleSuspendCallsOnSameLine.kt"); + } + @TestMetadata("runtimeDebugMetadata.kt") public void testRuntimeDebugMetadata() throws Exception { runTest("compiler/testData/codegen/box/coroutines/debug/runtimeDebugMetadata.kt"); } + + @TestMetadata("throwsOnSameLine.kt") + public void testThrowsOnSameLine() throws Exception { + runTest("compiler/testData/codegen/box/coroutines/debug/throwsOnSameLine.kt"); + } } @TestMetadata("compiler/testData/codegen/box/coroutines/featureIntersection")