From 281f09f077ba09720c81984d41e6a5032293b163 Mon Sep 17 00:00:00 2001 From: Ilmir Usmanov Date: Fri, 28 Sep 2018 18:20:05 +0300 Subject: [PATCH] Rework check of suspension point inside critical section There is a trade-off between robustness of check and accuracy of the diagnostic: the previous version, which works on generation, was too fragile and lead to false-positives. Now we check on state machine generation. However, since we do not have PSI for call, we can only report diagnostic on whole suspend function or suspend lambda. Additionally, the state machine is generated on crossinline suspend lambdas regeneration and thus we do not have the PSI for the lambda as well! #KT-27130 Fixed #KT-27258 Open --- .../codegen/coroutines/CoroutineCodegen.kt | 3 +- .../CoroutineTransformerMethodVisitor.kt | 42 ++++++++++++++++++- .../SuspendFunctionGenerationStrategy.kt | 3 +- .../inline/AnonymousObjectTransformer.kt | 6 ++- .../diagnostics/DefaultErrorMessagesJvm.java | 2 +- .../resolve/jvm/diagnostics/ErrorsJvm.java | 2 +- .../suspensionPointInMonitor/output.txt | 17 ++++---- .../suspensionPointInMonitor/source.kt | 32 +++++++++++++- 8 files changed, 92 insertions(+), 15 deletions(-) diff --git a/compiler/backend/src/org/jetbrains/kotlin/codegen/coroutines/CoroutineCodegen.kt b/compiler/backend/src/org/jetbrains/kotlin/codegen/coroutines/CoroutineCodegen.kt index 39df85b2caf..3d1e091ad6f 100644 --- a/compiler/backend/src/org/jetbrains/kotlin/codegen/coroutines/CoroutineCodegen.kt +++ b/compiler/backend/src/org/jetbrains/kotlin/codegen/coroutines/CoroutineCodegen.kt @@ -448,7 +448,8 @@ class CoroutineCodegenForLambda private constructor( return CoroutineTransformerMethodVisitor( mv, access, name, desc, null, null, obtainClassBuilderForCoroutineState = { v }, - lineNumber = CodegenUtil.getLineNumberForElement(element, false) ?: 0, + element = element, + diagnostics = state.diagnostics, shouldPreserveClassInitialization = constructorCallNormalizationMode.shouldPreserveClassInitialization, containingClassInternalName = v.thisName, isForNamedFunction = 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 4ea1a7fb1b4..1963a6b0334 100644 --- a/compiler/backend/src/org/jetbrains/kotlin/codegen/coroutines/CoroutineTransformerMethodVisitor.kt +++ b/compiler/backend/src/org/jetbrains/kotlin/codegen/coroutines/CoroutineTransformerMethodVisitor.kt @@ -6,6 +6,7 @@ package org.jetbrains.kotlin.codegen.coroutines import com.intellij.util.containers.Stack +import org.jetbrains.kotlin.backend.common.CodegenUtil import org.jetbrains.kotlin.codegen.AsmUtil import org.jetbrains.kotlin.codegen.ClassBuilder import org.jetbrains.kotlin.codegen.StackValue @@ -19,7 +20,10 @@ import org.jetbrains.kotlin.codegen.optimization.fixStack.top import org.jetbrains.kotlin.codegen.optimization.transformer.MethodTransformer import org.jetbrains.kotlin.config.LanguageVersionSettings import org.jetbrains.kotlin.config.isReleaseCoroutines +import org.jetbrains.kotlin.diagnostics.DiagnosticSink +import org.jetbrains.kotlin.psi.KtElement import org.jetbrains.kotlin.resolve.jvm.AsmTypes +import org.jetbrains.kotlin.resolve.jvm.diagnostics.ErrorsJvm import org.jetbrains.kotlin.resolve.jvm.diagnostics.JvmDeclarationOrigin import org.jetbrains.kotlin.utils.addToStdlib.cast import org.jetbrains.kotlin.utils.addToStdlib.safeAs @@ -56,9 +60,11 @@ class CoroutineTransformerMethodVisitor( obtainClassBuilderForCoroutineState: () -> ClassBuilder, private val isForNamedFunction: Boolean, private val shouldPreserveClassInitialization: Boolean, - private val lineNumber: Int, private val languageVersionSettings: LanguageVersionSettings, private val sourceFile: String, + // These two are needed to report diagnostics about suspension points inside critical section + private val element: KtElement?, + private val diagnostics: DiagnosticSink, // It's only matters for named functions, may differ from '!isStatic(access)' in case of DefaultImpls private val needDispatchReceiver: Boolean = false, // May differ from containingClassInternalName in case of DefaultImpls @@ -68,6 +74,7 @@ class CoroutineTransformerMethodVisitor( ) : TransformationMethodVisitor(delegate, access, name, desc, signature, exceptions) { private val classBuilderForCoroutineState: ClassBuilder by lazy(obtainClassBuilderForCoroutineState) + private val lineNumber = element?.let { CodegenUtil.getLineNumberForElement(it, false) } ?: 0 private var continuationIndex = if (isForNamedFunction) -1 else 0 private var dataIndex = if (isForNamedFunction) -1 else 1 @@ -90,6 +97,8 @@ class CoroutineTransformerMethodVisitor( val suspensionPoints = collectSuspensionPoints(methodNode) + checkForSuspensionPointInsideMonitor(methodNode, suspensionPoints) + // First instruction in the method node may change in case of named function val actualCoroutineStart = methodNode.instructions.first @@ -191,6 +200,37 @@ class CoroutineTransformerMethodVisitor( } } + private fun checkForSuspensionPointInsideMonitor(methodNode: MethodNode, suspensionPoints: List) { + if (methodNode.instructions.asSequence().none { it.opcode == Opcodes.MONITORENTER }) return + + val cfg = ControlFlowGraph.build(methodNode) + val monitorDepthMap = hashMapOf() + fun addMonitorDepthToSuccs(index: Int, depth: Int) { + val insn = methodNode.instructions[index] + monitorDepthMap[insn] = depth + val newDepth = when (insn.opcode) { + Opcodes.MONITORENTER -> depth + 1 + Opcodes.MONITOREXIT -> depth - 1 + else -> depth + } + for (succIndex in cfg.getSuccessorsIndices(index)) { + if (monitorDepthMap[methodNode.instructions[succIndex]] == null) { + addMonitorDepthToSuccs(succIndex, newDepth) + } + } + } + + addMonitorDepthToSuccs(0, 0) + + for (suspensionPoint in suspensionPoints) { + if (monitorDepthMap[suspensionPoint.suspensionCallBegin]?.let { it > 0 } == true) { + // TODO: Support crossinline suspend lambdas + element?.let { diagnostics.report(ErrorsJvm.SUSPENSION_POINT_INSIDE_MONITOR.on(it)) } + return + } + } + } + private fun fixLvtForParameters(methodNode: MethodNode, startLabel: LabelNode, endLabel: LabelNode) { // We need to skip continuation, since the inliner likes to remap variables there. // But this is not a problem, since we have separate $continuation LVT entry diff --git a/compiler/backend/src/org/jetbrains/kotlin/codegen/coroutines/SuspendFunctionGenerationStrategy.kt b/compiler/backend/src/org/jetbrains/kotlin/codegen/coroutines/SuspendFunctionGenerationStrategy.kt index 20385d27f98..733bee7dccf 100644 --- a/compiler/backend/src/org/jetbrains/kotlin/codegen/coroutines/SuspendFunctionGenerationStrategy.kt +++ b/compiler/backend/src/org/jetbrains/kotlin/codegen/coroutines/SuspendFunctionGenerationStrategy.kt @@ -66,7 +66,8 @@ open class SuspendFunctionGenerationStrategy( return CoroutineTransformerMethodVisitor( mv, access, name, desc, null, null, containingClassInternalName, this::classBuilderForCoroutineState, isForNamedFunction = true, - lineNumber = CodegenUtil.getLineNumberForElement(declaration, false) ?: 0, + element = declaration, + diagnostics = state.diagnostics, shouldPreserveClassInitialization = constructorCallNormalizationMode.shouldPreserveClassInitialization, needDispatchReceiver = originalSuspendDescriptor.dispatchReceiverParameter != null, internalNameForDispatchReceiver = containingClassInternalNameOrNull(), diff --git a/compiler/backend/src/org/jetbrains/kotlin/codegen/inline/AnonymousObjectTransformer.kt b/compiler/backend/src/org/jetbrains/kotlin/codegen/inline/AnonymousObjectTransformer.kt index a4852c403cc..2e0d4e811a5 100644 --- a/compiler/backend/src/org/jetbrains/kotlin/codegen/inline/AnonymousObjectTransformer.kt +++ b/compiler/backend/src/org/jetbrains/kotlin/codegen/inline/AnonymousObjectTransformer.kt @@ -448,7 +448,8 @@ class AnonymousObjectTransformer( ArrayUtil.toStringArray(original.exceptions) ), original.access, original.name, original.desc, null, null, obtainClassBuilderForCoroutineState = { builder }, - lineNumber = 0, // <- TODO + element = null, + diagnostics = state.diagnostics, languageVersionSettings = languageVersionSettings, shouldPreserveClassInitialization = state.constructorCallNormalizationMode.shouldPreserveClassInitialization, containingClassInternalName = builder.thisName, @@ -477,7 +478,8 @@ class AnonymousObjectTransformer( ArrayUtil.toStringArray(original.exceptions) ), original.access, original.name, original.desc, null, null, obtainClassBuilderForCoroutineState = { (inliningContext as RegeneratedClassContext).continuationBuilders[continuationClassName]!! }, - lineNumber = 0, // <- TODO + element = null, + diagnostics = state.diagnostics, languageVersionSettings = languageVersionSettings, shouldPreserveClassInitialization = state.constructorCallNormalizationMode.shouldPreserveClassInitialization, containingClassInternalName = builder.thisName, diff --git a/compiler/frontend.java/src/org/jetbrains/kotlin/resolve/jvm/diagnostics/DefaultErrorMessagesJvm.java b/compiler/frontend.java/src/org/jetbrains/kotlin/resolve/jvm/diagnostics/DefaultErrorMessagesJvm.java index d940baddcc1..5a7e0a6c8d3 100644 --- a/compiler/frontend.java/src/org/jetbrains/kotlin/resolve/jvm/diagnostics/DefaultErrorMessagesJvm.java +++ b/compiler/frontend.java/src/org/jetbrains/kotlin/resolve/jvm/diagnostics/DefaultErrorMessagesJvm.java @@ -147,7 +147,7 @@ public class DefaultErrorMessagesJvm implements DefaultErrorMessages.Extension { MAP.put(USAGE_OF_JVM_DEFAULT_THROUGH_SUPER_CALL, "Super calls of '@JvmDefault' members are only allowed with -Xjvm-default option"); MAP.put(NON_JVM_DEFAULT_OVERRIDES_JAVA_DEFAULT, "Non-@JvmDefault interface method cannot override default Java method. Please annotate this method with @JvmDefault"); MAP.put(EXPLICIT_METADATA_IS_DISALLOWED, "Explicit @Metadata is disallowed"); - MAP.put(SUSPENSION_POINT_INSIDE_MONITOR, "The ''{0}'' suspension point is inside a critical section", NAME); + MAP.put(SUSPENSION_POINT_INSIDE_MONITOR, "A suspension point is inside a critical section"); } @NotNull diff --git a/compiler/frontend.java/src/org/jetbrains/kotlin/resolve/jvm/diagnostics/ErrorsJvm.java b/compiler/frontend.java/src/org/jetbrains/kotlin/resolve/jvm/diagnostics/ErrorsJvm.java index c5004fb981a..af54b15c3b4 100644 --- a/compiler/frontend.java/src/org/jetbrains/kotlin/resolve/jvm/diagnostics/ErrorsJvm.java +++ b/compiler/frontend.java/src/org/jetbrains/kotlin/resolve/jvm/diagnostics/ErrorsJvm.java @@ -133,7 +133,7 @@ public interface ErrorsJvm { DiagnosticFactory1 ANNOTATION_TARGETS_NON_EXISTENT_ACCESSOR = DiagnosticFactory1.create(WARNING); - DiagnosticFactory1 SUSPENSION_POINT_INSIDE_MONITOR = DiagnosticFactory1.create(ERROR, DEFAULT); + DiagnosticFactory0 SUSPENSION_POINT_INSIDE_MONITOR = DiagnosticFactory0.create(ERROR); @SuppressWarnings("UnusedDeclaration") Object _initializer = new Object() { diff --git a/compiler/testData/compileKotlinAgainstCustomBinaries/suspensionPointInMonitor/output.txt b/compiler/testData/compileKotlinAgainstCustomBinaries/suspensionPointInMonitor/output.txt index 2d8085c78a2..e0018b4ce6b 100644 --- a/compiler/testData/compileKotlinAgainstCustomBinaries/suspensionPointInMonitor/output.txt +++ b/compiler/testData/compileKotlinAgainstCustomBinaries/suspensionPointInMonitor/output.txt @@ -1,10 +1,13 @@ -compiler/testData/compileKotlinAgainstCustomBinaries/suspensionPointInMonitor/source.kt:10:13: error: the 'suspensionPoint' suspension point is inside a critical section - suspensionPoint() +compiler/testData/compileKotlinAgainstCustomBinaries/suspensionPointInMonitor/source.kt:8:13: error: a suspension point is inside a critical section + builder { ^ -compiler/testData/compileKotlinAgainstCustomBinaries/suspensionPointInMonitor/source.kt:14:13: error: the 'suspensionPoint' suspension point is inside a critical section - suspensionPoint() +compiler/testData/compileKotlinAgainstCustomBinaries/suspensionPointInMonitor/source.kt:14:13: error: a suspension point is inside a critical section + builder { ^ -compiler/testData/compileKotlinAgainstCustomBinaries/suspensionPointInMonitor/source.kt:19:15: error: the 'suspensionPoint' suspension point is inside a critical section - { suspensionPoint() } - ^ +compiler/testData/compileKotlinAgainstCustomBinaries/suspensionPointInMonitor/source.kt:20:13: error: a suspension point is inside a critical section + builder { + ^ +compiler/testData/compileKotlinAgainstCustomBinaries/suspensionPointInMonitor/source.kt:42:9: error: a suspension point is inside a critical section + override suspend fun run() { + ^ COMPILATION_ERROR diff --git a/compiler/testData/compileKotlinAgainstCustomBinaries/suspensionPointInMonitor/source.kt b/compiler/testData/compileKotlinAgainstCustomBinaries/suspensionPointInMonitor/source.kt index 283dc0bdcfe..feae7e6d102 100644 --- a/compiler/testData/compileKotlinAgainstCustomBinaries/suspensionPointInMonitor/source.kt +++ b/compiler/testData/compileKotlinAgainstCustomBinaries/suspensionPointInMonitor/source.kt @@ -9,14 +9,44 @@ fun test() { synchronized(lock) { suspensionPoint() } + } + builder { inlineMe { suspensionPoint() } + } + builder { monitorInFinally( - { suspensionPoint() }, + {}, { suspensionPoint() } ) } + + synchronized(lock) { + builder { + suspensionPoint() + } + } + + synchronized(lock) { + object : SuspendRunnable { + override suspend fun run() { + suspensionPoint() + } + } + } + + object : SuspendRunnable { + override suspend fun run() { + synchronized(lock) { + suspensionPoint() + } + } + } +} + +interface SuspendRunnable { + suspend fun run() }