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)
2016-07-10 14:08:40 +03:00
parent 434a84eab6
commit 18c2d3070d
7 changed files with 212 additions and 11 deletions
@@ -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<VariableLivenessFrame> {
+    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<VariableLivenessFrame> {
            }

            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<BitSet>) {
-    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<BasicValue>?
+) {
+    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()) {
@@ -0,0 +1,36 @@
+class Controller {
+    suspend fun suspendHere(x: Continuation<String>) {
+        x.resume("OK")
+    }
+}
+
+fun builder(coroutine c: Controller.() -> Continuation<Unit>) {
+    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
+}
@@ -0,0 +1,36 @@
+class Controller {
+    suspend fun suspendHere(x: Continuation<String>) {
+        x.resume("OK")
+    }
+}
+
+fun builder(coroutine c: Controller.() -> Continuation<Unit>) {
+    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
+}
@@ -0,0 +1,42 @@
+class Controller {
+    suspend fun suspendHere(x: Continuation<String>) {
+        x.resume("OK")
+    }
+}
+
+fun builder(coroutine c: Controller.() -> Continuation<Unit>) {
+    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
@@ -0,0 +1,45 @@
+class Controller {
+    suspend fun suspendHere(x: Continuation<String>) {
+        x.resume("OK")
+    }
+}
+
+fun builder(coroutine c: Controller.() -> Continuation<Unit>) {
+    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
@@ -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")
@@ -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)