From 8255118204c7af77514c7288535ccb03eb8fab62 Mon Sep 17 00:00:00 2001 From: Mads Ager Date: Mon, 22 Nov 2021 11:47:37 +0100 Subject: [PATCH] [JVM] Do not extend local ranges across control-flow merges. The coroutine method transformer extends the range of locals across code where the local is not live when it is safe to do so. However, it only bailed out for one case of control-flow merging, namely backwards branches for loops. That is not sufficient as there can be control flow merges without loops where the local is only defined on one control-flow path. This change generalizes the bailout to any control-flow merge. ^KT-49834 Fixed --- .../CoroutineTransformerMethodVisitor.kt | 97 ++++++++++++------- .../optimization/common/backwardAnalysis.kt | 2 + .../optimization/common/variableLiveness.kt | 16 ++- .../FirBlackBoxCodegenTestGenerated.java | 6 ++ .../box/coroutines/varSpilling/kt49834.kt | 18 ++++ .../codegen/BlackBoxCodegenTestGenerated.java | 6 ++ .../IrBlackBoxCodegenTestGenerated.java | 6 ++ .../LightAnalysisModeTestGenerated.java | 5 + .../js/test/JsCodegenBoxTestGenerated.java | 6 ++ .../test/ir/IrJsCodegenBoxTestGenerated.java | 6 ++ .../IrCodegenBoxWasmTestGenerated.java | 5 + .../NativeExtBlackBoxTestGenerated.java | 6 ++ 12 files changed, 139 insertions(+), 40 deletions(-) create mode 100644 compiler/testData/codegen/box/coroutines/varSpilling/kt49834.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 5c7809aee17..4671f555bb0 100644 --- a/compiler/backend/src/org/jetbrains/kotlin/codegen/coroutines/CoroutineTransformerMethodVisitor.kt +++ b/compiler/backend/src/org/jetbrains/kotlin/codegen/coroutines/CoroutineTransformerMethodVisitor.kt @@ -1251,45 +1251,24 @@ 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 || lvtRecord.name == SUSPEND_CALL_RESULT_NAME) continue - // 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) + // End the local when it is no longer live and then attempt to extend its range when safe. 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, if there is no back-edge + // the original local variable table if there are no control-flow merges. val latest = oldLvtNodeToLatestNewLvtNode[lvtRecord] - // if we can extend the previous range to where the local variable dies, we do not need a + // If we can extend the previous range to where the local variable dies, we do not need a // new entry, we know we cannot extend it to the lvt.endOffset, if we could we would have // done so when we added it below. - val extended = latest?.extendRecordIfPossible(method, suspensionPoints, lvtRecord.end) ?: false + val extended = latest?.extendRecordIfPossible(method, suspensionPoints, lvtRecord.end, liveness) ?: false if (!extended) { val new = LocalVariableNode(lvtRecord.name, lvtRecord.desc, lvtRecord.signature, startLabel, endLabel, lvtRecord.index) oldLvtNodeToLatestNewLvtNode[lvtRecord] = new method.localVariables.add(new) - // see if we can extend it all the way to the old end - new.extendRecordIfPossible(method, suspensionPoints, lvtRecord.end) + // See if we can extend it all the way to the old end. + new.extendRecordIfPossible(method, suspensionPoints, lvtRecord.end, liveness) } } } @@ -1313,26 +1292,71 @@ private fun updateLvtAccordingToLiveness(method: MethodNode, isForNamedFunction: } } -/* We cannot extend a record if there is STORE instruction or a back-edge. - * STORE instructions can signify a unspilling operation, in which case, the variable will become visible before it unspilled, - * back-edges occur in loops. +/* We cannot extend a record if there is STORE instruction or a control-flow merge. + * + * STORE instructions can signify a unspilling operation, in which case, the variable will become visible before it unspilled. + * + * If there is a control-flow merge point in a range where a variable is dead, it might not have been restored on one of the paths + * and therefore it is not safe to extend the record across the control flow merge 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. L1 is a merge point and therefore, we do + * not extend. + * + * 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) + * + * Code such as: + * + * val value = getValue() + * return if (value == null) { + * computeValueAsync() // suspension point + * } else { + * value + * } + "K" + * + * Generates code of this form, where it is not safe to extend the `value` local variable across the control-flow + * merge because it is dead and will not have been restored after the suspend point in one of the branches. + * + * value = getValue() + * if (value != null) goto L2 + * L1: (value dead here) + * temp = computeValueAsync() // suspension point and resumption point, value NOT restored as it is dead + * load temp + * goto L3 + * L2: (value alive here) + * load value + * L3: (merge point, cannot extend `value` local across as it is not defined on one of the paths) + * load "K" + * add strings + * return * * @return true if the range has been extended */ private fun LocalVariableNode.extendRecordIfPossible( method: MethodNode, suspensionPoints: List, - endLabel: LabelNode + endLabel: LabelNode, + liveness: List ): Boolean { val nextSuspensionPointLabel = suspensionPoints.find { it in InsnSequence(end, endLabel) } ?: endLabel var current: AbstractInsnNode? = end + var index = method.instructions.indexOf(current) while (current != null && current != nextSuspensionPointLabel) { - if (current is JumpInsnNode) { - if (method.instructions.indexOf(current.label) < method.instructions.indexOf(current)) { - return false - } - } + if (liveness[index].isControlFlowMerge()) return false // TODO: HACK // TODO: Find correct label, which is OK to be used as end label. if (current.opcode == Opcodes.ARETURN && nextSuspensionPointLabel != endLabel) return false @@ -1340,6 +1364,7 @@ private fun LocalVariableNode.extendRecordIfPossible( return false } current = current.next + ++index } end = nextSuspensionPointLabel return true diff --git a/compiler/backend/src/org/jetbrains/kotlin/codegen/optimization/common/backwardAnalysis.kt b/compiler/backend/src/org/jetbrains/kotlin/codegen/optimization/common/backwardAnalysis.kt index bf5ccbc5a15..6aa5d7142a4 100644 --- a/compiler/backend/src/org/jetbrains/kotlin/codegen/optimization/common/backwardAnalysis.kt +++ b/compiler/backend/src/org/jetbrains/kotlin/codegen/optimization/common/backwardAnalysis.kt @@ -21,6 +21,7 @@ import org.jetbrains.org.objectweb.asm.tree.MethodNode interface VarFrame> { fun mergeFrom(other: F) + fun markControlFlowMerge() override fun equals(other: Any?): Boolean override fun hashCode(): Int } @@ -48,6 +49,7 @@ fun > analyze(node: MethodNode, interpreter: BackwardAnalysisInt for (successorIndex in graph.getSuccessorsIndices(insn)) { newFrame.mergeFrom(frames[successorIndex]) } + if (graph.getPredecessorsIndices(insn).size > 1) newFrame.markControlFlowMerge() interpreter.def(newFrame, insn) interpreter.use(newFrame, insn) diff --git a/compiler/backend/src/org/jetbrains/kotlin/codegen/optimization/common/variableLiveness.kt b/compiler/backend/src/org/jetbrains/kotlin/codegen/optimization/common/variableLiveness.kt index 23e6df14244..4cff1b4bacb 100644 --- a/compiler/backend/src/org/jetbrains/kotlin/codegen/optimization/common/variableLiveness.kt +++ b/compiler/backend/src/org/jetbrains/kotlin/codegen/optimization/common/variableLiveness.kt @@ -14,11 +14,16 @@ import java.util.* class VariableLivenessFrame(val maxLocals: Int) : VarFrame { private val bitSet = BitSet(maxLocals) + private var controlFlowMerge = false override fun mergeFrom(other: VariableLivenessFrame) { bitSet.or(other.bitSet) } + override fun markControlFlowMerge() { + controlFlowMerge = true + } + fun markAlive(varIndex: Int) { bitSet.set(varIndex, true) } @@ -29,14 +34,17 @@ class VariableLivenessFrame(val maxLocals: Int) : VarFrame = @@ -59,4 +67,4 @@ private fun useVar(frame: VariableLivenessFrame, insn: AbstractInsnNode) { } else if (insn is IincInsnNode) { frame.markAlive(insn.`var`) } -} \ No newline at end of file +} diff --git a/compiler/fir/fir2ir/tests-gen/org/jetbrains/kotlin/test/runners/codegen/FirBlackBoxCodegenTestGenerated.java b/compiler/fir/fir2ir/tests-gen/org/jetbrains/kotlin/test/runners/codegen/FirBlackBoxCodegenTestGenerated.java index 5c6d7418e46..b449105110a 100644 --- a/compiler/fir/fir2ir/tests-gen/org/jetbrains/kotlin/test/runners/codegen/FirBlackBoxCodegenTestGenerated.java +++ b/compiler/fir/fir2ir/tests-gen/org/jetbrains/kotlin/test/runners/codegen/FirBlackBoxCodegenTestGenerated.java @@ -12623,6 +12623,12 @@ public class FirBlackBoxCodegenTestGenerated extends AbstractFirBlackBoxCodegenT runTest("compiler/testData/codegen/box/coroutines/varSpilling/kt38925.kt"); } + @Test + @TestMetadata("kt49834.kt") + public void testKt49834() throws Exception { + runTest("compiler/testData/codegen/box/coroutines/varSpilling/kt49834.kt"); + } + @Test @TestMetadata("lvtWithInlineOnly.kt") public void testLvtWithInlineOnly() throws Exception { diff --git a/compiler/testData/codegen/box/coroutines/varSpilling/kt49834.kt b/compiler/testData/codegen/box/coroutines/varSpilling/kt49834.kt new file mode 100644 index 00000000000..da8de7c04bf --- /dev/null +++ b/compiler/testData/codegen/box/coroutines/varSpilling/kt49834.kt @@ -0,0 +1,18 @@ +// WITH_STDLIB + +fun getValue() : String? = null +suspend fun computeValue() = "O" + +suspend fun repro() : String { + val value = getValue() + return if (value == null) { + computeValue() + } else { + value + } + "K" +} + +// This test is checking that the local variable table for `repro` is valid. +// This is checked because the D8 dexer is run on the produced code and +// we fail the tests on warnings because of invalid locals. +fun box() = "OK" diff --git a/compiler/tests-common-new/tests-gen/org/jetbrains/kotlin/test/runners/codegen/BlackBoxCodegenTestGenerated.java b/compiler/tests-common-new/tests-gen/org/jetbrains/kotlin/test/runners/codegen/BlackBoxCodegenTestGenerated.java index ed6b79b847f..ae450c5352f 100644 --- a/compiler/tests-common-new/tests-gen/org/jetbrains/kotlin/test/runners/codegen/BlackBoxCodegenTestGenerated.java +++ b/compiler/tests-common-new/tests-gen/org/jetbrains/kotlin/test/runners/codegen/BlackBoxCodegenTestGenerated.java @@ -12545,6 +12545,12 @@ public class BlackBoxCodegenTestGenerated extends AbstractBlackBoxCodegenTest { runTest("compiler/testData/codegen/box/coroutines/varSpilling/kt38925.kt"); } + @Test + @TestMetadata("kt49834.kt") + public void testKt49834() throws Exception { + runTest("compiler/testData/codegen/box/coroutines/varSpilling/kt49834.kt"); + } + @Test @TestMetadata("lvtWithInlineOnly.kt") public void testLvtWithInlineOnly() throws Exception { diff --git a/compiler/tests-common-new/tests-gen/org/jetbrains/kotlin/test/runners/codegen/IrBlackBoxCodegenTestGenerated.java b/compiler/tests-common-new/tests-gen/org/jetbrains/kotlin/test/runners/codegen/IrBlackBoxCodegenTestGenerated.java index a05b726ba82..dea2da0dff5 100644 --- a/compiler/tests-common-new/tests-gen/org/jetbrains/kotlin/test/runners/codegen/IrBlackBoxCodegenTestGenerated.java +++ b/compiler/tests-common-new/tests-gen/org/jetbrains/kotlin/test/runners/codegen/IrBlackBoxCodegenTestGenerated.java @@ -12623,6 +12623,12 @@ public class IrBlackBoxCodegenTestGenerated extends AbstractIrBlackBoxCodegenTes runTest("compiler/testData/codegen/box/coroutines/varSpilling/kt38925.kt"); } + @Test + @TestMetadata("kt49834.kt") + public void testKt49834() throws Exception { + runTest("compiler/testData/codegen/box/coroutines/varSpilling/kt49834.kt"); + } + @Test @TestMetadata("lvtWithInlineOnly.kt") public void testLvtWithInlineOnly() throws Exception { diff --git a/compiler/tests-gen/org/jetbrains/kotlin/codegen/LightAnalysisModeTestGenerated.java b/compiler/tests-gen/org/jetbrains/kotlin/codegen/LightAnalysisModeTestGenerated.java index 78faa1b4f6d..d3efb6af2a6 100644 --- a/compiler/tests-gen/org/jetbrains/kotlin/codegen/LightAnalysisModeTestGenerated.java +++ b/compiler/tests-gen/org/jetbrains/kotlin/codegen/LightAnalysisModeTestGenerated.java @@ -10121,6 +10121,11 @@ public class LightAnalysisModeTestGenerated extends AbstractLightAnalysisModeTes runTest("compiler/testData/codegen/box/coroutines/varSpilling/kt38925.kt"); } + @TestMetadata("kt49834.kt") + public void testKt49834() throws Exception { + runTest("compiler/testData/codegen/box/coroutines/varSpilling/kt49834.kt"); + } + @TestMetadata("lvtWithInlineOnly.kt") public void testLvtWithInlineOnly() throws Exception { runTest("compiler/testData/codegen/box/coroutines/varSpilling/lvtWithInlineOnly.kt"); diff --git a/js/js.tests/tests-gen/org/jetbrains/kotlin/js/test/JsCodegenBoxTestGenerated.java b/js/js.tests/tests-gen/org/jetbrains/kotlin/js/test/JsCodegenBoxTestGenerated.java index 6edb6b219ba..c6a4c0acc34 100644 --- a/js/js.tests/tests-gen/org/jetbrains/kotlin/js/test/JsCodegenBoxTestGenerated.java +++ b/js/js.tests/tests-gen/org/jetbrains/kotlin/js/test/JsCodegenBoxTestGenerated.java @@ -9403,6 +9403,12 @@ public class JsCodegenBoxTestGenerated extends AbstractJsCodegenBoxTest { runTest("compiler/testData/codegen/box/coroutines/varSpilling/kt38925.kt"); } + @Test + @TestMetadata("kt49834.kt") + public void testKt49834() throws Exception { + runTest("compiler/testData/codegen/box/coroutines/varSpilling/kt49834.kt"); + } + @Test @TestMetadata("lvtWithInlineOnly.kt") public void testLvtWithInlineOnly() throws Exception { diff --git a/js/js.tests/tests-gen/org/jetbrains/kotlin/js/test/ir/IrJsCodegenBoxTestGenerated.java b/js/js.tests/tests-gen/org/jetbrains/kotlin/js/test/ir/IrJsCodegenBoxTestGenerated.java index b75e7bcb797..472c96619e2 100644 --- a/js/js.tests/tests-gen/org/jetbrains/kotlin/js/test/ir/IrJsCodegenBoxTestGenerated.java +++ b/js/js.tests/tests-gen/org/jetbrains/kotlin/js/test/ir/IrJsCodegenBoxTestGenerated.java @@ -9445,6 +9445,12 @@ public class IrJsCodegenBoxTestGenerated extends AbstractIrJsCodegenBoxTest { runTest("compiler/testData/codegen/box/coroutines/varSpilling/kt38925.kt"); } + @Test + @TestMetadata("kt49834.kt") + public void testKt49834() throws Exception { + runTest("compiler/testData/codegen/box/coroutines/varSpilling/kt49834.kt"); + } + @Test @TestMetadata("lvtWithInlineOnly.kt") public void testLvtWithInlineOnly() throws Exception { diff --git a/js/js.tests/tests-gen/org/jetbrains/kotlin/js/testOld/wasm/semantics/IrCodegenBoxWasmTestGenerated.java b/js/js.tests/tests-gen/org/jetbrains/kotlin/js/testOld/wasm/semantics/IrCodegenBoxWasmTestGenerated.java index 0f8f91ccacf..5ff09516cff 100644 --- a/js/js.tests/tests-gen/org/jetbrains/kotlin/js/testOld/wasm/semantics/IrCodegenBoxWasmTestGenerated.java +++ b/js/js.tests/tests-gen/org/jetbrains/kotlin/js/testOld/wasm/semantics/IrCodegenBoxWasmTestGenerated.java @@ -8243,6 +8243,11 @@ public class IrCodegenBoxWasmTestGenerated extends AbstractIrCodegenBoxWasmTest runTest("compiler/testData/codegen/box/coroutines/varSpilling/kt38925.kt"); } + @TestMetadata("kt49834.kt") + public void testKt49834() throws Exception { + runTest("compiler/testData/codegen/box/coroutines/varSpilling/kt49834.kt"); + } + @TestMetadata("lvtWithInlineOnly.kt") public void testLvtWithInlineOnly() throws Exception { runTest("compiler/testData/codegen/box/coroutines/varSpilling/lvtWithInlineOnly.kt"); diff --git a/native/native.tests/tests-gen/org/jetbrains/kotlin/konan/blackboxtest/NativeExtBlackBoxTestGenerated.java b/native/native.tests/tests-gen/org/jetbrains/kotlin/konan/blackboxtest/NativeExtBlackBoxTestGenerated.java index fc3a9fe228d..ce86993d282 100644 --- a/native/native.tests/tests-gen/org/jetbrains/kotlin/konan/blackboxtest/NativeExtBlackBoxTestGenerated.java +++ b/native/native.tests/tests-gen/org/jetbrains/kotlin/konan/blackboxtest/NativeExtBlackBoxTestGenerated.java @@ -12759,6 +12759,12 @@ public class NativeExtBlackBoxTestGenerated extends AbstractNativeBlackBoxTest { runTest("compiler/testData/codegen/box/coroutines/varSpilling/kt38925.kt"); } + @Test + @TestMetadata("kt49834.kt") + public void testKt49834() throws Exception { + runTest("compiler/testData/codegen/box/coroutines/varSpilling/kt49834.kt"); + } + @Test @TestMetadata("lvtWithInlineOnly.kt") public void testLvtWithInlineOnly() throws Exception {