From 18c2d3070df893e941ecfb90439b4fad461d6b58 Mon Sep 17 00:00:00 2001 From: Denis Zharkov Date: Sun, 10 Jul 2016 14:08:40 +0300 Subject: [PATCH] Refine variables liveness analysis Do not treat var as alive just because current instruction belongs to an item range in local variables table, but the item has different sort of type As liveness analysis is mostly used in coroutines spilling, not applying this change may lead that to problems on Android (see tests) --- .../optimization/common/variableLiveness.kt | 31 ++++++++----- .../coroutines/varValueConflictsWithTable.kt | 36 +++++++++++++++ .../varValueConflictsWithTableSameSort.kt | 36 +++++++++++++++ .../coroutines/varValueConflictsWithTable.kt | 42 +++++++++++++++++ .../varValueConflictsWithTableSameSort.kt | 45 +++++++++++++++++++ .../codegen/BlackBoxCodegenTestGenerated.java | 12 +++++ .../codegen/BytecodeTextTestGenerated.java | 21 +++++++++ 7 files changed, 212 insertions(+), 11 deletions(-) create mode 100644 compiler/testData/codegen/box/coroutines/varValueConflictsWithTable.kt create mode 100644 compiler/testData/codegen/box/coroutines/varValueConflictsWithTableSameSort.kt create mode 100644 compiler/testData/codegen/bytecodeText/coroutines/varValueConflictsWithTable.kt create mode 100644 compiler/testData/codegen/bytecodeText/coroutines/varValueConflictsWithTableSameSort.kt 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 e0f448f03c7..3c9947b529f 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 @@ -16,10 +16,14 @@ package org.jetbrains.kotlin.codegen.optimization.common +import org.jetbrains.kotlin.codegen.optimization.transformer.MethodTransformer +import org.jetbrains.org.objectweb.asm.Type import org.jetbrains.org.objectweb.asm.tree.AbstractInsnNode import org.jetbrains.org.objectweb.asm.tree.IincInsnNode import org.jetbrains.org.objectweb.asm.tree.MethodNode import org.jetbrains.org.objectweb.asm.tree.VarInsnNode +import org.jetbrains.org.objectweb.asm.tree.analysis.BasicValue +import org.jetbrains.org.objectweb.asm.tree.analysis.Frame import java.util.* @@ -49,16 +53,13 @@ class VariableLivenessFrame(val maxLocals: Int) { } fun analyzeLiveness(node: MethodNode): Array { + val typeAnnotatedFrames = MethodTransformer.analyze("fake", node, OptimizationBasicInterpreter()) + val graph = ControlFlowGraph.build(node) val insnList = node.instructions val frames = Array(insnList.size()) { VariableLivenessFrame(node.maxLocals) } val insnArray = insnList.toArray() - val varVisibility = Array(node.maxLocals) { BitSet(insnArray.size) } - for (localVar in node.localVariables) { - varVisibility[localVar.index].set(insnList.indexOf(localVar.start), insnList.indexOf(localVar.end), true) - } - // see Figure 9.16 from Dragon book var wereChanges: Boolean @@ -72,7 +73,7 @@ fun analyzeLiveness(node: MethodNode): Array { } def(newFrame, insn) - use(newFrame, insn, index, varVisibility) + use(newFrame, insn, node, typeAnnotatedFrames[index]) if (frames[index] != newFrame) { frames[index] = newFrame @@ -91,11 +92,19 @@ private fun def(frame: VariableLivenessFrame, insn: AbstractInsnNode) { } } -private fun use(frame: VariableLivenessFrame, insn: AbstractInsnNode, index: Int, varVisibility: Array) { - for (i in 0..frame.maxLocals - 1) { - if (varVisibility[i].get(index)) { - frame.markAlive(i) - } +private fun use( + frame: VariableLivenessFrame, + insn: AbstractInsnNode, + node: MethodNode, + // May be null in case of dead code + typeAnnotatedFrame: Frame? +) { + val index = node.instructions.indexOf(insn) + node.localVariables.filter { + node.instructions.indexOf(it.start) < index && index < node.instructions.indexOf(it.end) && + Type.getType(it.desc).sort == typeAnnotatedFrame?.getLocal(it.index)?.type?.sort + }.forEach { + frame.markAlive(it.index) } if (insn is VarInsnNode && insn.isLoadOperation()) { diff --git a/compiler/testData/codegen/box/coroutines/varValueConflictsWithTable.kt b/compiler/testData/codegen/box/coroutines/varValueConflictsWithTable.kt new file mode 100644 index 00000000000..798641da29a --- /dev/null +++ b/compiler/testData/codegen/box/coroutines/varValueConflictsWithTable.kt @@ -0,0 +1,36 @@ +class Controller { + suspend fun suspendHere(x: Continuation) { + x.resume("OK") + } +} + +fun builder(coroutine c: Controller.() -> Continuation) { + c(Controller()).resume(Unit) +} + +fun box(): String { + var result = "fail 1" + builder { + // Initialize var with Int value + for (i in 1..1) { + if (i != 1) continue + } + + // This variable should take the same slot as 'i' had + var s: String + + // We should not spill 's' to continuation field because it's not initialized + // More precisely it contains a value of wrong type (it conflicts with contents of local var table), + // so an attempt of spilling may lead to problems on Android + if (suspendHere() == "OK") { + s = "OK" + } + else { + s = "fail 2" + } + + result = s + } + + return result +} diff --git a/compiler/testData/codegen/box/coroutines/varValueConflictsWithTableSameSort.kt b/compiler/testData/codegen/box/coroutines/varValueConflictsWithTableSameSort.kt new file mode 100644 index 00000000000..29a3eb3136d --- /dev/null +++ b/compiler/testData/codegen/box/coroutines/varValueConflictsWithTableSameSort.kt @@ -0,0 +1,36 @@ +class Controller { + suspend fun suspendHere(x: Continuation) { + x.resume("OK") + } +} + +fun builder(coroutine c: Controller.() -> Continuation) { + c(Controller()).resume(Unit) +} + +fun box(): String { + var result = "fail 1" + builder { + // Initialize var with Int value + try { + var i: String = "abc" + i = "123" + } finally { } + + // This variable should take the same slot as 'i' had + var s: String + + // We shout not spill 's' to continuation field because it's not effectively initialized + // But we do this because it's not illegal (at least in Android/OpenJDK VM's) + if (suspendHere() == "OK") { + s = "OK" + } + else { + s = "fail 2" + } + + result = s + } + + return result +} diff --git a/compiler/testData/codegen/bytecodeText/coroutines/varValueConflictsWithTable.kt b/compiler/testData/codegen/bytecodeText/coroutines/varValueConflictsWithTable.kt new file mode 100644 index 00000000000..5578a8a0ecf --- /dev/null +++ b/compiler/testData/codegen/bytecodeText/coroutines/varValueConflictsWithTable.kt @@ -0,0 +1,42 @@ +class Controller { + suspend fun suspendHere(x: Continuation) { + x.resume("OK") + } +} + +fun builder(coroutine c: Controller.() -> Continuation) { + c(Controller()).resume(Unit) +} + +fun box(): String { + var result = "fail 1" + builder { + // Initialize var with Int value + for (i in 1..1) { + if ("".length > 0) continue + } + + // This variable should take the same slot as 'i' had + var s: String + + // We should not spill 's' to continuation field because it's not initialized + // More precisely it contains a value of wrong type (it conflicts with contents of local var table), + // so an attempt of spilling may lead to problems on Android + if (suspendHere() == "OK") { + s = "OK" + } + else { + s = "fail 2" + } + + result = s + } + + return result +} + +// 1 LOCALVARIABLE i I L.* 3 +// 1 LOCALVARIABLE s Ljava/lang/String; L.* 3 +// 0 PUTFIELD VarValueConflictsWithTableKt\$box\$1.I\$0 : I +/* 2 loads in cycle */ +// 2 ILOAD 3 diff --git a/compiler/testData/codegen/bytecodeText/coroutines/varValueConflictsWithTableSameSort.kt b/compiler/testData/codegen/bytecodeText/coroutines/varValueConflictsWithTableSameSort.kt new file mode 100644 index 00000000000..a5bf8f07b4e --- /dev/null +++ b/compiler/testData/codegen/bytecodeText/coroutines/varValueConflictsWithTableSameSort.kt @@ -0,0 +1,45 @@ +class Controller { + suspend fun suspendHere(x: Continuation) { + x.resume("OK") + } +} + +fun builder(coroutine c: Controller.() -> Continuation) { + c(Controller()).resume(Unit) +} + +fun box(): String { + var result = "fail 1" + + builder { + // Initialize var with Int value + try { + var i: String = "abc" + i = "123" + } finally { } + + // This variable should take the same slot as 'i' had + var s: String + + // We shout not spill 's' to continuation field because it's not effectively initialized + // But we do this because it's not illegal (at least in Android/OpenJDK VM's) + if (suspendHere() == "OK") { + s = "OK" + } + else { + s = "fail 2" + } + + result = s + } + + return result +} + +// 1 LOCALVARIABLE i Ljava/lang/String; L.* 3 +// 1 LOCALVARIABLE s Ljava/lang/String; L.* 3 +// 1 PUTFIELD VarValueConflictsWithTableSameSortKt\$box\$1.L\$0 : Ljava/lang/Object; +/* 1 load in try/finally */ +/* 1 load in result = s */ +/* 1 load before suspension point */ +// 3 ALOAD 3 diff --git a/compiler/tests/org/jetbrains/kotlin/codegen/BlackBoxCodegenTestGenerated.java b/compiler/tests/org/jetbrains/kotlin/codegen/BlackBoxCodegenTestGenerated.java index 2ab6fc248bc..8e2b49c5a57 100644 --- a/compiler/tests/org/jetbrains/kotlin/codegen/BlackBoxCodegenTestGenerated.java +++ b/compiler/tests/org/jetbrains/kotlin/codegen/BlackBoxCodegenTestGenerated.java @@ -4386,6 +4386,18 @@ public class BlackBoxCodegenTestGenerated extends AbstractBlackBoxCodegenTest { String fileName = KotlinTestUtils.navigationMetadata("compiler/testData/codegen/box/coroutines/tryFinallyWithHandleResult.kt"); doTest(fileName); } + + @TestMetadata("varValueConflictsWithTable.kt") + public void testVarValueConflictsWithTable() throws Exception { + String fileName = KotlinTestUtils.navigationMetadata("compiler/testData/codegen/box/coroutines/varValueConflictsWithTable.kt"); + doTest(fileName); + } + + @TestMetadata("varValueConflictsWithTableSameSort.kt") + public void testVarValueConflictsWithTableSameSort() throws Exception { + String fileName = KotlinTestUtils.navigationMetadata("compiler/testData/codegen/box/coroutines/varValueConflictsWithTableSameSort.kt"); + doTest(fileName); + } } @TestMetadata("compiler/testData/codegen/box/dataClasses") diff --git a/compiler/tests/org/jetbrains/kotlin/codegen/BytecodeTextTestGenerated.java b/compiler/tests/org/jetbrains/kotlin/codegen/BytecodeTextTestGenerated.java index 37c381a4ddb..8e96e9d7543 100644 --- a/compiler/tests/org/jetbrains/kotlin/codegen/BytecodeTextTestGenerated.java +++ b/compiler/tests/org/jetbrains/kotlin/codegen/BytecodeTextTestGenerated.java @@ -773,6 +773,27 @@ public class BytecodeTextTestGenerated extends AbstractBytecodeTextTest { } } + @TestMetadata("compiler/testData/codegen/bytecodeText/coroutines") + @TestDataPath("$PROJECT_ROOT") + @RunWith(JUnit3RunnerWithInners.class) + public static class Coroutines extends AbstractBytecodeTextTest { + public void testAllFilesPresentInCoroutines() throws Exception { + KotlinTestUtils.assertAllTestsPresentByMetadata(this.getClass(), new File("compiler/testData/codegen/bytecodeText/coroutines"), Pattern.compile("^(.+)\\.kt$"), true); + } + + @TestMetadata("varValueConflictsWithTable.kt") + public void testVarValueConflictsWithTable() throws Exception { + String fileName = KotlinTestUtils.navigationMetadata("compiler/testData/codegen/bytecodeText/coroutines/varValueConflictsWithTable.kt"); + doTest(fileName); + } + + @TestMetadata("varValueConflictsWithTableSameSort.kt") + public void testVarValueConflictsWithTableSameSort() throws Exception { + String fileName = KotlinTestUtils.navigationMetadata("compiler/testData/codegen/bytecodeText/coroutines/varValueConflictsWithTableSameSort.kt"); + doTest(fileName); + } + } + @TestMetadata("compiler/testData/codegen/bytecodeText/deadCodeElimination") @TestDataPath("$PROJECT_ROOT") @RunWith(JUnit3RunnerWithInners.class)