From 04441da0957d0a927a175924d0ac7a737b4dba8b Mon Sep 17 00:00:00 2001 From: Ilmir Usmanov Date: Fri, 1 Nov 2019 02:22:52 +0300 Subject: [PATCH] Replace result on stack with Unit if callee is suspend function returning Unit. Because on resume the result might be not a Unit if the callee is tail-call and its callee return something different from Unit and suspends. Luckily, we generated ReturnsUnitMarker on such calls in all release versions since 1.3. So, even if the code is inline and generated by older versions, it will still work correctly. The only version of the compiler, which does not generate the markers, is 1.3.60-eap-76, because we did not generate the markers since cc06798e2c64f573192ee2300b074fbcf046254c. But I think, this is not an issue. #KT-34703 --- .../kotlin/codegen/ExpressionCodegen.java | 2 + .../CoroutineTransformerMethodVisitor.kt | 50 +++++++++++++++++-- .../codegen/inline/inlineCodegenUtils.kt | 24 +++++++++ 3 files changed, 72 insertions(+), 4 deletions(-) diff --git a/compiler/backend/src/org/jetbrains/kotlin/codegen/ExpressionCodegen.java b/compiler/backend/src/org/jetbrains/kotlin/codegen/ExpressionCodegen.java index af45214f35d..e3be799847d 100644 --- a/compiler/backend/src/org/jetbrains/kotlin/codegen/ExpressionCodegen.java +++ b/compiler/backend/src/org/jetbrains/kotlin/codegen/ExpressionCodegen.java @@ -2533,6 +2533,8 @@ public class ExpressionCodegen extends KtVisitor impleme callGenerator.genCall(callableMethod, resolvedCall, defaultMaskWasGenerated, this); if (isSuspendNoInlineCall) { + addReturnsUnitMarkerIfNecessary(v, resolvedCall); + addSuspendMarker(v, false); addInlineMarker(v, false); } diff --git a/compiler/backend/src/org/jetbrains/kotlin/codegen/coroutines/CoroutineTransformerMethodVisitor.kt b/compiler/backend/src/org/jetbrains/kotlin/codegen/coroutines/CoroutineTransformerMethodVisitor.kt index 11fda09ef6a..50b76f52d21 100644 --- a/compiler/backend/src/org/jetbrains/kotlin/codegen/coroutines/CoroutineTransformerMethodVisitor.kt +++ b/compiler/backend/src/org/jetbrains/kotlin/codegen/coroutines/CoroutineTransformerMethodVisitor.kt @@ -82,8 +82,7 @@ class CoroutineTransformerMethodVisitor( override fun performTransformations(methodNode: MethodNode) { removeFakeContinuationConstructorCall(methodNode) - // Remove redundant markers which came from compiled bytecode - cleanUpReturnsUnitMarkers(methodNode) + replaceReturnsUnitMarkersWithPushingUnitOnStack(methodNode) replaceFakeContinuationsWithRealOnes( methodNode, @@ -228,8 +227,51 @@ class CoroutineTransformerMethodVisitor( ) } - private fun cleanUpReturnsUnitMarkers(methodNode: MethodNode) { - for (marker in methodNode.instructions.asSequence().filter(::isReturnsUnitMarker)) { + /* Put { POP, GETSTATIC Unit } after suspension point if suspension point is a call of suspend function, that returns Unit. + * + * Otherwise, upon resume, the function would seem to not return Unit, despite being declared as returning Unit. + * + * This happens when said function is tail-call and its callee does not return Unit. + * + * Let's have an example + * + * suspend fun int(): Int = suspendCoroutine { ...; 1 } + * + * suspend fun unit() { + * int() + * } + * + * suspend fun main() { + * println(unit()) + * } + * + * So, in order to understand the necessity of { POP, GETSTATIC Unit } inside `main`, we need to consider two different scenarios + * + * 1. `unit` is not a tail-call function. + * 2. `unit` is a tail-call function. + * + * When `unit` is a not tail-call function, calling `resumeWith` on its continuation will resume `unit`, + * it will hit { GETSTATIC Unit; ARETURN } and this Unit will be the result of the suspend call. `unit`'s continuation will then call + * `main` continuation's `resumeWith`, passing the Unit instance. The continuation in turn will resume `main` and the Unit will be + * the result of `unit()` call. This result will then printed. + * + * However, when `unit` is a tail-call function, there is no continuation, generated for it. This is the point of tail-call + * optimization. Thus, resume call will skip `unit` and land direcly in `main` continuation's `resumeWith`. And its result is not + * Unit. Thus, we must ignore this result on call-site and use Unit instead. In other words, POP the result and GETSTATIC Unit + * instead. + */ + private fun replaceReturnsUnitMarkersWithPushingUnitOnStack(methodNode: MethodNode) { + for (marker in methodNode.instructions.asSequence().filter(::isReturnsUnitMarker).toList()) { + assert(marker.next?.next?.let { isAfterSuspendMarker(it) } == true) { + "Expected AfterSuspendMarker after ReturnUnitMarker, got ${marker.next?.next}" + } + methodNode.instructions.insert( + marker.next.next, + withInstructionAdapter { + pop() + getstatic("kotlin/Unit", "INSTANCE", "Lkotlin/Unit;") + } + ) methodNode.instructions.removeAll(listOf(marker.previous, marker)) } } diff --git a/compiler/backend/src/org/jetbrains/kotlin/codegen/inline/inlineCodegenUtils.kt b/compiler/backend/src/org/jetbrains/kotlin/codegen/inline/inlineCodegenUtils.kt index 1114405cb6d..ba16c86568a 100644 --- a/compiler/backend/src/org/jetbrains/kotlin/codegen/inline/inlineCodegenUtils.kt +++ b/compiler/backend/src/org/jetbrains/kotlin/codegen/inline/inlineCodegenUtils.kt @@ -403,6 +403,30 @@ fun addInlineMarker(v: InstructionAdapter, isStartNotEnd: Boolean) { ) } +internal fun addReturnsUnitMarkerIfNecessary(v: InstructionAdapter, resolvedCall: ResolvedCall<*>) { + val wrapperDescriptor = resolvedCall.candidateDescriptor.safeAs() ?: return + val unsubstitutedDescriptor = wrapperDescriptor.unwrapInitialDescriptorForSuspendFunction() + + val typeSubstitutor = TypeSubstitutor.create( + unsubstitutedDescriptor.typeParameters + .withIndex() + .associateBy({ it.value.typeConstructor }) { + TypeProjectionImpl(resolvedCall.typeArguments[wrapperDescriptor.typeParameters[it.index]] ?: return) + } + ) + + val substitutedDescriptor = unsubstitutedDescriptor.substitute(typeSubstitutor) ?: return + val returnType = substitutedDescriptor.returnType ?: return + + if (KotlinBuiltIns.isUnit(returnType)) { + addReturnsUnitMarker(v) + } +} + +private fun addReturnsUnitMarker(v: InstructionAdapter) { + v.emitInlineMarker(INLINE_MARKER_RETURNS_UNIT) +} + fun addSuspendMarker(v: InstructionAdapter, isStartNotEnd: Boolean) { v.emitInlineMarker(if (isStartNotEnd) INLINE_MARKER_BEFORE_SUSPEND_ID else INLINE_MARKER_AFTER_SUSPEND_ID) }