From dddd7413a1d0384958124b1f4bacf1c4c730b867 Mon Sep 17 00:00:00 2001 From: Denis Zharkov Date: Mon, 30 May 2016 11:26:24 +0300 Subject: [PATCH] Fix generated code for suspending in the middle of object construction --- .../CoroutineTransformationClassBuilder.kt | 7 +- .../coroutines/processUninitializedStores.kt | 193 ++++++++++++++++++ .../common/CustomFramesMethodAnalyzer.kt | 29 +++ .../codegen/optimization/common/Util.kt | 2 + .../suspendInTheMiddleOfObjectConstruction.kt | 91 +++++++++ .../codegen/BlackBoxCodegenTestGenerated.java | 6 + 6 files changed, 326 insertions(+), 2 deletions(-) create mode 100644 compiler/backend/src/org/jetbrains/kotlin/codegen/coroutines/processUninitializedStores.kt create mode 100644 compiler/backend/src/org/jetbrains/kotlin/codegen/optimization/common/CustomFramesMethodAnalyzer.kt create mode 100644 compiler/testData/codegen/box/coroutines/suspendInTheMiddleOfObjectConstruction.kt diff --git a/compiler/backend/src/org/jetbrains/kotlin/codegen/coroutines/CoroutineTransformationClassBuilder.kt b/compiler/backend/src/org/jetbrains/kotlin/codegen/coroutines/CoroutineTransformationClassBuilder.kt index 013ae134c59..0b19812337d 100644 --- a/compiler/backend/src/org/jetbrains/kotlin/codegen/coroutines/CoroutineTransformationClassBuilder.kt +++ b/compiler/backend/src/org/jetbrains/kotlin/codegen/coroutines/CoroutineTransformationClassBuilder.kt @@ -21,6 +21,7 @@ import org.jetbrains.kotlin.codegen.optimization.MandatoryMethodTransformer import org.jetbrains.kotlin.codegen.optimization.SKIP_MANDATORY_TRANSFORMATIONS_ANNOTATION_DESC import org.jetbrains.kotlin.codegen.optimization.common.OptimizationBasicInterpreter import org.jetbrains.kotlin.codegen.optimization.common.asSequence +import org.jetbrains.kotlin.codegen.optimization.common.insnListOf import org.jetbrains.kotlin.codegen.optimization.transformer.MethodTransformer import org.jetbrains.kotlin.resolve.jvm.AsmTypes import org.jetbrains.kotlin.resolve.jvm.diagnostics.JvmDeclarationOrigin @@ -64,7 +65,11 @@ class CoroutineTransformerMethodVisitor( if (methodNode.visibleAnnotations?.none { it.desc == CONTINUATION_METHOD_ANNOTATION_DESC } != false) return methodNode.visibleAnnotations.removeAll { it.desc == CONTINUATION_METHOD_ANNOTATION_DESC } + // Spill stack to variables before suspension points MandatoryMethodTransformer().transform("fake", methodNode) + + processUninitializedStores(methodNode) + methodNode.visibleAnnotations.add(AnnotationNode(SKIP_MANDATORY_TRANSFORMATIONS_ANNOTATION_DESC)) val suspensionPoints = collectSuspensionPoints(methodNode) @@ -259,5 +264,3 @@ private fun Type.normalize() = } private class SuspensionPoint(val id: Int, val suspensionCall: MethodInsnNode) - -private fun insnListOf(vararg insns: AbstractInsnNode) = InsnList().apply { insns.forEach { add(it) } } diff --git a/compiler/backend/src/org/jetbrains/kotlin/codegen/coroutines/processUninitializedStores.kt b/compiler/backend/src/org/jetbrains/kotlin/codegen/coroutines/processUninitializedStores.kt new file mode 100644 index 00000000000..a16cab1c7a6 --- /dev/null +++ b/compiler/backend/src/org/jetbrains/kotlin/codegen/coroutines/processUninitializedStores.kt @@ -0,0 +1,193 @@ +/* + * Copyright 2010-2016 JetBrains s.r.o. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.jetbrains.kotlin.codegen.coroutines + +import org.jetbrains.kotlin.codegen.optimization.common.CustomFramesMethodAnalyzer +import org.jetbrains.kotlin.codegen.optimization.common.OptimizationBasicInterpreter +import org.jetbrains.kotlin.codegen.optimization.common.insnListOf +import org.jetbrains.org.objectweb.asm.Opcodes +import org.jetbrains.org.objectweb.asm.Type +import org.jetbrains.org.objectweb.asm.tree.* +import org.jetbrains.org.objectweb.asm.tree.analysis.BasicValue +import org.jetbrains.org.objectweb.asm.tree.analysis.Frame +import org.jetbrains.org.objectweb.asm.tree.analysis.Interpreter + +/** + * In cases like: + * NEW + * DUP + * LDC "First" + * ASTORE 1 + * ASTORE 2 + * ASTORE 3 + * INVOKE suspensionPoint + * ALOAD 3 + * ALOAD 2 + * ALOAD 1 + * LDC "Second" + * INVOKESPECIAL (String;String) + + * Replace store/load instruction with moving NEW/DUP after suspension point: + * LDC "First" + * ASTORE 1 + * INVOKE suspensionPoint + * ALOAD 1 + * LDC "Second" + * ASTORE 5 + * ASTORE 4 + * NEW + * DUP + * ALOAD 4 + * ASTORE 5 + * INVOKESPECIAL (String) + * + * This is needed because later we spill this variables containing uninitialized object into fields -> leads to VerifyError + * Note that this transformation changes semantics a bit (class may be invoked by NEW instruction) + * TODO: current implementation affects all store/loads of uninitialized objects, even valid ones: + * MyClass(try { 1 } catch (e: Exception) { 0 }) // here uninitialized MyClass-object is being spilled before try-catch and then loaded + * + * How this works: + * 1. For each invokespecial determine if NEW uninitialized value was saved to local at least once + * 2. If it wasn't then do nothing + * 3. If it was then: + * - remove all relevant NEW/DUP/LOAD/STORE instructions + * - spill rest of constructor arguments to new local vars + * - generate NEW/DUP + * - restore constructor arguments + */ +internal fun processUninitializedStores(methodNode: MethodNode) { + val interpreter = UninitializedNewValueMarkerInterpreter() + val frames = CustomFramesMethodAnalyzer("fake", methodNode, interpreter, ::UninitializedNewValueFrame).analyze() + + for ((index, insn) in methodNode.instructions.toArray().withIndex()) { + val frame = frames[index] ?: continue + val uninitializedValue = frame.getUninitializedValueForConstructorCall(insn) ?: continue + + val copyUsages: Set = interpreter.uninitializedValuesToCopyUsages[uninitializedValue.newInsn]!! + assert(copyUsages.size > 0) { "At least DUP copy operation expected" } + + // Value generated by NEW wasn't store to local/field (only DUPed) + if (copyUsages.size == 1) continue + + (copyUsages + uninitializedValue.newInsn).forEach { + methodNode.instructions.remove(it) + } + + val indexOfConstructorArgumentFromTopOfStack = Type.getArgumentTypes((insn as MethodInsnNode).desc).size + val storedTypes = arrayListOf() + var nextVarIndex = methodNode.maxLocals + + for (i in 0 until indexOfConstructorArgumentFromTopOfStack) { + val value = frame.getStack(frame.stackSize - 1 - i) + val type = value.type + methodNode.instructions.insertBefore(insn, VarInsnNode(type.getOpcode(Opcodes.ISTORE), nextVarIndex)) + nextVarIndex += type.size + storedTypes.add(type) + } + methodNode.maxLocals = Math.max(methodNode.maxLocals, nextVarIndex) + + methodNode.instructions.insertBefore(insn, insnListOf( + TypeInsnNode(Opcodes.NEW, uninitializedValue.newInsn.desc), + InsnNode(Opcodes.DUP) + )) + + for (type in storedTypes.reversed()) { + nextVarIndex -= type.size + methodNode.instructions.insertBefore(insn, VarInsnNode(type.getOpcode(Opcodes.ILOAD), nextVarIndex)) + } + } +} + +private class UninitializedNewValue( + val newInsn: TypeInsnNode, val internalName: String +) : BasicValue(Type.getObjectType(internalName)) { + override fun toString() = "UninitializedNewValue(internalName='$internalName')" +} + +private class UninitializedNewValueFrame(nLocals: Int, nStack: Int) : Frame(nLocals, nStack) { + override fun execute(insn: AbstractInsnNode, interpreter: Interpreter?) { + val replaceTopValueWithInitialized = getUninitializedValueForConstructorCall(insn) != null + + super.execute(insn, interpreter) + + if (replaceTopValueWithInitialized) { + // Drop top value + val value = pop() as UninitializedNewValue + + // uninitialized value become initialized after call + push(BasicValue(value.type)) + } + } +} + +/** + * @return value generated by NEW that used as 0-th argument of constructor call or null if current instruction is not constructor call + */ +private fun Frame.getUninitializedValueForConstructorCall( + insn: AbstractInsnNode +): UninitializedNewValue? { + if (!insn.isConstructorCall()) return null + + assert(insn.opcode == Opcodes.INVOKESPECIAL) { "Expected opcode Opcodes.INVOKESPECIAL for , but ${insn.opcode} found" } + val paramsCountIncludingReceiver = Type.getArgumentTypes((insn as MethodInsnNode).desc).size + 1 + val newValue = getStack(stackSize - (paramsCountIncludingReceiver + 1)) as? UninitializedNewValue ?: error("Expected value generated with NEW") + + assert(getStack(stackSize - paramsCountIncludingReceiver) is UninitializedNewValue) { + "Next value after NEW should be one generated by DUP" + } + + return newValue +} + +private fun AbstractInsnNode.isConstructorCall() = this is MethodInsnNode && this.name == "" + +private class UninitializedNewValueMarkerInterpreter : OptimizationBasicInterpreter() { + val uninitializedValuesToCopyUsages = hashMapOf>() + override fun newOperation(insn: AbstractInsnNode): BasicValue? { + if (insn.opcode == Opcodes.NEW) { + uninitializedValuesToCopyUsages[insn] = mutableSetOf() + return UninitializedNewValue(insn as TypeInsnNode, insn.desc) + } + return super.newOperation(insn) + } + + override fun copyOperation(insn: AbstractInsnNode, value: BasicValue?): BasicValue? { + if (value is UninitializedNewValue) { + uninitializedValuesToCopyUsages[value.newInsn]!!.add(insn) + return value + } + return super.copyOperation(insn, value) + } + + override fun merge(v: BasicValue, w: BasicValue): BasicValue { + if (v === w) return v + if (v === BasicValue.UNINITIALIZED_VALUE || w === BasicValue.UNINITIALIZED_VALUE) { + return BasicValue.UNINITIALIZED_VALUE + } + + if (v is UninitializedNewValue || w is UninitializedNewValue) { + if ((v as? UninitializedNewValue)?.newInsn !== (w as? UninitializedNewValue)?.newInsn) { + // Merge of two different ANEW result is possible, but such values should not be used further + return BasicValue.UNINITIALIZED_VALUE + } + + return v + } + + return super.merge(v, w) + } +} diff --git a/compiler/backend/src/org/jetbrains/kotlin/codegen/optimization/common/CustomFramesMethodAnalyzer.kt b/compiler/backend/src/org/jetbrains/kotlin/codegen/optimization/common/CustomFramesMethodAnalyzer.kt new file mode 100644 index 00000000000..ec80501283f --- /dev/null +++ b/compiler/backend/src/org/jetbrains/kotlin/codegen/optimization/common/CustomFramesMethodAnalyzer.kt @@ -0,0 +1,29 @@ +/* + * Copyright 2010-2016 JetBrains s.r.o. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.jetbrains.kotlin.codegen.optimization.common + +import org.jetbrains.org.objectweb.asm.tree.MethodNode +import org.jetbrains.org.objectweb.asm.tree.analysis.Frame +import org.jetbrains.org.objectweb.asm.tree.analysis.Interpreter +import org.jetbrains.org.objectweb.asm.tree.analysis.Value + +class CustomFramesMethodAnalyzer( + owner: String, method: MethodNode, interpreter: Interpreter, + private val frameFactory: (Int, Int) -> Frame +) : MethodAnalyzer(owner, method, interpreter) { + override fun newFrame(nLocals: Int, nStack: Int) = frameFactory(nLocals, nStack) +} diff --git a/compiler/backend/src/org/jetbrains/kotlin/codegen/optimization/common/Util.kt b/compiler/backend/src/org/jetbrains/kotlin/codegen/optimization/common/Util.kt index ec4ce267914..e88552c117a 100644 --- a/compiler/backend/src/org/jetbrains/kotlin/codegen/optimization/common/Util.kt +++ b/compiler/backend/src/org/jetbrains/kotlin/codegen/optimization/common/Util.kt @@ -119,3 +119,5 @@ val AbstractInsnNode.intConstant: Int? get() = LDC -> (this as LdcInsnNode).cst as? Int else -> null } + +fun insnListOf(vararg insns: AbstractInsnNode) = InsnList().apply { insns.forEach { add(it) } } diff --git a/compiler/testData/codegen/box/coroutines/suspendInTheMiddleOfObjectConstruction.kt b/compiler/testData/codegen/box/coroutines/suspendInTheMiddleOfObjectConstruction.kt new file mode 100644 index 00000000000..3d45dab49f7 --- /dev/null +++ b/compiler/testData/codegen/box/coroutines/suspendInTheMiddleOfObjectConstruction.kt @@ -0,0 +1,91 @@ +class Controller { + suspend fun suspendHere(x: Continuation) { + x.resume("K") + } + + suspend fun suspendWithArgument(v: String, x: Continuation) { + x.resume(v) + } + + suspend fun suspendWithDouble(v: Double, x: Continuation) { + x.resume(v) + } +} + +fun builder(coroutine c: Controller.() -> Continuation) { + c(Controller()).resume(Unit) +} + +class A(val first: String, val second: String) { + override fun toString() = "$first$second" +} +class B(val first: String, val second: String, val third: String) { + override fun toString() = "$first$second$third" +} + +class C(val first: Long, val second: Double, val third: String) { + override fun toString() = "$first#$second#$third" +} + + +fun box(): String { + var result = "OK" + + builder { + var local: Any = A("O", suspendHere()) + + if (local.toString() != "OK") { + result = "fail 1: $local" + return@builder + } + + local = A(suspendWithArgument("O"), suspendHere()) + + if (local.toString() != "OK") { + result = "fail 2: $local" + return@builder + } + + local = B("#", suspendWithArgument("O"), suspendHere()) + + if (local.toString() != "#OK") { + result = "fail 3: $local" + return@builder + } + + local = B(suspendWithArgument("#"), "O", suspendHere()) + + if (local.toString() != "#OK") { + result = "fail 4: $local" + return@builder + } + + local = B("#", B("", "O", suspendWithArgument("")).toString(), suspendHere()) + + if (local.toString() != "#OK") { + result = "fail 5: $local" + return@builder + } + + val condition = local.toString() == "#OK" + + local = B( + if (!condition) "1" else suspendWithArgument("#"), + if (condition) suspendWithArgument("O") else "2", + if (condition) suspendHere() else suspendWithArgument("3")) + + if (local.toString() != "#OK") { + result = "fail 5: $local" + return@builder + } + + local = C(1234567890123L, suspendWithDouble(3.14), suspendWithArgument("OK")) + + if (local.toString() != "1234567890123#3.14#OK") { + result = "fail 5: $local" + return@builder + } + } + + return result +} diff --git a/compiler/tests/org/jetbrains/kotlin/codegen/BlackBoxCodegenTestGenerated.java b/compiler/tests/org/jetbrains/kotlin/codegen/BlackBoxCodegenTestGenerated.java index b0d90d20b3b..ba76dedfd6c 100644 --- a/compiler/tests/org/jetbrains/kotlin/codegen/BlackBoxCodegenTestGenerated.java +++ b/compiler/tests/org/jetbrains/kotlin/codegen/BlackBoxCodegenTestGenerated.java @@ -4176,6 +4176,12 @@ public class BlackBoxCodegenTestGenerated extends AbstractBlackBoxCodegenTest { String fileName = KotlinTestUtils.navigationMetadata("compiler/testData/codegen/box/coroutines/suspendInCycle.kt"); doTest(fileName); } + + @TestMetadata("suspendInTheMiddleOfObjectConstruction.kt") + public void testSuspendInTheMiddleOfObjectConstruction() throws Exception { + String fileName = KotlinTestUtils.navigationMetadata("compiler/testData/codegen/box/coroutines/suspendInTheMiddleOfObjectConstruction.kt"); + doTest(fileName); + } } @TestMetadata("compiler/testData/codegen/box/dataClasses")