From 2dd5b8fa7043dd25089ebf23080e932bbbf122e1 Mon Sep 17 00:00:00 2001 From: Ilmir Usmanov Date: Tue, 11 Sep 2018 21:29:36 +0300 Subject: [PATCH] Forbid suspension points in critical sections #KT-26480: Fixed --- .../kotlin/codegen/ExpressionCodegen.java | 5 ++ .../coroutines/GlobalCoroutinesContext.kt | 55 ++++++++++++++++ .../kotlin/codegen/inline/InlineCodegen.kt | 65 ++++++++++++++++++- .../kotlin/codegen/inline/MethodInliner.kt | 2 +- .../kotlin/codegen/state/GenerationState.kt | 2 + .../jetbrains/kotlin/diagnostics/Errors.java | 1 + .../rendering/DefaultErrorMessages.java | 1 + .../suspensionPointInMonitor/library/a.kt | 17 +++++ .../suspensionPointInMonitor/output.txt | 10 +++ .../suspensionPointInMonitor/source.kt | 22 +++++++ .../CompileKotlinAgainstCustomBinariesTest.kt | 9 +++ 11 files changed, 186 insertions(+), 3 deletions(-) create mode 100644 compiler/backend/src/org/jetbrains/kotlin/codegen/coroutines/GlobalCoroutinesContext.kt create mode 100644 compiler/testData/compileKotlinAgainstCustomBinaries/suspensionPointInMonitor/library/a.kt create mode 100644 compiler/testData/compileKotlinAgainstCustomBinaries/suspensionPointInMonitor/output.txt create mode 100644 compiler/testData/compileKotlinAgainstCustomBinaries/suspensionPointInMonitor/source.kt diff --git a/compiler/backend/src/org/jetbrains/kotlin/codegen/ExpressionCodegen.java b/compiler/backend/src/org/jetbrains/kotlin/codegen/ExpressionCodegen.java index ba306d62a47..cd8b1ea4c39 100644 --- a/compiler/backend/src/org/jetbrains/kotlin/codegen/ExpressionCodegen.java +++ b/compiler/backend/src/org/jetbrains/kotlin/codegen/ExpressionCodegen.java @@ -2432,6 +2432,11 @@ public class ExpressionCodegen extends KtVisitor impleme return; } + if (resolvedCall.getResultingDescriptor() instanceof FunctionDescriptor && + ((FunctionDescriptor) resolvedCall.getResultingDescriptor()).isSuspend()) { + state.getGlobalCoroutinesContext().checkSuspendCall(resolvedCall); + } + boolean isSuspendNoInlineCall = CoroutineCodegenUtilKt.isSuspendNoInlineCall(resolvedCall, this, state.getLanguageVersionSettings()); boolean isConstructor = resolvedCall.getResultingDescriptor() instanceof ConstructorDescriptor; diff --git a/compiler/backend/src/org/jetbrains/kotlin/codegen/coroutines/GlobalCoroutinesContext.kt b/compiler/backend/src/org/jetbrains/kotlin/codegen/coroutines/GlobalCoroutinesContext.kt new file mode 100644 index 00000000000..7064bfc2b39 --- /dev/null +++ b/compiler/backend/src/org/jetbrains/kotlin/codegen/coroutines/GlobalCoroutinesContext.kt @@ -0,0 +1,55 @@ +/* + * Copyright 2010-2018 JetBrains s.r.o. 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.codegen.coroutines + +import org.jetbrains.kotlin.backend.common.peek +import org.jetbrains.kotlin.backend.common.pop +import org.jetbrains.kotlin.diagnostics.DiagnosticSink +import org.jetbrains.kotlin.diagnostics.Errors.SUSPENSION_POINT_INSIDE_MONITOR +import org.jetbrains.kotlin.resolve.calls.model.ResolvedCall + +class GlobalCoroutinesContext(private val diagnostics: DiagnosticSink) { + private var monitorsDepth = 0 + + private val inlineLambdaInsideMonitorSourceArgumentIndexes = arrayListOf>() + + fun pushArgumentIndexes(indexes: Set) { + inlineLambdaInsideMonitorSourceArgumentIndexes.add(indexes) + } + + fun popArgumentIndexes() { + inlineLambdaInsideMonitorSourceArgumentIndexes.pop() + } + + private fun enterMonitor() { + monitorsDepth++ + } + + fun enterMonitorIfNeeded(index: Int?) { + if (index == null) return + if (inlineLambdaInsideMonitorSourceArgumentIndexes.peek()?.contains(index) != true) return + enterMonitor() + } + + private fun exitMonitor() { + assert(monitorsDepth > 0) { + "exitMonitor without corresponding enterMonitor" + } + monitorsDepth-- + } + + fun exitMonitorIfNeeded(index: Int?) { + if (index == null) return + if (inlineLambdaInsideMonitorSourceArgumentIndexes.peek()?.contains(index) != true) return + exitMonitor() + } + + fun checkSuspendCall(call: ResolvedCall<*>) { + if (monitorsDepth != 0) { + diagnostics.report(SUSPENSION_POINT_INSIDE_MONITOR.on(call.call.callElement, call.resultingDescriptor)) + } + } +} \ No newline at end of file diff --git a/compiler/backend/src/org/jetbrains/kotlin/codegen/inline/InlineCodegen.kt b/compiler/backend/src/org/jetbrains/kotlin/codegen/inline/InlineCodegen.kt index 232d86c5b25..684a16b9c53 100644 --- a/compiler/backend/src/org/jetbrains/kotlin/codegen/inline/InlineCodegen.kt +++ b/compiler/backend/src/org/jetbrains/kotlin/codegen/inline/InlineCodegen.kt @@ -8,6 +8,7 @@ package org.jetbrains.kotlin.codegen.inline import com.intellij.psi.PsiElement import com.intellij.util.ArrayUtil import org.jetbrains.kotlin.backend.common.isBuiltInIntercepted +import org.jetbrains.kotlin.backend.common.isTopLevelInPackage import org.jetbrains.kotlin.builtins.BuiltInsPackageFragment import org.jetbrains.kotlin.codegen.* import org.jetbrains.kotlin.codegen.AsmUtil.getMethodAsmFlags @@ -19,6 +20,8 @@ import org.jetbrains.kotlin.codegen.coroutines.createMethodNodeForSuspendCorouti import org.jetbrains.kotlin.codegen.coroutines.isBuiltInSuspendCoroutineUninterceptedOrReturnInJvm import org.jetbrains.kotlin.codegen.intrinsics.bytecode import org.jetbrains.kotlin.codegen.intrinsics.classId +import org.jetbrains.kotlin.codegen.optimization.common.ControlFlowGraph +import org.jetbrains.kotlin.codegen.optimization.common.asSequence import org.jetbrains.kotlin.codegen.state.GenerationState import org.jetbrains.kotlin.codegen.state.KotlinTypeMapper import org.jetbrains.kotlin.config.isReleaseCoroutines @@ -244,7 +247,18 @@ abstract class InlineCodegen( } } val reificationResult = reifiedTypeInliner.reifyInstructions(node) - generateClosuresBodies() + + val hasMonitor = node.instructions.asSequence().any { it.opcode == Opcodes.MONITORENTER } + if (hasMonitor) { + state.globalCoroutinesContext.pushArgumentIndexes(findInlineLambdasInsideMonitor(node)) + } + try { + generateClosuresBodies() + } finally { + if (hasMonitor) { + state.globalCoroutinesContext.popArgumentIndexes() + } + } //through generation captured parameters will be added to invocationParamBuilder putClosureParametersOnStack() @@ -301,6 +315,45 @@ abstract class InlineCodegen( return result } + private fun findInlineLambdasInsideMonitor(node: MethodNode): Set { + val sources = MethodInliner.analyzeMethodNodeBeforeInline(node) + + val cfg = ControlFlowGraph.build(node) + val monitorDepthMap = hashMapOf() + val result = hashSetOf() + + fun addMonitorDepthToSuccs(index: Int, depth: Int) { + val insn = node.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[node.instructions[succIndex]] == null) { + addMonitorDepthToSuccs(succIndex, newDepth) + } + } + } + + addMonitorDepthToSuccs(0, 0) + + for (insn in node.instructions.asSequence()) { + if (insn !is MethodInsnNode) continue + if (!isInvokeOnLambda(insn.owner, insn.name)) continue + if (monitorDepthMap[insn]?.let { it > 0 } != true) continue + val frame = sources[node.instructions.indexOf(insn)] ?: continue + for (source in frame.getStack(frame.stackSize - Type.getArgumentTypes(insn.desc).size - 1).insns) { + if (source.opcode == Opcodes.ALOAD) { + result.add((source as VarInsnNode).`var`) + } + } + } + + return result + } + private fun isInlinedToInlineFunInKotlinRuntime(): Boolean { val codegen = this.codegen as? ExpressionCodegen ?: return false val caller = codegen.context.functionDescriptor @@ -310,8 +363,16 @@ abstract class InlineCodegen( } private fun generateClosuresBodies() { + val parameters = invocationParamBuilder.buildParameters() + for (info in expressionMap.values) { - info.generateLambdaBody(sourceCompiler, reifiedTypeInliner) + val index = parameters.find { it.lambda == info }?.index + state.globalCoroutinesContext.enterMonitorIfNeeded(index) + try { + info.generateLambdaBody(sourceCompiler, reifiedTypeInliner) + } finally { + state.globalCoroutinesContext.exitMonitorIfNeeded(index) + } } } diff --git a/compiler/backend/src/org/jetbrains/kotlin/codegen/inline/MethodInliner.kt b/compiler/backend/src/org/jetbrains/kotlin/codegen/inline/MethodInliner.kt index 374db2345fc..dbe2fa63ed8 100644 --- a/compiler/backend/src/org/jetbrains/kotlin/codegen/inline/MethodInliner.kt +++ b/compiler/backend/src/org/jetbrains/kotlin/codegen/inline/MethodInliner.kt @@ -945,7 +945,7 @@ class MethodInliner( ) } - private fun analyzeMethodNodeBeforeInline(node: MethodNode): Array?> { + fun analyzeMethodNodeBeforeInline(node: MethodNode): Array?> { val analyzer = object : Analyzer(SourceInterpreter()) { override fun newFrame(nLocals: Int, nStack: Int): Frame { diff --git a/compiler/backend/src/org/jetbrains/kotlin/codegen/state/GenerationState.kt b/compiler/backend/src/org/jetbrains/kotlin/codegen/state/GenerationState.kt index afef75291cb..c637da15de5 100644 --- a/compiler/backend/src/org/jetbrains/kotlin/codegen/state/GenerationState.kt +++ b/compiler/backend/src/org/jetbrains/kotlin/codegen/state/GenerationState.kt @@ -13,6 +13,7 @@ import org.jetbrains.kotlin.codegen.`when`.MappingsClassesForWhenByEnum import org.jetbrains.kotlin.codegen.binding.CodegenBinding import org.jetbrains.kotlin.codegen.context.CodegenContext import org.jetbrains.kotlin.codegen.context.RootContext +import org.jetbrains.kotlin.codegen.coroutines.GlobalCoroutinesContext import org.jetbrains.kotlin.codegen.extensions.ClassBuilderInterceptorExtension import org.jetbrains.kotlin.codegen.inline.GlobalInlineContext import org.jetbrains.kotlin.codegen.inline.InlineCache @@ -202,6 +203,7 @@ class GenerationState private constructor( } val samWrapperClasses: SamWrapperClasses = SamWrapperClasses(this) val globalInlineContext: GlobalInlineContext = GlobalInlineContext(diagnostics) + val globalCoroutinesContext: GlobalCoroutinesContext = GlobalCoroutinesContext(diagnostics) val mappingsClassesForWhenByEnum: MappingsClassesForWhenByEnum = MappingsClassesForWhenByEnum(this) val jvmRuntimeTypes: JvmRuntimeTypes = JvmRuntimeTypes(module, configuration.languageVersionSettings) val factory: ClassFileFactory diff --git a/compiler/frontend/src/org/jetbrains/kotlin/diagnostics/Errors.java b/compiler/frontend/src/org/jetbrains/kotlin/diagnostics/Errors.java index 4bc61728159..5cc1c289f36 100644 --- a/compiler/frontend/src/org/jetbrains/kotlin/diagnostics/Errors.java +++ b/compiler/frontend/src/org/jetbrains/kotlin/diagnostics/Errors.java @@ -1037,6 +1037,7 @@ public interface Errors { DiagnosticFactory0 OVERRIDE_BY_INLINE = DiagnosticFactory0.create(WARNING, DECLARATION_SIGNATURE); DiagnosticFactory0 REIFIED_TYPE_PARAMETER_IN_OVERRIDE = DiagnosticFactory0.create(ERROR); DiagnosticFactory1 INLINE_CALL_CYCLE = DiagnosticFactory1.create(ERROR, DEFAULT); + DiagnosticFactory1 SUSPENSION_POINT_INSIDE_MONITOR = DiagnosticFactory1.create(ERROR, DEFAULT); DiagnosticFactory0 NON_LOCAL_RETURN_IN_DISABLED_INLINE = DiagnosticFactory0.create(ERROR); DiagnosticFactory0 INLINE_PROPERTY_WITH_BACKING_FIELD = DiagnosticFactory0.create(ERROR, DECLARATION_SIGNATURE); DiagnosticFactory0 NON_INTERNAL_PUBLISHED_API = DiagnosticFactory0.create(ERROR); diff --git a/compiler/frontend/src/org/jetbrains/kotlin/diagnostics/rendering/DefaultErrorMessages.java b/compiler/frontend/src/org/jetbrains/kotlin/diagnostics/rendering/DefaultErrorMessages.java index 83e4d2eae01..f5a2f8fea01 100644 --- a/compiler/frontend/src/org/jetbrains/kotlin/diagnostics/rendering/DefaultErrorMessages.java +++ b/compiler/frontend/src/org/jetbrains/kotlin/diagnostics/rendering/DefaultErrorMessages.java @@ -934,6 +934,7 @@ public class DefaultErrorMessages { //Inline non locals MAP.put(NON_LOCAL_RETURN_NOT_ALLOWED, "Can''t inline ''{0}'' here: it may contain non-local returns. Add ''crossinline'' modifier to parameter declaration ''{0}''", ELEMENT_TEXT); MAP.put(INLINE_CALL_CYCLE, "The ''{0}'' invocation is a part of inline cycle", NAME); + MAP.put(SUSPENSION_POINT_INSIDE_MONITOR, "The ''{0}'' suspension point is inside a critical section", NAME); MAP.put(NON_LOCAL_RETURN_IN_DISABLED_INLINE, "Non-local returns are not allowed with inlining disabled"); MAP.put(NON_LOCAL_SUSPENSION_POINT, "Suspension functions can be called only within coroutine body"); MAP.put(ILLEGAL_SUSPEND_FUNCTION_CALL, "Suspend function ''{0}'' should be called only from a coroutine or another suspend function", NAME); diff --git a/compiler/testData/compileKotlinAgainstCustomBinaries/suspensionPointInMonitor/library/a.kt b/compiler/testData/compileKotlinAgainstCustomBinaries/suspensionPointInMonitor/library/a.kt new file mode 100644 index 00000000000..fd7910b47b0 --- /dev/null +++ b/compiler/testData/compileKotlinAgainstCustomBinaries/suspensionPointInMonitor/library/a.kt @@ -0,0 +1,17 @@ +val lock = Any() + +inline fun inlineMe(c: () -> Unit) { + synchronized(lock) { + c() + } +} + +inline fun monitorInFinally(a: () -> Unit, b: () -> Unit) { + try { + a() + } finally { + synchronized(lock) { + b() + } + } +} diff --git a/compiler/testData/compileKotlinAgainstCustomBinaries/suspensionPointInMonitor/output.txt b/compiler/testData/compileKotlinAgainstCustomBinaries/suspensionPointInMonitor/output.txt new file mode 100644 index 00000000000..2d8085c78a2 --- /dev/null +++ b/compiler/testData/compileKotlinAgainstCustomBinaries/suspensionPointInMonitor/output.txt @@ -0,0 +1,10 @@ +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:14:13: error: the 'suspensionPoint' suspension point is inside a critical section + suspensionPoint() + ^ +compiler/testData/compileKotlinAgainstCustomBinaries/suspensionPointInMonitor/source.kt:19:15: error: the 'suspensionPoint' suspension point is inside a critical section + { suspensionPoint() } + ^ +COMPILATION_ERROR diff --git a/compiler/testData/compileKotlinAgainstCustomBinaries/suspensionPointInMonitor/source.kt b/compiler/testData/compileKotlinAgainstCustomBinaries/suspensionPointInMonitor/source.kt new file mode 100644 index 00000000000..283dc0bdcfe --- /dev/null +++ b/compiler/testData/compileKotlinAgainstCustomBinaries/suspensionPointInMonitor/source.kt @@ -0,0 +1,22 @@ +fun builder(c: suspend () -> Unit) {} + +private val lock = Any() + +suspend fun suspensionPoint() {} + +fun test() { + builder { + synchronized(lock) { + suspensionPoint() + } + + inlineMe { + suspensionPoint() + } + + monitorInFinally( + { suspensionPoint() }, + { suspensionPoint() } + ) + } +} diff --git a/compiler/tests/org/jetbrains/kotlin/jvm/compiler/CompileKotlinAgainstCustomBinariesTest.kt b/compiler/tests/org/jetbrains/kotlin/jvm/compiler/CompileKotlinAgainstCustomBinariesTest.kt index 72bbf88bd76..5c73c379d52 100644 --- a/compiler/tests/org/jetbrains/kotlin/jvm/compiler/CompileKotlinAgainstCustomBinariesTest.kt +++ b/compiler/tests/org/jetbrains/kotlin/jvm/compiler/CompileKotlinAgainstCustomBinariesTest.kt @@ -140,6 +140,15 @@ class CompileKotlinAgainstCustomBinariesTest : AbstractKotlinCompilerIntegration compileKotlin("main.kt", tmpdir, listOf(compileLibrary("library"))) } + fun testSuspensionPointInMonitor() { + compileKotlin( + "source.kt", + tmpdir, + listOf(compileLibrary("library", additionalOptions = listOf("-Xskip-metadata-version-check"))), + additionalOptions = listOf("-Xskip-metadata-version-check") + ) + } + fun testDuplicateObjectInBinaryAndSources() { val allDescriptors = analyzeAndGetAllDescriptors(compileLibrary("library")) assertEquals(allDescriptors.toString(), 2, allDescriptors.size)