JVM: fix remapping of new T inside regenerated copies of T

In general, `InliningContext.findAnonymousTransformationInfo` was not reliable because it mapped each type to *some* info for that type, preferring ones with `shouldRegenerate == true` if those exist. Thus, it returned incorrect results if one type was regenerated multiple times, e.g. in a nested inlining context or because of a `finally` (which duplicates anonymous objects). The solution is to avoid a global map and attach the current transformation info directly to the current inlining context.
2020-04-14 18:03:53 +02:00
parent 3f8ffb5ea7
commit 456469eb3b
10 changed files with 153 additions and 63 deletions
@@ -27,7 +27,8 @@ class RegeneratedClassContext(
    nameGenerator: NameGenerator,
    typeRemapper: TypeRemapper,
    lambdaInfo: LambdaInfo?,
-    override val callSiteInfo: InlineCallSiteInfo
+    override val callSiteInfo: InlineCallSiteInfo,
+    override val transformationInfo: TransformationInfo
 ) : InliningContext(
    parent, expressionMap, state, nameGenerator, typeRemapper, lambdaInfo, true
 ) {
@@ -48,7 +49,8 @@ open class InliningContext(

    var generateAssertField = false

-    private val internalNameToAnonymousObjectTransformationInfo = hashMapOf<String, AnonymousObjectTransformationInfo>()
+    open val transformationInfo: TransformationInfo?
+        get() = null

    var isContinuation: Boolean = false

@@ -57,12 +59,13 @@ open class InliningContext(
    val root: RootInliningContext
        get() = if (isRoot) this as RootInliningContext else parent!!.root

-    fun findAnonymousObjectTransformationInfo(internalName: String, searchInParent: Boolean = true): AnonymousObjectTransformationInfo? =
-        internalNameToAnonymousObjectTransformationInfo[internalName]
-            ?: if (searchInParent) parent?.findAnonymousObjectTransformationInfo(internalName, searchInParent) else null
+    private val regeneratedAnonymousObjects = hashSetOf<String>()

-    fun recordIfNotPresent(internalName: String, info: AnonymousObjectTransformationInfo) {
-        internalNameToAnonymousObjectTransformationInfo.putIfAbsent(internalName, info)
+    fun isRegeneratedAnonymousObject(internalName: String): Boolean =
+        internalName in regeneratedAnonymousObjects || (parent != null && parent.isRegeneratedAnonymousObject(internalName))
+
+    fun recordRegeneratedAnonymousObject(internalName: String) {
+        regeneratedAnonymousObjects.add(internalName)
    }

    fun subInlineLambda(lambdaInfo: LambdaInfo): InliningContext =
@@ -82,10 +85,11 @@ open class InliningContext(
    fun subInlineWithClassRegeneration(
        generator: NameGenerator,
        newTypeMappings: MutableMap<String, String?>,
-        callSiteInfo: InlineCallSiteInfo
+        callSiteInfo: InlineCallSiteInfo,
+        transformationInfo: TransformationInfo
    ): InliningContext = RegeneratedClassContext(
        this, expressionMap, state, generator, TypeRemapper.createFrom(typeRemapper, newTypeMappings),
-        lambdaInfo, callSiteInfo
+        lambdaInfo, callSiteInfo, transformationInfo
    )

    @JvmOverloads
@@ -73,6 +73,9 @@ class MethodInliner(
        if (!inliningContext.isInliningLambda) {
            inliningContext.root.state.globalInlineContext.recordTypeFromInlineFunction(info.oldClassName)
        }
+        if (info.shouldRegenerate(isSameModule)) {
+            inliningContext.recordRegeneratedAnonymousObject(info.oldClassName)
+        }
        transformations.add(info)
    }

@@ -171,7 +174,8 @@ class MethodInliner(
                    val childInliningContext = inliningContext.subInlineWithClassRegeneration(
                        inliningContext.nameGenerator,
                        currentTypeMapping,
-                        inlineCallSiteInfo
+                        inlineCallSiteInfo,
+                        transformationInfo!!
                    )
                    val transformer = transformationInfo!!.createTransformer(
                        childInliningContext, isSameModule, fakeContinuationName
@@ -305,51 +309,33 @@ class MethodInliner(
                    inlineOnlySmapSkipper?.markCallSiteLineNumber(remappingMethodAdapter)
                } else if (isAnonymousConstructorCall(owner, name)) { //TODO add method
                    //TODO add proper message
-                    assert(transformationInfo is AnonymousObjectTransformationInfo) {
+                    var info = transformationInfo as? AnonymousObjectTransformationInfo ?: throw AssertionError(
                        "<init> call doesn't correspond to object transformation info for '$owner.$name': $transformationInfo"
+                    )
+                    val objectConstructsItself = inlineCallSiteInfo.ownerClassName == info.oldClassName
+                    if (objectConstructsItself) {
+                        // inline fun f() -> new f$1 -> fun something() in class f$1 -> new f$1
+                        //                   ^-- fetch the info that was created for this instruction
+                        info = inliningContext.parent?.transformationInfo as? AnonymousObjectTransformationInfo
+                            ?: throw AssertionError("anonymous object $owner constructs itself, but we have no info on in")
                    }
-                    val parent = inliningContext.parent
-                    val shouldRegenerate = transformationInfo!!.shouldRegenerate(isSameModule)
-                    val isContinuation = parent != null && parent.isContinuation
-                    if (shouldRegenerate || isContinuation) {
-                        assert(shouldRegenerate || inlineCallSiteInfo.ownerClassName == transformationInfo!!.oldClassName) { "Only coroutines can call their own constructors" }
-
-                        //put additional captured parameters on stack
-                        var info = transformationInfo as AnonymousObjectTransformationInfo
-
-                        val oldInfo = inliningContext.findAnonymousObjectTransformationInfo(owner)
-                        if (oldInfo != null && isContinuation) {
-                            info = oldInfo
-                        }
-
-                        val isContinuationCreate = isContinuation && oldInfo != null && resultNode.name == "create" &&
-                                resultNode.desc.endsWith(")" + languageVersionSettings.continuationAsmType().descriptor)
-
+                    if (info.shouldRegenerate(isSameModule)) {
                        for (capturedParamDesc in info.allRecapturedParameters) {
-                            if (capturedParamDesc.fieldName == AsmUtil.THIS && isContinuationCreate) {
-                                // Common inliner logic doesn't support cases when transforming anonymous object can
-                                // be instantiated by itself.
-                                // To support such cases workaround with 'oldInfo' is used.
-                                // But it corresponds to outer context and a bit inapplicable for nested 'create' method context.
-                                // 'This' in outer context corresponds to outer instance in current
-                                visitFieldInsn(
-                                    Opcodes.GETSTATIC, owner,
-                                    FieldRemapper.foldName(AsmUtil.CAPTURED_THIS_FIELD), capturedParamDesc.type.descriptor
-                                )
-                            } else {
-                                visitFieldInsn(
-                                    Opcodes.GETSTATIC, capturedParamDesc.containingLambdaName,
-                                    FieldRemapper.foldName(capturedParamDesc.fieldName), capturedParamDesc.type.descriptor
-                                )
-                            }
+                            val realDesc = if (objectConstructsItself && capturedParamDesc.fieldName == AsmUtil.THIS) {
+                                // The captures in `info` are relative to the parent context, so a normal `this` there
+                                // is a captured outer `this` here.
+                                CapturedParamDesc(Type.getObjectType(owner), AsmUtil.CAPTURED_THIS_FIELD, capturedParamDesc.type)
+                            } else capturedParamDesc
+                            visitFieldInsn(
+                                Opcodes.GETSTATIC, realDesc.containingLambdaName,
+                                FieldRemapper.foldName(realDesc.fieldName), realDesc.type.descriptor
+                            )
                        }
                        super.visitMethodInsn(opcode, info.newClassName, name, info.newConstructorDescriptor, itf)

                        //TODO: add new inner class also for other contexts
                        if (inliningContext.parent is RegeneratedClassContext) {
-                            inliningContext.parent.typeRemapper.addAdditionalMappings(
-                                transformationInfo!!.oldClassName, transformationInfo!!.newClassName
-                            )
+                            inliningContext.parent.typeRemapper.addAdditionalMappings(info.oldClassName, info.newClassName)
                        }

                        transformationInfo = null
@@ -826,8 +812,7 @@ class MethodInliner(

    private fun isAnonymousClassThatMustBeRegenerated(type: Type?): Boolean {
        if (type == null || type.sort != Type.OBJECT) return false
-        val info = inliningContext.findAnonymousObjectTransformationInfo(type.internalName)
-        return info != null && info.shouldRegenerate(true)
+        return inliningContext.isRegeneratedAnonymousObject(type.internalName)
    }

    private fun buildConstructorInvocation(
@@ -849,7 +834,7 @@ class MethodInliner(
        // inline lambda cannot possibly reference it, while V is not yet bound so regenerating the object while inlining
        // the lambda into f() is pointless.
        val inNonDefaultLambda = inliningContext.isInliningLambda && inliningContext.lambdaInfo !is DefaultLambda
-        val info = AnonymousObjectTransformationInfo(
+        return AnonymousObjectTransformationInfo(
            anonymousType, needReification && !inNonDefaultLambda, lambdaMapping,
            inliningContext.classRegeneration,
            isAlreadyRegenerated(anonymousType),
@@ -858,19 +843,6 @@ class MethodInliner(
            inliningContext.nameGenerator,
            capturesAnonymousObjectThatMustBeRegenerated
        )
-
-        val memoizeAnonymousObject = inliningContext.findAnonymousObjectTransformationInfo(anonymousType)
-        if (memoizeAnonymousObject == null ||
-            //anonymous object could be inlined in several context without transformation (keeps same class name)
-            // and on further inlining such code some of such cases would be transformed and some not,
-            // so we should distinguish one classes from another more clearly
-            !memoizeAnonymousObject.shouldRegenerate(isSameModule) &&
-            info.shouldRegenerate(isSameModule)
-        ) {
-
-            inliningContext.recordIfNotPresent(anonymousType, info)
-        }
-        return info
    }

    private fun isAlreadyRegenerated(owner: String): Boolean {
@@ -0,0 +1,27 @@
+// WITH_RUNTIME
+// WITH_COROUTINES
+// NO_CHECK_LAMBDA_INLINING
+// FILE: 1.kt
+package test
+
+inline fun foo(crossinline x: () -> Unit) = suspend {
+    try { } finally {
+        // This object is regenerated twice (normal return & "catch Throwable, execute finally, and rethrow")
+        // It doesn't *need* to be, but this should work regardless.
+        { x() }()
+    }
+}
+
+// FILE: 2.kt
+import helpers.*
+import kotlin.coroutines.*
+import kotlin.coroutines.intrinsics.*
+import test.*
+
+var result = "fail"
+fun box(): String {
+    suspend {
+        foo { result = "OK" }()
+    }.startCoroutine(EmptyContinuation)
+    return result
+}
@@ -0,0 +1,27 @@
+// WITH_RUNTIME
+// WITH_COROUTINES
+// NO_CHECK_LAMBDA_INLINING
+// FILE: 1.kt
+package test
+
+inline fun foo(crossinline x: () -> Unit) = suspend {
+    try { } finally {
+        // This object is regenerated twice (normal return & "catch Throwable, execute finally, and rethrow")
+        // It doesn't *need* to be, but this should work regardless.
+        suspend { x() }()
+    }
+}
+
+// FILE: 2.kt
+import helpers.*
+import kotlin.coroutines.*
+import kotlin.coroutines.intrinsics.*
+import test.*
+
+var result = "fail"
+fun box(): String {
+    suspend {
+        foo { result = "OK" }()
+    }.startCoroutine(EmptyContinuation)
+    return result
+}
@@ -3953,6 +3953,16 @@ public class BlackBoxInlineCodegenTestGenerated extends AbstractBlackBoxInlineCo
            runTestWithPackageReplacement("compiler/testData/codegen/boxInline/suspend/tryCatchStackTransform.kt", "kotlin.coroutines");
        }

+        @TestMetadata("twiceRegeneratedAnonymousObject.kt")
+        public void testTwiceRegeneratedAnonymousObject() throws Exception {
+            runTest("compiler/testData/codegen/boxInline/suspend/twiceRegeneratedAnonymousObject.kt");
+        }
+
+        @TestMetadata("twiceRegeneratedSuspendLambda.kt")
+        public void testTwiceRegeneratedSuspendLambda() throws Exception {
+            runTest("compiler/testData/codegen/boxInline/suspend/twiceRegeneratedSuspendLambda.kt");
+        }
+
        @TestMetadata("compiler/testData/codegen/boxInline/suspend/callableReference")
        @TestDataPath("$PROJECT_ROOT")
        @RunWith(JUnit3RunnerWithInners.class)
@@ -3953,6 +3953,16 @@ public class CompileKotlinAgainstInlineKotlinTestGenerated extends AbstractCompi
            runTestWithPackageReplacement("compiler/testData/codegen/boxInline/suspend/tryCatchStackTransform.kt", "kotlin.coroutines");
        }

+        @TestMetadata("twiceRegeneratedAnonymousObject.kt")
+        public void testTwiceRegeneratedAnonymousObject() throws Exception {
+            runTest("compiler/testData/codegen/boxInline/suspend/twiceRegeneratedAnonymousObject.kt");
+        }
+
+        @TestMetadata("twiceRegeneratedSuspendLambda.kt")
+        public void testTwiceRegeneratedSuspendLambda() throws Exception {
+            runTest("compiler/testData/codegen/boxInline/suspend/twiceRegeneratedSuspendLambda.kt");
+        }
+
        @TestMetadata("compiler/testData/codegen/boxInline/suspend/callableReference")
        @TestDataPath("$PROJECT_ROOT")
        @RunWith(JUnit3RunnerWithInners.class)
@@ -3848,6 +3848,16 @@ public class IrBlackBoxInlineCodegenTestGenerated extends AbstractIrBlackBoxInli
            runTestWithPackageReplacement("compiler/testData/codegen/boxInline/suspend/tryCatchStackTransform.kt", "kotlin.coroutines");
        }

+        @TestMetadata("twiceRegeneratedAnonymousObject.kt")
+        public void testTwiceRegeneratedAnonymousObject() throws Exception {
+            runTest("compiler/testData/codegen/boxInline/suspend/twiceRegeneratedAnonymousObject.kt");
+        }
+
+        @TestMetadata("twiceRegeneratedSuspendLambda.kt")
+        public void testTwiceRegeneratedSuspendLambda() throws Exception {
+            runTest("compiler/testData/codegen/boxInline/suspend/twiceRegeneratedSuspendLambda.kt");
+        }
+
        @TestMetadata("compiler/testData/codegen/boxInline/suspend/callableReference")
        @TestDataPath("$PROJECT_ROOT")
        @RunWith(JUnit3RunnerWithInners.class)
@@ -3848,6 +3848,16 @@ public class IrCompileKotlinAgainstInlineKotlinTestGenerated extends AbstractIrC
            runTestWithPackageReplacement("compiler/testData/codegen/boxInline/suspend/tryCatchStackTransform.kt", "kotlin.coroutines");
        }

+        @TestMetadata("twiceRegeneratedAnonymousObject.kt")
+        public void testTwiceRegeneratedAnonymousObject() throws Exception {
+            runTest("compiler/testData/codegen/boxInline/suspend/twiceRegeneratedAnonymousObject.kt");
+        }
+
+        @TestMetadata("twiceRegeneratedSuspendLambda.kt")
+        public void testTwiceRegeneratedSuspendLambda() throws Exception {
+            runTest("compiler/testData/codegen/boxInline/suspend/twiceRegeneratedSuspendLambda.kt");
+        }
+
        @TestMetadata("compiler/testData/codegen/boxInline/suspend/callableReference")
        @TestDataPath("$PROJECT_ROOT")
        @RunWith(JUnit3RunnerWithInners.class)
@@ -3418,6 +3418,16 @@ public class IrJsCodegenInlineTestGenerated extends AbstractIrJsCodegenInlineTes
            runTestWithPackageReplacement("compiler/testData/codegen/boxInline/suspend/tryCatchStackTransform.kt", "kotlin.coroutines");
        }

+        @TestMetadata("twiceRegeneratedAnonymousObject.kt")
+        public void testTwiceRegeneratedAnonymousObject() throws Exception {
+            runTest("compiler/testData/codegen/boxInline/suspend/twiceRegeneratedAnonymousObject.kt");
+        }
+
+        @TestMetadata("twiceRegeneratedSuspendLambda.kt")
+        public void testTwiceRegeneratedSuspendLambda() throws Exception {
+            runTest("compiler/testData/codegen/boxInline/suspend/twiceRegeneratedSuspendLambda.kt");
+        }
+
        @TestMetadata("compiler/testData/codegen/boxInline/suspend/callableReference")
        @TestDataPath("$PROJECT_ROOT")
        @RunWith(JUnit3RunnerWithInners.class)
@@ -3418,6 +3418,16 @@ public class JsCodegenInlineTestGenerated extends AbstractJsCodegenInlineTest {
            runTestWithPackageReplacement("compiler/testData/codegen/boxInline/suspend/tryCatchStackTransform.kt", "kotlin.coroutines");
        }

+        @TestMetadata("twiceRegeneratedAnonymousObject.kt")
+        public void testTwiceRegeneratedAnonymousObject() throws Exception {
+            runTest("compiler/testData/codegen/boxInline/suspend/twiceRegeneratedAnonymousObject.kt");
+        }
+
+        @TestMetadata("twiceRegeneratedSuspendLambda.kt")
+        public void testTwiceRegeneratedSuspendLambda() throws Exception {
+            runTest("compiler/testData/codegen/boxInline/suspend/twiceRegeneratedSuspendLambda.kt");
+        }
+
        @TestMetadata("compiler/testData/codegen/boxInline/suspend/callableReference")
        @TestDataPath("$PROJECT_ROOT")
        @RunWith(JUnit3RunnerWithInners.class)