From 1124887aa04695e80515a2b655ba582eeae95971 Mon Sep 17 00:00:00 2001 From: Mads Ager Date: Thu, 28 May 2020 15:05:18 +0200 Subject: [PATCH] [JVM_IR] Fix stepping behavior for assignments to local variables. --- ...pturedVarsOptimizationMethodTransformer.kt | 19 ++++++++++-- .../backend/jvm/codegen/ExpressionCodegen.kt | 6 ++-- .../jvm/descriptors/SharedVariablesManager.kt | 10 +++---- compiler/testData/debug/stepping/assertion.kt | 11 ++++++- .../debug/stepping/compileTimeConstant.kt | 3 -- .../debug/stepping/functionCallWithDefault.kt | 5 ++-- .../debug/stepping/inlineCallableReference.kt | 17 +++++++++-- .../stepping/simpleDefaultArgWithInline.kt | 29 +++++++++++++++---- .../stepping/variablesWithoutInitializer.kt | 29 +++++++++++++++++++ .../IrSteppingTestGenerated.java | 6 ++++ .../SteppingTestGenerated.java | 6 ++++ 11 files changed, 116 insertions(+), 25 deletions(-) create mode 100644 compiler/testData/debug/stepping/variablesWithoutInitializer.kt diff --git a/compiler/backend/src/org/jetbrains/kotlin/codegen/optimization/CapturedVarsOptimizationMethodTransformer.kt b/compiler/backend/src/org/jetbrains/kotlin/codegen/optimization/CapturedVarsOptimizationMethodTransformer.kt index 4ed2cac5348..2bf01a52891 100644 --- a/compiler/backend/src/org/jetbrains/kotlin/codegen/optimization/CapturedVarsOptimizationMethodTransformer.kt +++ b/compiler/backend/src/org/jetbrains/kotlin/codegen/optimization/CapturedVarsOptimizationMethodTransformer.kt @@ -242,6 +242,17 @@ class CapturedVarsOptimizationMethodTransformer : MethodTransformer() { methodNode.removeUnusedLocalVariables() } + // Be careful to not remove instructions that are the only instruction for a line number. That will + // break debugging. If the previous instruction is a line number and the following instruction is + // a label followed by a line number, insert a nop instead of deleting the instruction. + private fun InsnList.removeOrReplaceByNop(insn: AbstractInsnNode) { + if (insn.previous is LineNumberNode && insn.next is LabelNode && insn.next.next is LineNumberNode) { + set(insn, InsnNode(Opcodes.NOP)) + } else { + remove(insn) + } + } + private fun rewriteRefValue(capturedVar: CapturedVarDescriptor) { methodNode.instructions.run { val localVar = capturedVar.localVar!! @@ -259,9 +270,10 @@ class CapturedVarsOptimizationMethodTransformer : MethodTransformer() { remove(capturedVar.newInsn) remove(capturedVar.initCallInsn!!) - capturedVar.stackInsns.forEach { remove(it) } - capturedVar.aloadInsns.forEach { remove(it) } - capturedVar.astoreInsns.forEach { remove(it) } + + capturedVar.stackInsns.forEach { removeOrReplaceByNop(it) } + capturedVar.aloadInsns.forEach { removeOrReplaceByNop(it) } + capturedVar.astoreInsns.forEach { removeOrReplaceByNop(it) } capturedVar.getFieldInsns.forEach { set(it, VarInsnNode(loadOpcode, capturedVar.localVarIndex)) } capturedVar.putFieldInsns.forEach { set(it, VarInsnNode(storeOpcode, capturedVar.localVarIndex)) } @@ -273,6 +285,7 @@ class CapturedVarsOptimizationMethodTransformer : MethodTransformer() { } } } + } } 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 e7eb4bb6bc0..22ae84d7680 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 @@ -406,6 +406,8 @@ class ExpressionCodegen( mv.dup() } expression is IrDelegatingConstructorCall -> { + expression.markLineNumber(startOffset = true) + // In this case the receiver is `this` (not specified in IR) and the return value is discarded anyway. mv.load(0, OBJECT_TYPE) @@ -502,13 +504,12 @@ class ExpressionCodegen( val varType = typeMapper.mapType(declaration) val index = frameMap.enter(declaration.symbol, varType) - declaration.markLineNumber(startOffset = true) - val initializer = declaration.initializer if (initializer != null) { var value = initializer.accept(this, data) initializer.markLineNumber(startOffset = true) value.materializeAt(varType, declaration.type) + declaration.markLineNumber(startOffset = true) mv.store(index, varType) } else if (declaration.isVisibleInLVT) { pushDefaultValueOnStack(varType, mv) @@ -569,6 +570,7 @@ class ExpressionCodegen( value.materializeAt(fieldType, callee.type) } + expression.markLineNumber(startOffset = true) when { isStatic -> mv.putstatic(ownerName, fieldName, fieldType.descriptor) else -> mv.putfield(ownerName, fieldName, fieldType.descriptor) diff --git a/compiler/ir/backend.jvm/src/org/jetbrains/kotlin/backend/jvm/descriptors/SharedVariablesManager.kt b/compiler/ir/backend.jvm/src/org/jetbrains/kotlin/backend/jvm/descriptors/SharedVariablesManager.kt index 9ad5d3d90b6..3afcbcec49d 100644 --- a/compiler/ir/backend.jvm/src/org/jetbrains/kotlin/backend/jvm/descriptors/SharedVariablesManager.kt +++ b/compiler/ir/backend.jvm/src/org/jetbrains/kotlin/backend/jvm/descriptors/SharedVariablesManager.kt @@ -10,7 +10,6 @@ import org.jetbrains.kotlin.backend.common.ir.createImplicitParameterDeclaration import org.jetbrains.kotlin.backend.jvm.JvmSymbols import org.jetbrains.kotlin.backend.jvm.lower.inlineclasses.InlineClassAbi import org.jetbrains.kotlin.descriptors.ModuleDescriptor -import org.jetbrains.kotlin.descriptors.impl.EmptyPackageFragmentDescriptor import org.jetbrains.kotlin.ir.IrStatement import org.jetbrains.kotlin.ir.builders.declarations.* import org.jetbrains.kotlin.ir.declarations.* @@ -21,7 +20,6 @@ import org.jetbrains.kotlin.ir.descriptors.WrappedVariableDescriptor import org.jetbrains.kotlin.ir.expressions.* import org.jetbrains.kotlin.ir.expressions.impl.* import org.jetbrains.kotlin.ir.symbols.IrVariableSymbol -import org.jetbrains.kotlin.ir.symbols.impl.IrExternalPackageFragmentSymbolImpl import org.jetbrains.kotlin.ir.symbols.impl.IrVariableSymbolImpl import org.jetbrains.kotlin.ir.types.* import org.jetbrains.kotlin.name.FqName @@ -87,7 +85,9 @@ class JvmSharedVariablesManager( val provider = getProvider(InlineClassAbi.unboxType(valueType) ?: valueType) val typeArguments = provider.refClass.typeParameters.map { valueType } val refType = provider.refClass.typeWith(typeArguments) - val refConstructorCall = IrConstructorCallImpl.fromSymbolOwner(refType, provider.refConstructor.symbol).apply { + val refConstructorCall = IrConstructorCallImpl.fromSymbolOwner( + originalDeclaration.startOffset, originalDeclaration.startOffset, refType, provider.refConstructor.symbol + ).apply { typeArguments.forEachIndexed(::putTypeArgument) } return with(originalDeclaration) { @@ -110,8 +110,8 @@ class JvmSharedVariablesManager( // The field is preinitialized to the default value, so an explicit set is not required. return sharedVariableDeclaration } - val initializationStatement = with(initializer) { - IrSetVariableImpl(startOffset, endOffset, irBuiltIns.unitType, originalDeclaration.symbol, this, null) + val initializationStatement = with (originalDeclaration) { + IrSetVariableImpl(startOffset, endOffset, irBuiltIns.unitType, symbol, initializer, null) } val sharedVariableInitialization = setSharedValue(sharedVariableDeclaration.symbol, initializationStatement) return with(originalDeclaration) { diff --git a/compiler/testData/debug/stepping/assertion.kt b/compiler/testData/debug/stepping/assertion.kt index e0befbeafcf..3a03d0bf54d 100644 --- a/compiler/testData/debug/stepping/assertion.kt +++ b/compiler/testData/debug/stepping/assertion.kt @@ -1,5 +1,5 @@ -// IGNORE_BACKEND: JVM_IR // FILE: test.kt + public val MASSERTIONS_ENABLED: Boolean = true public inline fun massert(value: Boolean, lazyMessage: () -> String) { @@ -29,6 +29,12 @@ fun box(): String { return "OK" } +// JVM_IR steps on line 15 both on the way in and on the way out +// of the massert method. This is consistent with what would +// happen if massert was not inline and we used force-step-into +// to step through the $default method. JVM only hits line 15 on +// the way in. + // LINENUMBERS // test.kt:24 box // test.kt:15 box @@ -37,6 +43,9 @@ fun box(): String { // test.kt:16 box // test.kt:17 box // test.kt:21 box +// LINENUMBERS JVM_IR +// test.kt:15 box +// LINENUMBERS // test.kt:25 box // test.kt:6 box // test.kt:3 getMASSERTIONS_ENABLED diff --git a/compiler/testData/debug/stepping/compileTimeConstant.kt b/compiler/testData/debug/stepping/compileTimeConstant.kt index 5f8cc4377db..3476c972c1c 100644 --- a/compiler/testData/debug/stepping/compileTimeConstant.kt +++ b/compiler/testData/debug/stepping/compileTimeConstant.kt @@ -5,9 +5,6 @@ fun box() { 42 } -// IGNORE_BACKEND: JVM_IR -// The JVM_IR backend does not hit line 4. That should be fixed. - // LINENUMBERS // test.kt:5 box // test.kt:4 box diff --git a/compiler/testData/debug/stepping/functionCallWithDefault.kt b/compiler/testData/debug/stepping/functionCallWithDefault.kt index 7d8a3a6ec1b..fc4ccf2feed 100644 --- a/compiler/testData/debug/stepping/functionCallWithDefault.kt +++ b/compiler/testData/debug/stepping/functionCallWithDefault.kt @@ -11,11 +11,10 @@ fun foo(i: Int = 1) { inline fun bar(i: Int = 1) { } -// TODO: The JVM_IR backend has line number 11 for the inlined +// The JVM_IR backend has line number 11 for the inlined // default argument handling both before and after the actual // body of bar. That is consistent with what happens with the -// $default method in the non-inlined case, but it is not what -// happens with the JVM backend. +// $default method in the non-inlined case. // FORCE_STEP_INTO // LINENUMBERS diff --git a/compiler/testData/debug/stepping/inlineCallableReference.kt b/compiler/testData/debug/stepping/inlineCallableReference.kt index c51dcc75bb2..3d6309074e0 100644 --- a/compiler/testData/debug/stepping/inlineCallableReference.kt +++ b/compiler/testData/debug/stepping/inlineCallableReference.kt @@ -4,6 +4,11 @@ fun box() { f { x = true } + var y = + true + f { + y = false + } } inline fun f(block: () -> Unit) { @@ -13,8 +18,16 @@ inline fun f(block: () -> Unit) { // LINENUMBERS // test.kt:3 box // test.kt:4 box -// test.kt:10 box +// test.kt:15 box // test.kt:5 box // test.kt:6 box -// test.kt:11 box +// test.kt:16 box // test.kt:7 box +// test.kt:8 box +// test.kt:7 box +// test.kt:9 box +// test.kt:15 box +// test.kt:10 box +// test.kt:11 box +// test.kt:16 box +// test.kt:12 box \ No newline at end of file diff --git a/compiler/testData/debug/stepping/simpleDefaultArgWithInline.kt b/compiler/testData/debug/stepping/simpleDefaultArgWithInline.kt index 34a622e27c2..5c22debf9bf 100644 --- a/compiler/testData/debug/stepping/simpleDefaultArgWithInline.kt +++ b/compiler/testData/debug/stepping/simpleDefaultArgWithInline.kt @@ -1,15 +1,32 @@ -// IGNORE_BACKEND: JVM_IR // FILE: test.kt + inline fun ifoo(ok: String = "OK"): String { return ok } -fun box(): String { - return ifoo() +fun ifoo2(ok: String = "OK"): String { + return ok } -// TODO: IR Backend behaves like simpleDefaultArg: 8,3,4, _3_ ,8 + +fun box(): String { + ifoo() + return ifoo2() +} + +// JVM_IR backend has the same stepping behavior as +// the non-inlined case when using force step into to step +// through $default method. JVM does not. + +// FORCE_STEP_INTO // LINENUMBERS -// test.kt:8 box +// test.kt:12 box // test.kt:3 box // test.kt:4 box -// test.kt:8 box \ No newline at end of file +// LINENUMBERS JVM_IR +// test.kt:3 box +// LINENUMBERS +// test.kt:13 box +// test.kt:7 ifoo2$default (synthetic) +// test.kt:8 ifoo2 +// test.kt:7 ifoo2$default (synthetic) +// test.kt:13 box \ No newline at end of file diff --git a/compiler/testData/debug/stepping/variablesWithoutInitializer.kt b/compiler/testData/debug/stepping/variablesWithoutInitializer.kt new file mode 100644 index 00000000000..407a6ef17d4 --- /dev/null +++ b/compiler/testData/debug/stepping/variablesWithoutInitializer.kt @@ -0,0 +1,29 @@ +// FILE: test.kt + +fun box() { + var x: String + var y: Int + var z: Boolean + z = false + y = 42 + if (!z) { + x = y.toString() + } +} + +// The JVM IR backend does not generate line number information for the +// declaration of local variables without initializer. Stepping through +// those does not seem useful. This is consistent with javac behavior +// as well. The JVM backend does generate these line numbers. + +// LINENUMBERS +// LINENUMBERS JVM +// test.kt:4 box +// test.kt:5 box +// test.kt:6 box +// LINENUMBERS +// test.kt:7 box +// test.kt:8 box +// test.kt:9 box +// test.kt:10 box +// test.kt:12 box \ No newline at end of file diff --git a/compiler/tests/org/jetbrains/kotlin/codegen/debugInformation/IrSteppingTestGenerated.java b/compiler/tests/org/jetbrains/kotlin/codegen/debugInformation/IrSteppingTestGenerated.java index d9dbfed815b..f9f42064f67 100644 --- a/compiler/tests/org/jetbrains/kotlin/codegen/debugInformation/IrSteppingTestGenerated.java +++ b/compiler/tests/org/jetbrains/kotlin/codegen/debugInformation/IrSteppingTestGenerated.java @@ -391,6 +391,12 @@ public class IrSteppingTestGenerated extends AbstractIrSteppingTest { runTest("compiler/testData/debug/stepping/tryFinally.kt"); } + @Test + @TestMetadata("variablesWithoutInitializer.kt") + public void testVariablesWithoutInitializer() throws Exception { + runTest("compiler/testData/debug/stepping/variablesWithoutInitializer.kt"); + } + @Test @TestMetadata("voidLambdaStepInline.kt") public void testVoidLambdaStepInline() throws Exception { diff --git a/compiler/tests/org/jetbrains/kotlin/codegen/debugInformation/SteppingTestGenerated.java b/compiler/tests/org/jetbrains/kotlin/codegen/debugInformation/SteppingTestGenerated.java index cbac473d1c9..445546ea7e9 100644 --- a/compiler/tests/org/jetbrains/kotlin/codegen/debugInformation/SteppingTestGenerated.java +++ b/compiler/tests/org/jetbrains/kotlin/codegen/debugInformation/SteppingTestGenerated.java @@ -391,6 +391,12 @@ public class SteppingTestGenerated extends AbstractSteppingTest { runTest("compiler/testData/debug/stepping/tryFinally.kt"); } + @Test + @TestMetadata("variablesWithoutInitializer.kt") + public void testVariablesWithoutInitializer() throws Exception { + runTest("compiler/testData/debug/stepping/variablesWithoutInitializer.kt"); + } + @Test @TestMetadata("voidLambdaStepInline.kt") public void testVoidLambdaStepInline() throws Exception {