From df225c0c7f7a918ec5438e1d9f2d663d7d9361aa Mon Sep 17 00:00:00 2001 From: Mads Ager Date: Tue, 27 Apr 2021 16:24:31 +0200 Subject: [PATCH] [JVM] Fix uninitialized locals after coroutine transformation. The coroutine transformation would leave locals in the local variable table across the code that reloads local variables from the continuation on reentry. However, when reentering the function the local has no value until after the reloads from the continuation. This change splits the locals in the local variable table to avoid such uninitialized locals. A local alive across a suspension point has its range split in two. One that goes from the original start to the state label for the restart after the suspension. The other goes from after the local has been reloaded from the continuation until the previous end of the local. --- .../CoroutineTransformerMethodVisitor.kt | 103 ++++++++++++------ .../coroutines/debug/thisAndResultInLvt.kt | 10 +- .../varValueConflictsWithTableSameSort.kt | 4 +- .../completion/staticStateMachineReceiver.kt | 4 +- 4 files changed, 81 insertions(+), 40 deletions(-) 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 9dec302abf4..176435bedba 100644 --- a/compiler/backend/src/org/jetbrains/kotlin/codegen/coroutines/CoroutineTransformerMethodVisitor.kt +++ b/compiler/backend/src/org/jetbrains/kotlin/codegen/coroutines/CoroutineTransformerMethodVisitor.kt @@ -650,7 +650,7 @@ class CoroutineTransformerMethodVisitor( val livenessFrames = analyzeLiveness(methodNode) - // References shall be cleaned up after uspill (during spill in next suspension point) to prevent memory leaks, + // References shall be cleaned up after unspill (during spill in next suspension point) to prevent memory leaks, val referencesToSpillBySuspensionPointIndex = arrayListOf>() // while primitives shall not val primitivesToSpillBySuspensionPointIndex = arrayListOf>() @@ -759,6 +759,35 @@ class CoroutineTransformerMethodVisitor( referencesToCleanBySuspensionPointIndex += currentSpilledReferencesCount to predSpilledReferencesCount } + // Calculate debug metadata mapping before modifying method node to make it easier to locate + // locals alive across suspension points. + + fun calculateSpilledVariableAndField( + suspension: SuspensionPoint, + slot: Int, + spillableVariable: SpillableVariable? + ): SpilledVariableAndField? { + if (spillableVariable == null) return null + val name = localVariableName(methodNode, slot, suspension.suspensionCallBegin.index()) ?: return null + return SpilledVariableAndField(spillableVariable.fieldName, name) + } + + val spilledToVariableMapping = arrayListOf>() + for (suspensionPointIndex in suspensionPoints.indices) { + val suspension = suspensionPoints[suspensionPointIndex] + + val spilledToVariable = arrayListOf() + + referencesToSpillBySuspensionPointIndex[suspensionPointIndex].mapNotNullTo(spilledToVariable) { (slot, spillableVariable) -> + calculateSpilledVariableAndField(suspension, slot, spillableVariable) + } + primitivesToSpillBySuspensionPointIndex[suspensionPointIndex].mapNotNullTo(spilledToVariable) { (slot, spillableVariable) -> + calculateSpilledVariableAndField(suspension, slot, spillableVariable) + } + + spilledToVariableMapping += spilledToVariable + } + // Mutate method node fun generateSpillAndUnspill(suspension: SuspensionPoint, slot: Int, spillableVariable: SpillableVariable?) { @@ -772,6 +801,22 @@ class CoroutineTransformerMethodVisitor( return } + // Find and remove the local variable node, if any, in the local variable table corresponding to the slot that is spilled. + var local: LocalVariableNode? = null + val localRestart = LabelNode().linkWithLabel() + val iterator = methodNode.localVariables.listIterator() + while (iterator.hasNext()) { + val node = iterator.next() + if (node.index == slot && + methodNode.instructions.indexOf(node.start) <= methodNode.instructions.indexOf(suspension.suspensionCallBegin) && + methodNode.instructions.indexOf(node.end) > methodNode.instructions.indexOf(suspension.tryCatchBlockEndLabelAfterSuspensionCall) + ) { + local = node + iterator.remove() + break + } + } + with(instructions) { // store variable before suspension call insertBefore(suspension.suspensionCallBegin, withInstructionAdapter { @@ -795,8 +840,31 @@ class CoroutineTransformerMethodVisitor( ) StackValue.coerce(spillableVariable.normalizedType, spillableVariable.type, this) store(slot, spillableVariable.type) + if (local != null) { + visitLabel(localRestart.label) + } }) } + + // Split the local variable range for the local so that it is visible until the next state label, but is + // not visible until it has been unspilled from the continuation on the reentry path. + if (local != null) { + val previousEnd = local.end + local.end = suspension.stateLabel + // Add the local back, but end it at the next state label. + methodNode.localVariables.add(local) + // Add a new entry that starts after the local variable is restored from the continuation. + methodNode.localVariables.add( + LocalVariableNode( + local.name, + local.desc, + local.signature, + localRestart, + previousEnd, + local.index + ) + ) + } } fun cleanUpField(suspension: SuspensionPoint, fieldIndex: Int) { @@ -839,33 +907,6 @@ class CoroutineTransformerMethodVisitor( } } - // Calculate debug metadata mapping - - fun calculateSpilledVariableAndField( - suspension: SuspensionPoint, - slot: Int, - spillableVariable: SpillableVariable? - ): SpilledVariableAndField? { - if (spillableVariable == null) return null - val name = localVariableName(methodNode, slot, suspension.suspensionCallEnd.next.index()) ?: return null - return SpilledVariableAndField(spillableVariable.fieldName, name) - } - - val spilledToVariableMapping = arrayListOf>() - for (suspensionPointIndex in suspensionPoints.indices) { - val suspension = suspensionPoints[suspensionPointIndex] - - val spilledToVariable = arrayListOf() - - referencesToSpillBySuspensionPointIndex[suspensionPointIndex].mapNotNullTo(spilledToVariable) { (slot, spillableVariable) -> - calculateSpilledVariableAndField(suspension, slot, spillableVariable) - } - primitivesToSpillBySuspensionPointIndex[suspensionPointIndex].mapNotNullTo(spilledToVariable) { (slot, spillableVariable) -> - calculateSpilledVariableAndField(suspension, slot, spillableVariable) - } - - spilledToVariableMapping += spilledToVariable - } return spilledToVariableMapping } @@ -901,7 +942,6 @@ class CoroutineTransformerMethodVisitor( suspendMarkerVarIndex: Int, suspendPointLineNumber: LineNumberNode? ): LabelNode { - val stateLabel = LabelNode().linkWithLabel() val continuationLabelAfterLoadedResult = LabelNode() val suspendElementLineNumber = lineNumber var nextLineNumberNode = nextDefinitelyHitLineNumber(suspension) @@ -929,7 +969,7 @@ class CoroutineTransformerMethodVisitor( load(suspendMarkerVarIndex, AsmTypes.OBJECT_TYPE) areturn(AsmTypes.OBJECT_TYPE) // Mark place for continuation - visitLabel(stateLabel.label) + visitLabel(suspension.stateLabel.label) }) // After suspension point there is always three nodes: L1, NOP, L2 @@ -985,7 +1025,7 @@ class CoroutineTransformerMethodVisitor( } } - return stateLabel + return suspension.stateLabel } // Find the next line number instruction that is defintely hit. That is, a line number @@ -1154,6 +1194,7 @@ internal class SuspensionPoint( ) { lateinit var tryCatchBlocksContinuationLabel: LabelNode + val stateLabel = LabelNode().linkWithLabel() val unboxInlineClassInstructions: List = findUnboxInlineClassInstructions() private fun findUnboxInlineClassInstructions(): List { diff --git a/compiler/testData/codegen/bytecodeText/coroutines/debug/thisAndResultInLvt.kt b/compiler/testData/codegen/bytecodeText/coroutines/debug/thisAndResultInLvt.kt index bd448d25da7..8d82b4840f7 100644 --- a/compiler/testData/codegen/bytecodeText/coroutines/debug/thisAndResultInLvt.kt +++ b/compiler/testData/codegen/bytecodeText/coroutines/debug/thisAndResultInLvt.kt @@ -23,11 +23,11 @@ class A { // foo, c's lambda and foo's continuation // 3 LOCALVARIABLE \$result Ljava/lang/Object; -// foo and -// 2 LOCALVARIABLE this LA; -// 1 LOCALVARIABLE a LA; -// 1 LOCALVARIABLE s Ljava/lang/String; -// 1 LOCALVARIABLE block Lkotlin/jvm/functions/Function2; +// foo x 3 since we split the local over restore code for the two calls to block(), and +// 4 LOCALVARIABLE this LA; +// 3 LOCALVARIABLE a LA; +// 3 LOCALVARIABLE s Ljava/lang/String; +// 3 LOCALVARIABLE block Lkotlin/jvm/functions/Function2; // 1 LOCALVARIABLE \$continuation Lkotlin/coroutines/Continuation; // JVM_TEMPLATES diff --git a/compiler/testData/codegen/bytecodeText/coroutines/varValueConflictsWithTableSameSort.kt b/compiler/testData/codegen/bytecodeText/coroutines/varValueConflictsWithTableSameSort.kt index 68e482c8b3e..88f3783d8ce 100644 --- a/compiler/testData/codegen/bytecodeText/coroutines/varValueConflictsWithTableSameSort.kt +++ b/compiler/testData/codegen/bytecodeText/coroutines/varValueConflictsWithTableSameSort.kt @@ -42,8 +42,8 @@ fun box(): String { } // 1 LOCALVARIABLE i Ljava/lang/String; L.* 3 -// We merge LVT records for two consequent branches. -// 1 LOCALVARIABLE s Ljava/lang/String; L.* 3 +// We merge LVT records for two consequent branches, but we split the local over the restore code. +// 2 LOCALVARIABLE s Ljava/lang/String; L.* 3 // 1 PUTFIELD VarValueConflictsWithTableSameSortKt\$box\$1.L\$0 : Ljava/lang/Object; /* 1 load in the catch (e: Throwable) { throw e } block which is implicitly wrapped around try/finally */ // 1 ALOAD 3\s+ATHROW diff --git a/compiler/testData/debug/localVariables/suspend/completion/staticStateMachineReceiver.kt b/compiler/testData/debug/localVariables/suspend/completion/staticStateMachineReceiver.kt index 91ebc4dd6c7..423f9e55639 100644 --- a/compiler/testData/debug/localVariables/suspend/completion/staticStateMachineReceiver.kt +++ b/compiler/testData/debug/localVariables/suspend/completion/staticStateMachineReceiver.kt @@ -32,10 +32,10 @@ suspend fun box() { // test.kt:8 foo1: $continuation:kotlin.coroutines.Continuation=TestKt$foo1$1, $result:java.lang.Object=null, $this$foo1:A=A, l:long=42:long // test.kt:6 foo: $this$foo:A=A, $completion:kotlin.coroutines.Continuation=TestKt$foo1$1 // test.kt:8 foo1: $continuation:kotlin.coroutines.Continuation=TestKt$foo1$1, $result:java.lang.Object=null, $this$foo1:A=A, l:long=42:long -// test.kt:9 foo1: $continuation:kotlin.coroutines.Continuation=TestKt$foo1$1, $result:java.lang.Object=null, $this$foo1:A=A, l:long=42:long +// test.kt:9 foo1: $continuation:kotlin.coroutines.Continuation=TestKt$foo1$1, $result:java.lang.Object=null, l:long=42:long, $this$foo1:A=A // 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: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