From 55acc296a24fc02f496f26ccf040a27e7af39ff7 Mon Sep 17 00:00:00 2001 From: pyos Date: Wed, 16 Oct 2019 13:23:57 +0200 Subject: [PATCH] JVM_IR: never create temporaries in $default stubs Given the strict pattern-matching in the inliner, this is the only way to make it not crash when attempting to inline these stubs. Note that the IR backend does not currently use the inliner's default method stub handling; the crash only occurs when a module compiled with the non-IR JVM backend is attempting to call an inline function with default arguments defined in a module that was compiled with the IR backend. --- .../RedundantGotoMethodTransformer.kt | 8 +++- .../lower/DefaultArgumentStubGenerator.kt | 18 ++++---- .../jetbrains/kotlin/backend/jvm/JvmLower.kt | 4 +- .../kotlin/backend/jvm/JvmSymbols.kt | 12 +++++ .../backend/jvm/codegen/ExpressionCodegen.kt | 13 ++++-- .../jvm/intrinsics/IrIntrinsicMethods.kt | 3 +- .../jvm/intrinsics/ReassignParameter.kt | 33 +++++++++++++ .../lower/JvmDefaultArgumentStubGenerator.kt | 46 +++++++++++++++++++ .../argumentOrder/sameOrderWithDefault.kt | 1 - .../defaultArguments/maskCheckSequence.kt | 6 +++ .../codegen/bytecodeText/defaultMethodBody.kt | 1 - .../codegen/BytecodeTextTestGenerated.java | 5 ++ .../ir/IrBytecodeTextTestGenerated.java | 5 ++ 13 files changed, 136 insertions(+), 19 deletions(-) create mode 100644 compiler/ir/backend.jvm/src/org/jetbrains/kotlin/backend/jvm/intrinsics/ReassignParameter.kt create mode 100644 compiler/ir/backend.jvm/src/org/jetbrains/kotlin/backend/jvm/lower/JvmDefaultArgumentStubGenerator.kt create mode 100644 compiler/testData/codegen/bytecodeText/defaultArguments/maskCheckSequence.kt diff --git a/compiler/backend/src/org/jetbrains/kotlin/codegen/optimization/RedundantGotoMethodTransformer.kt b/compiler/backend/src/org/jetbrains/kotlin/codegen/optimization/RedundantGotoMethodTransformer.kt index c005ef93f74..b4dd65063e2 100644 --- a/compiler/backend/src/org/jetbrains/kotlin/codegen/optimization/RedundantGotoMethodTransformer.kt +++ b/compiler/backend/src/org/jetbrains/kotlin/codegen/optimization/RedundantGotoMethodTransformer.kt @@ -31,6 +31,7 @@ class RedundantGotoMethodTransformer : MethodTransformer() { * (1) subsequent labels * ... * goto Label (can be removed) + * nop (any number of them, or maybe none; will be removed by RedundantNopsCleanupMethodTransformer) * Label: * ... * (2) indirect goto @@ -49,6 +50,10 @@ class RedundantGotoMethodTransformer : MethodTransformer() { var pendingGoto: JumpInsnNode? = null for (insn in insns) { + // We can remove jumps over labels, NOPs, GOTOs with the same target, and fake + // instructions used to describe the current frame state. We have to keep jumps + // over line numbers, though, as otherwise something like an `if` with an empty + // `else` will trigger a breakpoint inside the `else` even when the condition is true. when { insn is LabelNode -> { currentLabels.add(insn) @@ -62,8 +67,7 @@ class RedundantGotoMethodTransformer : MethodTransformer() { currentLabels.clear() } } - insn is LineNumberNode -> pendingGoto = null - insn.isMeaningful -> { + insn is LineNumberNode || (insn.isMeaningful && insn.opcode != Opcodes.NOP) -> { currentLabels.clear() pendingGoto = null } diff --git a/compiler/ir/backend.common/src/org/jetbrains/kotlin/backend/common/lower/DefaultArgumentStubGenerator.kt b/compiler/ir/backend.common/src/org/jetbrains/kotlin/backend/common/lower/DefaultArgumentStubGenerator.kt index 16f3160aae6..4eefcd9fac0 100644 --- a/compiler/ir/backend.common/src/org/jetbrains/kotlin/backend/common/lower/DefaultArgumentStubGenerator.kt +++ b/compiler/ir/backend.common/src/org/jetbrains/kotlin/backend/common/lower/DefaultArgumentStubGenerator.kt @@ -108,7 +108,6 @@ open class DefaultArgumentStubGenerator( val expressionBody = valueParameter.defaultValue!! expressionBody.patchDeclarationParents(newIrFunction) - expressionBody.transformChildrenVoid(object : IrElementTransformerVoid() { override fun visitGetValue(expression: IrGetValue): IrExpression { log { "GetValue: ${expression.symbol.owner}" } @@ -117,13 +116,7 @@ open class DefaultArgumentStubGenerator( } }) - val argument = irIfThenElse( - type = parameter.type, - condition = condition, - thenPart = expressionBody.expression, - elsePart = irGet(parameter) - ) - createTmpVariable(argument, nameHint = parameter.name.asString()) + selectArgumentOrDefault(condition, parameter, expressionBody.expression) } else { parameter } @@ -157,6 +150,15 @@ open class DefaultArgumentStubGenerator( return listOf(irFunction, newIrFunction) } + protected open fun IrBlockBodyBuilder.selectArgumentOrDefault( + shouldUseDefault: IrExpression, + parameter: IrValueParameter, + default: IrExpression + ): IrValueDeclaration { + val value = irIfThenElse(parameter.type, shouldUseDefault, default, irGet(parameter)) + return createTmpVariable(value, nameHint = parameter.name.asString()) + } + private fun IrBlockBodyBuilder.dispatchToImplementation( irFunction: IrSimpleFunction, newIrFunction: IrFunction, diff --git a/compiler/ir/backend.jvm/src/org/jetbrains/kotlin/backend/jvm/JvmLower.kt b/compiler/ir/backend.jvm/src/org/jetbrains/kotlin/backend/jvm/JvmLower.kt index 02d605d82dc..e93ecd690f6 100644 --- a/compiler/ir/backend.jvm/src/org/jetbrains/kotlin/backend/jvm/JvmLower.kt +++ b/compiler/ir/backend.jvm/src/org/jetbrains/kotlin/backend/jvm/JvmLower.kt @@ -130,8 +130,8 @@ internal val localDeclarationsPhase = makeIrFilePhase( prerequisite = setOf(callableReferencePhase, sharedVariablesPhase) ) -private val defaultArgumentStubPhase = makeIrFilePhase( - { context -> DefaultArgumentStubGenerator(context, false) }, +private val defaultArgumentStubPhase = makeIrFilePhase( + ::JvmDefaultArgumentStubGenerator, name = "DefaultArgumentsStubGenerator", description = "Generate synthetic stubs for functions with default parameter values", prerequisite = setOf(localDeclarationsPhase) diff --git a/compiler/ir/backend.jvm/src/org/jetbrains/kotlin/backend/jvm/JvmSymbols.kt b/compiler/ir/backend.jvm/src/org/jetbrains/kotlin/backend/jvm/JvmSymbols.kt index b9cb47f5334..68f30643992 100644 --- a/compiler/ir/backend.jvm/src/org/jetbrains/kotlin/backend/jvm/JvmSymbols.kt +++ b/compiler/ir/backend.jvm/src/org/jetbrains/kotlin/backend/jvm/JvmSymbols.kt @@ -378,6 +378,18 @@ class JvmSymbols( returnType = dst.defaultType }.symbol + val reassignParameterIntrinsic: IrSimpleFunctionSymbol = + buildFun { + name = Name.special("") + origin = IrDeclarationOrigin.IR_BUILTINS_STUB + }.apply { + parent = kotlinJvmInternalPackage + val type = addTypeParameter("T", irBuiltIns.anyNType) + addValueParameter("parameter", type.defaultType) // must be IrGetValue of an IrValueParameter + addValueParameter("value", type.defaultType) + returnType = irBuiltIns.unitType + }.symbol + private val collectionToArrayClass: IrClassSymbol = createClass(FqName("kotlin.jvm.internal.CollectionToArray")) { klass -> klass.origin = JvmLoweredDeclarationOrigin.TO_ARRAY diff --git a/compiler/ir/backend.jvm/src/org/jetbrains/kotlin/backend/jvm/codegen/ExpressionCodegen.kt b/compiler/ir/backend.jvm/src/org/jetbrains/kotlin/backend/jvm/codegen/ExpressionCodegen.kt index 97a7d0bc87d..90d0f911f3f 100644 --- a/compiler/ir/backend.jvm/src/org/jetbrains/kotlin/backend/jvm/codegen/ExpressionCodegen.kt +++ b/compiler/ir/backend.jvm/src/org/jetbrains/kotlin/backend/jvm/codegen/ExpressionCodegen.kt @@ -36,6 +36,7 @@ import org.jetbrains.kotlin.ir.declarations.* import org.jetbrains.kotlin.ir.expressions.* import org.jetbrains.kotlin.ir.symbols.IrSymbol import org.jetbrains.kotlin.ir.symbols.IrTypeParameterSymbol +import org.jetbrains.kotlin.ir.symbols.IrValueSymbol import org.jetbrains.kotlin.ir.types.* import org.jetbrains.kotlin.ir.util.* import org.jetbrains.kotlin.ir.visitors.IrElementVisitor @@ -131,7 +132,7 @@ class ExpressionCodegen( val IrExpression.asmType: Type get() = type.asmType - val IrVariable.asmType: Type + val IrValueDeclaration.asmType: Type get() = type.asmType // Assume this expression's result has already been materialized on the stack @@ -512,12 +513,16 @@ class ExpressionCodegen( override fun visitSetVariable(expression: IrSetVariable, data: BlockInfo): PromisedValue { expression.markLineNumber(startOffset = true) - expression.value.markLineNumber(startOffset = true) - expression.value.accept(this, data).coerce(expression.symbol.owner.type).materialize() - mv.store(findLocalIndex(expression.symbol), expression.symbol.owner.asmType) + setVariable(expression.symbol, expression.value, data) return defaultValue(expression.type) } + fun setVariable(symbol: IrValueSymbol, value: IrExpression, data: BlockInfo) { + value.markLineNumber(startOffset = true) + value.accept(this, data).coerce(symbol.owner.type).materialize() + mv.store(findLocalIndex(symbol), symbol.owner.asmType) + } + override fun visitConst(expression: IrConst, data: BlockInfo): PromisedValue { expression.markLineNumber(startOffset = true) when (val value = expression.value) { diff --git a/compiler/ir/backend.jvm/src/org/jetbrains/kotlin/backend/jvm/intrinsics/IrIntrinsicMethods.kt b/compiler/ir/backend.jvm/src/org/jetbrains/kotlin/backend/jvm/intrinsics/IrIntrinsicMethods.kt index 88915f3fa44..57b0f10b14c 100644 --- a/compiler/ir/backend.jvm/src/org/jetbrains/kotlin/backend/jvm/intrinsics/IrIntrinsicMethods.kt +++ b/compiler/ir/backend.jvm/src/org/jetbrains/kotlin/backend/jvm/intrinsics/IrIntrinsicMethods.kt @@ -91,7 +91,8 @@ class IrIntrinsicMethods(val irBuiltIns: IrBuiltIns, val symbols: JvmSymbols) { irBuiltIns.andandSymbol.toKey()!! to AndAnd, irBuiltIns.ororSymbol.toKey()!! to OrOr, symbols.unsafeCoerceIntrinsic.toKey()!! to UnsafeCoerce, - symbols.signatureStringIntrinsic.toKey()!! to SignatureString + symbols.signatureStringIntrinsic.toKey()!! to SignatureString, + symbols.reassignParameterIntrinsic.toKey()!! to ReassignParameter ) + numberConversionMethods() + unaryFunForPrimitives("plus", UnaryPlus) + diff --git a/compiler/ir/backend.jvm/src/org/jetbrains/kotlin/backend/jvm/intrinsics/ReassignParameter.kt b/compiler/ir/backend.jvm/src/org/jetbrains/kotlin/backend/jvm/intrinsics/ReassignParameter.kt new file mode 100644 index 00000000000..61a1be501c2 --- /dev/null +++ b/compiler/ir/backend.jvm/src/org/jetbrains/kotlin/backend/jvm/intrinsics/ReassignParameter.kt @@ -0,0 +1,33 @@ +/* + * Copyright 2010-2019 JetBrains s.r.o. and Kotlin Programming Language contributors. + * Use of this source code is governed by the Apache 2.0 license that can be found in the license/LICENSE.txt file. + */ + +package org.jetbrains.kotlin.backend.jvm.intrinsics + +import org.jetbrains.kotlin.backend.jvm.codegen.* +import org.jetbrains.kotlin.ir.expressions.IrFunctionAccessExpression +import org.jetbrains.kotlin.ir.expressions.IrGetValue +import org.jetbrains.kotlin.ir.symbols.IrValueParameterSymbol + +// A magic intrinsic that changes the supposedly-immutable parameter referenced by its argument: +// +// fun f(x: Int) { +// (x, 1) +// return x +// } +// assert(f(2) == 1) +// +// This *only* works for parameters; any other variable should be made mutable instead. +// +// This is used to optimize default method stubs (and, more importantly, produce bytecode that +// the inliner can actually handle) in `JvmDefaultArgumentStubGenerator`. +object ReassignParameter : IntrinsicMethod() { + override fun invoke(expression: IrFunctionAccessExpression, codegen: ExpressionCodegen, data: BlockInfo): PromisedValue { + val parameterGet = expression.getValueArgument(0) as? IrGetValue + val parameter = parameterGet?.symbol as? IrValueParameterSymbol + ?: throw AssertionError("${expression.getValueArgument(0)} is not a get of a parameter") + codegen.setVariable(parameter, expression.getValueArgument(1)!!, data) + return codegen.immaterialUnitValue + } +} diff --git a/compiler/ir/backend.jvm/src/org/jetbrains/kotlin/backend/jvm/lower/JvmDefaultArgumentStubGenerator.kt b/compiler/ir/backend.jvm/src/org/jetbrains/kotlin/backend/jvm/lower/JvmDefaultArgumentStubGenerator.kt new file mode 100644 index 00000000000..dbc81cb6c63 --- /dev/null +++ b/compiler/ir/backend.jvm/src/org/jetbrains/kotlin/backend/jvm/lower/JvmDefaultArgumentStubGenerator.kt @@ -0,0 +1,46 @@ +/* + * Copyright 2010-2019 JetBrains s.r.o. and Kotlin Programming Language contributors. + * Use of this source code is governed by the Apache 2.0 license that can be found in the license/LICENSE.txt file. + */ + +package org.jetbrains.kotlin.backend.jvm.lower + +import org.jetbrains.kotlin.backend.common.lower.DefaultArgumentStubGenerator +import org.jetbrains.kotlin.backend.common.lower.irIfThen +import org.jetbrains.kotlin.backend.jvm.JvmBackendContext +import org.jetbrains.kotlin.ir.builders.IrBlockBodyBuilder +import org.jetbrains.kotlin.ir.builders.irCall +import org.jetbrains.kotlin.ir.builders.irGet +import org.jetbrains.kotlin.ir.declarations.IrValueDeclaration +import org.jetbrains.kotlin.ir.declarations.IrValueParameter +import org.jetbrains.kotlin.ir.expressions.IrExpression + +class JvmDefaultArgumentStubGenerator(override val context: JvmBackendContext) : DefaultArgumentStubGenerator(context, false, false) { + override fun IrBlockBodyBuilder.selectArgumentOrDefault( + shouldUseDefault: IrExpression, + parameter: IrValueParameter, + default: IrExpression + ): IrValueDeclaration { + // We have to generate precisely this code because that results in the bytecode the inliner expects; + // see `expandMaskConditionsAndUpdateVariableNodes`. In short, the bytecode sequence should be + // + // -- no loads of the parameter here, as after inlining its value will be uninitialized + // ILOAD + // ICONST + // IAND + // IFEQ Lx + // -- any code inserted here is removed if the call site specifies the parameter + // STORE + // -- no jumps here + // Lx + // + // This control flow limits us to an if-then (without an else), and this together with the + // restriction on loading the parameter in the default case means we cannot create any temporaries. + +irIfThen(shouldUseDefault, irCall(this@JvmDefaultArgumentStubGenerator.context.ir.symbols.reassignParameterIntrinsic).apply { + putTypeArgument(0, parameter.type) + putValueArgument(0, irGet(parameter)) + putValueArgument(1, default) + }) + return parameter + } +} diff --git a/compiler/testData/codegen/bytecodeText/argumentOrder/sameOrderWithDefault.kt b/compiler/testData/codegen/bytecodeText/argumentOrder/sameOrderWithDefault.kt index add9536e525..971d79fa3ca 100644 --- a/compiler/testData/codegen/bytecodeText/argumentOrder/sameOrderWithDefault.kt +++ b/compiler/testData/codegen/bytecodeText/argumentOrder/sameOrderWithDefault.kt @@ -1,4 +1,3 @@ -// IGNORE_BACKEND: JVM_IR class A { fun test(x: String? = "x", a: String?, b: String?, y: String? = "y") { } diff --git a/compiler/testData/codegen/bytecodeText/defaultArguments/maskCheckSequence.kt b/compiler/testData/codegen/bytecodeText/defaultArguments/maskCheckSequence.kt new file mode 100644 index 00000000000..ce2230f2ec1 --- /dev/null +++ b/compiler/testData/codegen/bytecodeText/defaultArguments/maskCheckSequence.kt @@ -0,0 +1,6 @@ +inline fun f(x: Int = 1) = x + +// MUST contain these instructions to avoid breaking the inliner. +// See `expandMaskConditionsAndUpdateVariableNodes`. +// 1 ILOAD 1\s*ICONST_1\s*IAND\s*IFEQ L1 +// 1 ICONST_1\s*ISTORE 0\s*(L\d\s*)*L1 diff --git a/compiler/testData/codegen/bytecodeText/defaultMethodBody.kt b/compiler/testData/codegen/bytecodeText/defaultMethodBody.kt index 19f9a10baed..6ce69824f90 100644 --- a/compiler/testData/codegen/bytecodeText/defaultMethodBody.kt +++ b/compiler/testData/codegen/bytecodeText/defaultMethodBody.kt @@ -1,4 +1,3 @@ -// IGNORE_BACKEND: JVM_IR fun test(a: Int, z: String = try{"1"} catch (e: Exception) {"2"}) { "Default body" } diff --git a/compiler/tests/org/jetbrains/kotlin/codegen/BytecodeTextTestGenerated.java b/compiler/tests/org/jetbrains/kotlin/codegen/BytecodeTextTestGenerated.java index 82d6c40927a..c92d9f786d1 100644 --- a/compiler/tests/org/jetbrains/kotlin/codegen/BytecodeTextTestGenerated.java +++ b/compiler/tests/org/jetbrains/kotlin/codegen/BytecodeTextTestGenerated.java @@ -1590,6 +1590,11 @@ public class BytecodeTextTestGenerated extends AbstractBytecodeTextTest { runTest("compiler/testData/codegen/bytecodeText/defaultArguments/maskAndArgumentElimination.kt"); } + @TestMetadata("maskCheckSequence.kt") + public void testMaskCheckSequence() throws Exception { + runTest("compiler/testData/codegen/bytecodeText/defaultArguments/maskCheckSequence.kt"); + } + @TestMetadata("methodHandlerElimination.kt") public void testMethodHandlerElimination() throws Exception { runTest("compiler/testData/codegen/bytecodeText/defaultArguments/methodHandlerElimination.kt"); diff --git a/compiler/tests/org/jetbrains/kotlin/codegen/ir/IrBytecodeTextTestGenerated.java b/compiler/tests/org/jetbrains/kotlin/codegen/ir/IrBytecodeTextTestGenerated.java index 1bdb1744727..8aad27c62f3 100644 --- a/compiler/tests/org/jetbrains/kotlin/codegen/ir/IrBytecodeTextTestGenerated.java +++ b/compiler/tests/org/jetbrains/kotlin/codegen/ir/IrBytecodeTextTestGenerated.java @@ -1545,6 +1545,11 @@ public class IrBytecodeTextTestGenerated extends AbstractIrBytecodeTextTest { runTest("compiler/testData/codegen/bytecodeText/defaultArguments/maskAndArgumentElimination.kt"); } + @TestMetadata("maskCheckSequence.kt") + public void testMaskCheckSequence() throws Exception { + runTest("compiler/testData/codegen/bytecodeText/defaultArguments/maskCheckSequence.kt"); + } + @TestMetadata("methodHandlerElimination.kt") public void testMethodHandlerElimination() throws Exception { runTest("compiler/testData/codegen/bytecodeText/defaultArguments/methodHandlerElimination.kt");