From 456469eb3b39146b6bf5d4708cd20da60ab8f77b Mon Sep 17 00:00:00 2001 From: pyos Date: Tue, 14 Apr 2020 18:03:53 +0200 Subject: [PATCH] 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. --- .../kotlin/codegen/inline/InliningContext.kt | 22 ++--- .../kotlin/codegen/inline/MethodInliner.kt | 80 ++++++------------- .../twiceRegeneratedAnonymousObject.kt | 27 +++++++ .../suspend/twiceRegeneratedSuspendLambda.kt | 27 +++++++ .../BlackBoxInlineCodegenTestGenerated.java | 10 +++ ...otlinAgainstInlineKotlinTestGenerated.java | 10 +++ .../IrBlackBoxInlineCodegenTestGenerated.java | 10 +++ ...otlinAgainstInlineKotlinTestGenerated.java | 10 +++ .../IrJsCodegenInlineTestGenerated.java | 10 +++ .../JsCodegenInlineTestGenerated.java | 10 +++ 10 files changed, 153 insertions(+), 63 deletions(-) create mode 100644 compiler/testData/codegen/boxInline/suspend/twiceRegeneratedAnonymousObject.kt create mode 100644 compiler/testData/codegen/boxInline/suspend/twiceRegeneratedSuspendLambda.kt diff --git a/compiler/backend/src/org/jetbrains/kotlin/codegen/inline/InliningContext.kt b/compiler/backend/src/org/jetbrains/kotlin/codegen/inline/InliningContext.kt index 6a66814d591..1123336f413 100644 --- a/compiler/backend/src/org/jetbrains/kotlin/codegen/inline/InliningContext.kt +++ b/compiler/backend/src/org/jetbrains/kotlin/codegen/inline/InliningContext.kt @@ -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() + 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() - 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, - callSiteInfo: InlineCallSiteInfo + callSiteInfo: InlineCallSiteInfo, + transformationInfo: TransformationInfo ): InliningContext = RegeneratedClassContext( this, expressionMap, state, generator, TypeRemapper.createFrom(typeRemapper, newTypeMappings), - lambdaInfo, callSiteInfo + lambdaInfo, callSiteInfo, transformationInfo ) @JvmOverloads diff --git a/compiler/backend/src/org/jetbrains/kotlin/codegen/inline/MethodInliner.kt b/compiler/backend/src/org/jetbrains/kotlin/codegen/inline/MethodInliner.kt index bead579a354..4fb05b10762 100644 --- a/compiler/backend/src/org/jetbrains/kotlin/codegen/inline/MethodInliner.kt +++ b/compiler/backend/src/org/jetbrains/kotlin/codegen/inline/MethodInliner.kt @@ -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( " 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 { diff --git a/compiler/testData/codegen/boxInline/suspend/twiceRegeneratedAnonymousObject.kt b/compiler/testData/codegen/boxInline/suspend/twiceRegeneratedAnonymousObject.kt new file mode 100644 index 00000000000..9f92b2a1a50 --- /dev/null +++ b/compiler/testData/codegen/boxInline/suspend/twiceRegeneratedAnonymousObject.kt @@ -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 +} diff --git a/compiler/testData/codegen/boxInline/suspend/twiceRegeneratedSuspendLambda.kt b/compiler/testData/codegen/boxInline/suspend/twiceRegeneratedSuspendLambda.kt new file mode 100644 index 00000000000..77ba8aa654f --- /dev/null +++ b/compiler/testData/codegen/boxInline/suspend/twiceRegeneratedSuspendLambda.kt @@ -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 +} diff --git a/compiler/tests/org/jetbrains/kotlin/codegen/BlackBoxInlineCodegenTestGenerated.java b/compiler/tests/org/jetbrains/kotlin/codegen/BlackBoxInlineCodegenTestGenerated.java index 2de2205abd6..4e622d6d24e 100644 --- a/compiler/tests/org/jetbrains/kotlin/codegen/BlackBoxInlineCodegenTestGenerated.java +++ b/compiler/tests/org/jetbrains/kotlin/codegen/BlackBoxInlineCodegenTestGenerated.java @@ -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) diff --git a/compiler/tests/org/jetbrains/kotlin/codegen/CompileKotlinAgainstInlineKotlinTestGenerated.java b/compiler/tests/org/jetbrains/kotlin/codegen/CompileKotlinAgainstInlineKotlinTestGenerated.java index 0010462f2cb..e790a47aacc 100644 --- a/compiler/tests/org/jetbrains/kotlin/codegen/CompileKotlinAgainstInlineKotlinTestGenerated.java +++ b/compiler/tests/org/jetbrains/kotlin/codegen/CompileKotlinAgainstInlineKotlinTestGenerated.java @@ -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) diff --git a/compiler/tests/org/jetbrains/kotlin/codegen/ir/IrBlackBoxInlineCodegenTestGenerated.java b/compiler/tests/org/jetbrains/kotlin/codegen/ir/IrBlackBoxInlineCodegenTestGenerated.java index 52d0b4a940b..faac3b839a0 100644 --- a/compiler/tests/org/jetbrains/kotlin/codegen/ir/IrBlackBoxInlineCodegenTestGenerated.java +++ b/compiler/tests/org/jetbrains/kotlin/codegen/ir/IrBlackBoxInlineCodegenTestGenerated.java @@ -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) diff --git a/compiler/tests/org/jetbrains/kotlin/codegen/ir/IrCompileKotlinAgainstInlineKotlinTestGenerated.java b/compiler/tests/org/jetbrains/kotlin/codegen/ir/IrCompileKotlinAgainstInlineKotlinTestGenerated.java index c2e62fe49eb..eaff72b2f2e 100644 --- a/compiler/tests/org/jetbrains/kotlin/codegen/ir/IrCompileKotlinAgainstInlineKotlinTestGenerated.java +++ b/compiler/tests/org/jetbrains/kotlin/codegen/ir/IrCompileKotlinAgainstInlineKotlinTestGenerated.java @@ -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) diff --git a/js/js.tests/test/org/jetbrains/kotlin/js/test/ir/semantics/IrJsCodegenInlineTestGenerated.java b/js/js.tests/test/org/jetbrains/kotlin/js/test/ir/semantics/IrJsCodegenInlineTestGenerated.java index 50ec07abb1a..80146d5aaff 100644 --- a/js/js.tests/test/org/jetbrains/kotlin/js/test/ir/semantics/IrJsCodegenInlineTestGenerated.java +++ b/js/js.tests/test/org/jetbrains/kotlin/js/test/ir/semantics/IrJsCodegenInlineTestGenerated.java @@ -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) diff --git a/js/js.tests/test/org/jetbrains/kotlin/js/test/semantics/JsCodegenInlineTestGenerated.java b/js/js.tests/test/org/jetbrains/kotlin/js/test/semantics/JsCodegenInlineTestGenerated.java index b33d7cbe1a9..c23330608b4 100644 --- a/js/js.tests/test/org/jetbrains/kotlin/js/test/semantics/JsCodegenInlineTestGenerated.java +++ b/js/js.tests/test/org/jetbrains/kotlin/js/test/semantics/JsCodegenInlineTestGenerated.java @@ -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)