From b2f8a4e82a964a68ed55999a5ce63e4c1a3e256d Mon Sep 17 00:00:00 2001 From: Mads Ager Date: Thu, 9 Jan 2020 15:09:52 +0100 Subject: [PATCH] JVM_IR: Put continuation parameter before default mask and handler. This is important for calls using reflection as the reflect library assumes this ordering of arguments. It would be nice if this could be handled in the lowerings. Currently AddContinuationLowering is after DefaultArgumentStubGenerator. If we could add the continuation first and then do default stub generation maybe we could avoid the reshuffling introduced in coroutine codegen in this change. --- .../kotlin/backend/common/ir/IrUtils.kt | 22 ++++++++++- .../backend/jvm/codegen/CoroutineCodegen.kt | 38 +++++++++++++------ .../box/coroutines/reflect/callSuspendBy.kt | 1 - 3 files changed, 47 insertions(+), 14 deletions(-) diff --git a/compiler/ir/backend.common/src/org/jetbrains/kotlin/backend/common/ir/IrUtils.kt b/compiler/ir/backend.common/src/org/jetbrains/kotlin/backend/common/ir/IrUtils.kt index 29e0d076151..06954b45790 100644 --- a/compiler/ir/backend.common/src/org/jetbrains/kotlin/backend/common/ir/IrUtils.kt +++ b/compiler/ir/backend.common/src/org/jetbrains/kotlin/backend/common/ir/IrUtils.kt @@ -176,19 +176,37 @@ fun IrTypeParameter.copyToWithoutSuperTypes( } } -fun IrFunction.copyValueParametersFrom(from: IrFunction) { - // TODO: should dispatch receiver be copied? +private fun IrFunction.copyReceiverParametersFrom(from: IrFunction) { dispatchReceiverParameter = from.dispatchReceiverParameter?.let { IrValueParameterImpl(it.startOffset, it.endOffset, it.origin, it.descriptor, it.type, it.varargElementType).also { it.parent = this } } extensionReceiverParameter = from.extensionReceiverParameter?.copyTo(this) +} +fun IrFunction.copyValueParametersFrom(from: IrFunction) { + copyReceiverParametersFrom(from) val shift = valueParameters.size valueParameters += from.valueParameters.map { it.copyTo(this, index = it.index + shift) } } +fun IrFunction.copyValueParametersInsertingContinuationFrom(from: IrFunction, insertContinuation: () -> Unit) { + copyReceiverParametersFrom(from) + val shift = valueParameters.size + var additionalShift = 0 + from.valueParameters.forEach { + // The continuation parameter goes before the default argument mask and handler. + if (it.origin == IrDeclarationOrigin.MASK_FOR_DEFAULT_FUNCTION) { + insertContinuation() + additionalShift = 1 + } + valueParameters.add(it.copyTo(this, index = it.index + shift + additionalShift)) + } + // If there was no default argument mask and handler, the continuation goes last. + if (additionalShift == 0) insertContinuation() +} + fun IrFunction.copyParameterDeclarationsFrom(from: IrFunction) { assert(typeParameters.isEmpty()) copyTypeParametersFrom(from) diff --git a/compiler/ir/backend.jvm/src/org/jetbrains/kotlin/backend/jvm/codegen/CoroutineCodegen.kt b/compiler/ir/backend.jvm/src/org/jetbrains/kotlin/backend/jvm/codegen/CoroutineCodegen.kt index 08bbd674c20..4b7f1464a7c 100644 --- a/compiler/ir/backend.jvm/src/org/jetbrains/kotlin/backend/jvm/codegen/CoroutineCodegen.kt +++ b/compiler/ir/backend.jvm/src/org/jetbrains/kotlin/backend/jvm/codegen/CoroutineCodegen.kt @@ -7,7 +7,7 @@ package org.jetbrains.kotlin.backend.jvm.codegen import org.jetbrains.kotlin.backend.common.CodegenUtil import org.jetbrains.kotlin.backend.common.ir.copyTypeParametersFrom -import org.jetbrains.kotlin.backend.common.ir.copyValueParametersFrom +import org.jetbrains.kotlin.backend.common.ir.copyValueParametersInsertingContinuationFrom import org.jetbrains.kotlin.backend.common.ir.isSuspend import org.jetbrains.kotlin.backend.common.lower.VariableRemapper import org.jetbrains.kotlin.backend.jvm.JvmBackendContext @@ -165,16 +165,21 @@ private fun IrFunction.suspendFunctionView(context: JvmBackendContext): IrFuncti it.copyAttributes(this) it.copyTypeParametersFrom(this) - it.copyValueParametersFrom(this) - // TODO: this should come before the default masks, if they exist; otherwise, this is not binary - // compatible with the non-IR backend - it.addValueParameter(SUSPEND_FUNCTION_COMPLETION_PARAMETER_NAME, continuationType) + + // Copy the value parameters and insert the continuation parameter. The continuation parameter + // goes before the default argument mask and handler for default argument stubs. + // TODO: It would be nice if AddContinuationLowering could insert the continuation argument before default stub generation. + // That would avoid the reshuffling both here and in createSuspendFunctionClassViewIfNeeded. + var continuationValueParam: IrValueParameter? = null + it.copyValueParametersInsertingContinuationFrom(this) { + continuationValueParam = it.addValueParameter(SUSPEND_FUNCTION_COMPLETION_PARAMETER_NAME, continuationType) + } // Add the suspend function view to the map before transforming the body to make sure // that recursive suspend functions do not lead to unbounded recursion at compile time. context.recordSuspendFunctionView(this, it) - val valueParametersMapping = explicitParameters.zip(it.explicitParameters).toMap() + val valueParametersMapping = explicitParameters.zip(it.explicitParameters.filter { it != continuationValueParam }).toMap() it.body = body?.deepCopyWithSymbols(this) it.body?.transformChildrenVoid(object : VariableRemapper(valueParametersMapping) { // Do not cross class boundaries inside functions. Otherwise, callable references will try to access wrong $completion. @@ -198,18 +203,29 @@ internal fun IrCall.createSuspendFunctionCallViewIfNeeded( it.copyTypeArgumentsFrom(this) it.dispatchReceiver = dispatchReceiver it.extensionReceiver = extensionReceiver - for (i in 0 until valueArgumentsCount) { - it.putValueArgument(i, getValueArgument(i)) - } + // Locate the caller continuation parameter. The continuation parameter is before default argument mask and handler params. + val callerHasDefaultMask = caller.valueParameters.lastOrNull()?.origin == IrDeclarationOrigin.METHOD_HANDLER_IN_DEFAULT_FUNCTION + val callerContinuationIndex = caller.valueParameters.size - 1 - (if (callerHasDefaultMask) 2 else 0) val continuationParameter = when { caller.isInvokeSuspendOfLambda() || caller.isInvokeSuspendOfContinuation() || caller.isInvokeSuspendForInlineOfLambda() -> IrGetValueImpl(UNDEFINED_OFFSET, UNDEFINED_OFFSET, caller.dispatchReceiverParameter!!.symbol) callerIsInlineLambda -> context.fakeContinuation - else -> IrGetValueImpl(UNDEFINED_OFFSET, UNDEFINED_OFFSET, caller.valueParameters.last().symbol) + else -> IrGetValueImpl(UNDEFINED_OFFSET, UNDEFINED_OFFSET, caller.valueParameters[callerContinuationIndex].symbol) } - it.putValueArgument(valueArgumentsCount, continuationParameter) + // If the suspend function view that we are calling has default parameters, we need to make sure to pass the + // continuation before the default parameter mask and handler. + val hasDefaultMask = view.valueParameters.last().origin == IrDeclarationOrigin.METHOD_HANDLER_IN_DEFAULT_FUNCTION + val continuationIndex = valueArgumentsCount - (if (hasDefaultMask) 2 else 0) + for (i in 0 until continuationIndex) { + it.putValueArgument(i, getValueArgument(i)) + } + it.putValueArgument(continuationIndex, continuationParameter) + if (hasDefaultMask) { + it.putValueArgument(valueArgumentsCount - 1, getValueArgument(continuationIndex)) + it.putValueArgument(valueArgumentsCount, getValueArgument(continuationIndex + 1)) + } } } diff --git a/compiler/testData/codegen/box/coroutines/reflect/callSuspendBy.kt b/compiler/testData/codegen/box/coroutines/reflect/callSuspendBy.kt index a151ec3759c..640c0123999 100644 --- a/compiler/testData/codegen/box/coroutines/reflect/callSuspendBy.kt +++ b/compiler/testData/codegen/box/coroutines/reflect/callSuspendBy.kt @@ -2,7 +2,6 @@ // WITH_COROUTINES // WITH_REFLECT // TARGET_BACKEND: JVM -// IGNORE_BACKEND: JVM_IR import helpers.* import kotlin.coroutines.*