From d0239660547104065f31663cc69f550aeb3eea7b Mon Sep 17 00:00:00 2001 From: Mads Ager Date: Fri, 9 Apr 2021 09:19:35 +0200 Subject: [PATCH] [JVM] Fix various undefined locals issues. CoroutineTransformermethodVisitor attempts to extend the ranges of local variables in various situations. Probably in an attempt to give a better debugging experience. However, all of these range extensions lead to invalid local variable tables where something is in the local variable table where nothing is in the corresponding slot. The code that extends variables to the next suspension point instead of ending them when they are no longer live has issues with loops. When resuming and reentering the loop, the locals table will mention a local that we did not spill and which is therefore not restored when resuming. The code that extends local variable table entries if there are no suspension points between two entries doesn't work for code such as: ``` var s: String if (suspendHere() == "OK") { s = "OK" } else { s = "FAIL" } ``` If the local variable ranges are collapsed into one, one of the branches will have the local defined in the local variable table before the slot is initialized. --- build.gradle.kts | 2 +- .../CoroutineTransformerMethodVisitor.kt | 83 ++++++++----------- .../suspendFunctionDeadVariables.kt | 3 +- .../debug/localVariableCorrectLabel.kt | 8 +- .../completion/nonStaticStateMachine.kt | 4 +- .../suspend/completion/staticStateMachine.kt | 4 +- .../completion/staticStateMachineReceiver.kt | 2 +- .../suspend/localsStateMachineTransform.kt | 8 +- .../localVariables/suspend/underscoreNames.kt | 4 +- gradle/verification-metadata.xml | 8 +- 10 files changed, 52 insertions(+), 74 deletions(-) diff --git a/build.gradle.kts b/build.gradle.kts index 3903debc168..6faa16d9900 100644 --- a/build.gradle.kts +++ b/build.gradle.kts @@ -182,7 +182,7 @@ extra["versions.jflex"] = "1.7.0" extra["versions.markdown"] = "0.1.25" extra["versions.trove4j"] = "1.0.20181211" extra["versions.completion-ranking-kotlin"] = "0.1.3" -extra["versions.r8"] = "2.1.96" +extra["versions.r8"] = "2.2.64" val immutablesVersion = "0.3.1" extra["versions.kotlinx-collections-immutable"] = immutablesVersion extra["versions.kotlinx-collections-immutable-jvm"] = immutablesVersion 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 176435bedba..4998096ce05 100644 --- a/compiler/backend/src/org/jetbrains/kotlin/codegen/coroutines/CoroutineTransformerMethodVisitor.kt +++ b/compiler/backend/src/org/jetbrains/kotlin/codegen/coroutines/CoroutineTransformerMethodVisitor.kt @@ -1276,16 +1276,13 @@ private fun updateLvtAccordingToLiveness(method: MethodNode, isForNamedFunction: fun isAlive(insnIndex: Int, variableIndex: Int): Boolean = liveness[insnIndex].isAlive(variableIndex) - fun nextSuspensionPointEndLabel(insn: AbstractInsnNode): LabelNode { - val suspensionPoint = - InsnSequence(insn, method.instructions.last).firstOrNull { isAfterSuspendMarker(it) } ?: method.instructions.last - return suspensionPoint as? LabelNode ?: suspensionPoint.findNextOrNull { it is LabelNode } as LabelNode - } - - fun nextSuspensionPointStartLabel(insn: AbstractInsnNode): LabelNode { - val suspensionPoint = - InsnSequence(insn, method.instructions.last).firstOrNull { isBeforeSuspendMarker(it) } ?: method.instructions.last - return suspensionPoint as? LabelNode ?: suspensionPoint.findPreviousOrNull { it is LabelNode } as LabelNode + fun nextLabel(node: AbstractInsnNode?): LabelNode? { + var current = node + while (current != null) { + if (current is LabelNode) return current + current = current.next + } + return null } fun min(a: LabelNode, b: LabelNode): LabelNode = @@ -1294,9 +1291,6 @@ private fun updateLvtAccordingToLiveness(method: MethodNode, isForNamedFunction: fun max(a: LabelNode, b: LabelNode): LabelNode = if (method.instructions.indexOf(a) < method.instructions.indexOf(b)) b else a - fun containsSuspensionPoint(a: LabelNode, b: LabelNode): Boolean = - InsnSequence(min(a, b), max(a, b)).none { isBeforeSuspendMarker(it) } - val oldLvt = arrayListOf() for (record in method.localVariables) { oldLvt += record @@ -1317,35 +1311,40 @@ private fun updateLvtAccordingToLiveness(method: MethodNode, isForNamedFunction: // No variable in LVT -> do not add one val lvtRecord = oldLvt.findRecord(insnIndex, variableIndex) ?: continue if (lvtRecord.name == CONTINUATION_VARIABLE_NAME) continue - // Extend lvt record to the next suspension point - val endLabel = min(lvtRecord.end, nextSuspensionPointEndLabel(insn)) + // End the local when it is no longer live. Since it is not live, we will not spill and unspill it across + // suspension points. It is tempting to keep it alive until the next suspension point to leave it visible in + // the debugger for as long as possible. However, in the case of loops, the resumption after suspension can + // have a backwards edge targeting instruction between the point of death and the next suspension point. + // + // For example, code such as the following: + // + // listOf.forEach { + // yield(it) + // } + // + // Generates code of this form with a back edge after resumption that will lead to invalid locals tables + // if the local range is extended to the next suspension point. + // + // iterator = iterable.iterator() + // L1: (iterable dies here) + // load iterator.next if there + // yield suspension point + // + // L2: (resumption point) + // restore live variables (not including iterable) + // goto L1 (iterator not restored here, so we cannot not have iterator live at L1) + val endLabel = nextLabel(insn.next)?.let { min(lvtRecord.end, it) } ?: lvtRecord.end // startLabel can be null in case of parameters @Suppress("NAME_SHADOWING") val startLabel = startLabel ?: lvtRecord.start - // Attempt to extend existing local variable node corresponding to the record in - // the original local variable table. - val recordToExtend: LocalVariableNode? = oldLvtNodeToLatestNewLvtNode[lvtRecord] - if (recordToExtend != null && containsSuspensionPoint(recordToExtend.end, startLabel)) { - recordToExtend.end = endLabel - } else { - val node = LocalVariableNode(lvtRecord.name, lvtRecord.desc, lvtRecord.signature, startLabel, endLabel, lvtRecord.index) - if (lvtRecord !in oldLvtNodeToLatestNewLvtNode) { - method.localVariables.add(node) - } - oldLvtNodeToLatestNewLvtNode[lvtRecord] = node + val node = LocalVariableNode(lvtRecord.name, lvtRecord.desc, lvtRecord.signature, startLabel, endLabel, lvtRecord.index) + if (lvtRecord !in oldLvtNodeToLatestNewLvtNode) { + method.localVariables.add(node) } + oldLvtNodeToLatestNewLvtNode[lvtRecord] = node } } } - val deadVariables = arrayListOf() - outer@for (variableIndex in start until method.maxLocals) { - if (oldLvt.none { it.index == variableIndex }) continue - for (insnIndex in 0 until (method.instructions.size() - 1)) { - if (isAlive(insnIndex, variableIndex)) continue@outer - } - deadVariables += variableIndex - } - for (variable in oldLvt) { // $continuation and $result are dead, but they are used by debugger, as well as fake inliner variables // For example, $continuation is used to create async stack trace @@ -1361,19 +1360,5 @@ private fun updateLvtAccordingToLiveness(method: MethodNode, isForNamedFunction: method.localVariables.add(variable) continue } - - // Shrink LVT records of dead variables to the next suspension point - if (variable.index in deadVariables) { - method.localVariables.add( - LocalVariableNode( - variable.name, - variable.desc, - variable.signature, - variable.start, - min(variable.end, nextSuspensionPointStartLabel(variable.start)), - variable.index - ) - ) - } } } diff --git a/compiler/testData/checkLocalVariablesTable/suspendFunctionDeadVariables.kt b/compiler/testData/checkLocalVariablesTable/suspendFunctionDeadVariables.kt index 2999a4f1a7d..b19a3e850cc 100644 --- a/compiler/testData/checkLocalVariablesTable/suspendFunctionDeadVariables.kt +++ b/compiler/testData/checkLocalVariablesTable/suspendFunctionDeadVariables.kt @@ -9,6 +9,5 @@ suspend fun test() { // METHOD : SuspendFunctionDeadVariablesKt.test(Lkotlin/coroutines/Continuation;)Ljava/lang/Object; -// VARIABLE : NAME=a TYPE=I INDEX=1 // VARIABLE : NAME=$continuation TYPE=Lkotlin/coroutines/Continuation; INDEX=3 -// VARIABLE : NAME=$result TYPE=Ljava/lang/Object; INDEX=2 \ No newline at end of file +// VARIABLE : NAME=$result TYPE=Ljava/lang/Object; INDEX=2 diff --git a/compiler/testData/codegen/bytecodeText/coroutines/debug/localVariableCorrectLabel.kt b/compiler/testData/codegen/bytecodeText/coroutines/debug/localVariableCorrectLabel.kt index 1eb6b8692b6..c8b84dc53a5 100644 --- a/compiler/testData/codegen/bytecodeText/coroutines/debug/localVariableCorrectLabel.kt +++ b/compiler/testData/codegen/bytecodeText/coroutines/debug/localVariableCorrectLabel.kt @@ -15,12 +15,6 @@ fun main(args: Array) { suspend fun SequenceScope.awaitSeq(): Int = 42 // 1 LINENUMBER 9 L19 - -// JVM_IR_TEMPLATES -// 1 LOCALVARIABLE a I L[0-9]+ L4 - -// JVM_TEMPLATES -// 1 LOCALVARIABLE a I L[0-9]+ L19 -// TODO: Old BE generates LINENUMBER label after suspension point, unlike JVM_BE +// 1 LOCALVARIABLE a I L[0-9]+ L18 // IGNORE_BACKEND_FIR: JVM_IR diff --git a/compiler/testData/debug/localVariables/suspend/completion/nonStaticStateMachine.kt b/compiler/testData/debug/localVariables/suspend/completion/nonStaticStateMachine.kt index 4e7fb338122..7272d27e1dc 100644 --- a/compiler/testData/debug/localVariables/suspend/completion/nonStaticStateMachine.kt +++ b/compiler/testData/debug/localVariables/suspend/completion/nonStaticStateMachine.kt @@ -36,6 +36,6 @@ suspend fun box() { // test.kt:5 foo: $completion:kotlin.coroutines.Continuation=A$foo1$1 // test.kt:8 foo1: $continuation:kotlin.coroutines.Continuation=A$foo1$1, $result:java.lang.Object=null, l:long=42:long // test.kt:9 foo1: $continuation:kotlin.coroutines.Continuation=A$foo1$1, $result:java.lang.Object=null, l:long=42:long -// test.kt:10 foo1: $continuation:kotlin.coroutines.Continuation=A$foo1$1, $result:java.lang.Object=null, l:long=42:long, dead:long=42:long +// test.kt:10 foo1: $continuation:kotlin.coroutines.Continuation=A$foo1$1, $result:java.lang.Object=null // test.kt:14 box: $completion:kotlin.coroutines.Continuation=helpers.ResultContinuation -// test.kt:15 box: $completion:kotlin.coroutines.Continuation=helpers.ResultContinuation \ No newline at end of file +// test.kt:15 box: $completion:kotlin.coroutines.Continuation=helpers.ResultContinuation diff --git a/compiler/testData/debug/localVariables/suspend/completion/staticStateMachine.kt b/compiler/testData/debug/localVariables/suspend/completion/staticStateMachine.kt index 6b387ac6457..bc7b5d02ae5 100644 --- a/compiler/testData/debug/localVariables/suspend/completion/staticStateMachine.kt +++ b/compiler/testData/debug/localVariables/suspend/completion/staticStateMachine.kt @@ -32,6 +32,6 @@ suspend fun box() { // test.kt:4 foo: $completion:kotlin.coroutines.Continuation=TestKt$foo1$1 // test.kt:7 foo1: $continuation:kotlin.coroutines.Continuation=TestKt$foo1$1, $result:java.lang.Object=null, l:long=42:long // test.kt:8 foo1: $continuation:kotlin.coroutines.Continuation=TestKt$foo1$1, $result:java.lang.Object=null, l:long=42:long -// test.kt:9 foo1: $continuation:kotlin.coroutines.Continuation=TestKt$foo1$1, $result:java.lang.Object=null, l:long=42:long, dead:long=42:long +// test.kt:9 foo1: $continuation:kotlin.coroutines.Continuation=TestKt$foo1$1, $result:java.lang.Object=null // test.kt:12 box: $completion:kotlin.coroutines.Continuation=helpers.ResultContinuation -// test.kt:13 box: $completion:kotlin.coroutines.Continuation=helpers.ResultContinuation \ No newline at end of file +// test.kt:13 box: $completion:kotlin.coroutines.Continuation=helpers.ResultContinuation diff --git a/compiler/testData/debug/localVariables/suspend/completion/staticStateMachineReceiver.kt b/compiler/testData/debug/localVariables/suspend/completion/staticStateMachineReceiver.kt index 423f9e55639..889777d59f9 100644 --- a/compiler/testData/debug/localVariables/suspend/completion/staticStateMachineReceiver.kt +++ b/compiler/testData/debug/localVariables/suspend/completion/staticStateMachineReceiver.kt @@ -36,6 +36,6 @@ suspend fun box() { // test.kt:6 foo: $this$foo:A=A, $completion:kotlin.coroutines.Continuation=TestKt$foo1$1 // test.kt:9 foo1: $continuation:kotlin.coroutines.Continuation=TestKt$foo1$1, $result:java.lang.Object=null, l:long=42:long // test.kt:10 foo1: $continuation:kotlin.coroutines.Continuation=TestKt$foo1$1, $result:java.lang.Object=null, l:long=42:long -// test.kt:11 foo1: $continuation:kotlin.coroutines.Continuation=TestKt$foo1$1, $result:java.lang.Object=null, l:long=42:long, dead:long=42:long +// test.kt:11 foo1: $continuation:kotlin.coroutines.Continuation=TestKt$foo1$1, $result:java.lang.Object=null // test.kt:14 box: $completion:kotlin.coroutines.Continuation=helpers.ResultContinuation // test.kt:15 box: $completion:kotlin.coroutines.Continuation=helpers.ResultContinuation diff --git a/compiler/testData/debug/localVariables/suspend/localsStateMachineTransform.kt b/compiler/testData/debug/localVariables/suspend/localsStateMachineTransform.kt index c29155d9f0a..a68f0adf6a5 100644 --- a/compiler/testData/debug/localVariables/suspend/localsStateMachineTransform.kt +++ b/compiler/testData/debug/localVariables/suspend/localsStateMachineTransform.kt @@ -31,19 +31,19 @@ suspend fun box() { // test.kt:11 box: $continuation:kotlin.coroutines.Continuation=TestKt$box$1, $result:java.lang.Object=null, x:int=0:int // test.kt:5 f: x:int=0:int // test.kt:11 box: $continuation:kotlin.coroutines.Continuation=TestKt$box$1, $result:java.lang.Object=null, x:int=0:int -// test.kt:10 box: $continuation:kotlin.coroutines.Continuation=TestKt$box$1, $result:java.lang.Object=null, x:int=0:int +// test.kt:10 box: $continuation:kotlin.coroutines.Continuation=TestKt$box$1, $result:java.lang.Object=null // test.kt:11 box: $continuation:kotlin.coroutines.Continuation=TestKt$box$1, $result:java.lang.Object=null, x:int=1:int // test.kt:5 f: x:int=1:int // test.kt:11 box: $continuation:kotlin.coroutines.Continuation=TestKt$box$1, $result:java.lang.Object=null, x:int=1:int -// test.kt:10 box: $continuation:kotlin.coroutines.Continuation=TestKt$box$1, $result:java.lang.Object=null, x:int=1:int +// test.kt:10 box: $continuation:kotlin.coroutines.Continuation=TestKt$box$1, $result:java.lang.Object=null // test.kt:14 box: $continuation:kotlin.coroutines.Continuation=TestKt$box$1, $result:java.lang.Object=null // test.kt:15 box: $continuation:kotlin.coroutines.Continuation=TestKt$box$1, $result:java.lang.Object=null // test.kt:16 box: $continuation:kotlin.coroutines.Continuation=TestKt$box$1, $result:java.lang.Object=null, x:int=0:int // test.kt:5 f: x:int=0:int // test.kt:16 box: $continuation:kotlin.coroutines.Continuation=TestKt$box$1, $result:java.lang.Object=null, x:int=0:int -// test.kt:15 box: $continuation:kotlin.coroutines.Continuation=TestKt$box$1, $result:java.lang.Object=null, x:int=0:int +// test.kt:15 box: $continuation:kotlin.coroutines.Continuation=TestKt$box$1, $result:java.lang.Object=null // test.kt:16 box: $continuation:kotlin.coroutines.Continuation=TestKt$box$1, $result:java.lang.Object=null, x:int=1:int // test.kt:5 f: x:int=1:int // test.kt:16 box: $continuation:kotlin.coroutines.Continuation=TestKt$box$1, $result:java.lang.Object=null, x:int=1:int -// test.kt:15 box: $continuation:kotlin.coroutines.Continuation=TestKt$box$1, $result:java.lang.Object=null, x:int=1:int +// test.kt:15 box: $continuation:kotlin.coroutines.Continuation=TestKt$box$1, $result:java.lang.Object=null // test.kt:18 box: $continuation:kotlin.coroutines.Continuation=TestKt$box$1, $result:java.lang.Object=null diff --git a/compiler/testData/debug/localVariables/suspend/underscoreNames.kt b/compiler/testData/debug/localVariables/suspend/underscoreNames.kt index e7c4d05bb85..b5ceff8353d 100644 --- a/compiler/testData/debug/localVariables/suspend/underscoreNames.kt +++ b/compiler/testData/debug/localVariables/suspend/underscoreNames.kt @@ -45,7 +45,7 @@ suspend fun box() = foo(A()) { (x_param, _, y_param) -> // test.kt:12 invokeSuspend: $result:java.lang.Object=kotlin.Unit, $dstr$x_param$_u24__u24$y_param:A=A, x_param:java.lang.String="O":java.lang.String // LOCAL VARIABLES -// test.kt:13 invokeSuspend: $result:java.lang.Object=kotlin.Unit, $dstr$x_param$_u24__u24$y_param:A=A, x_param:java.lang.String="O":java.lang.String, y_param:java.lang.String="K":java.lang.String +// test.kt:13 invokeSuspend: $result:java.lang.Object=kotlin.Unit, x_param:java.lang.String="O":java.lang.String, y_param:java.lang.String="K":java.lang.String // LOCAL VARIABLES JVM // test.kt:-1 invoke: @@ -55,4 +55,4 @@ suspend fun box() = foo(A()) { (x_param, _, y_param) -> // LOCAL VARIABLES // test.kt:10 foo: a:A=A, block:kotlin.jvm.functions.Function2=TestKt$box$2, $completion:kotlin.coroutines.Continuation=helpers.ResultContinuation -// test.kt:14 box: $completion:kotlin.coroutines.Continuation=helpers.ResultContinuation \ No newline at end of file +// test.kt:14 box: $completion:kotlin.coroutines.Continuation=helpers.ResultContinuation diff --git a/gradle/verification-metadata.xml b/gradle/verification-metadata.xml index 7b8c8e1bcc9..49b89be1f67 100644 --- a/gradle/verification-metadata.xml +++ b/gradle/verification-metadata.xml @@ -333,10 +333,10 @@ - - - - + + + +