From 90690e07118f0ffbeb6123fdbafb95ac2893d615 Mon Sep 17 00:00:00 2001 From: Andrey Breslav Date: Wed, 2 Jul 2014 21:25:32 +0400 Subject: [PATCH] KT-4825 Implement "synchronized" properly via monitorenter/monitorexit #KT-4825 Fixed --- .../codegen/intrinsics/IntrinsicMethods.java | 3 +- ...tupidSync.java => MonitorInstruction.java} | 36 +++++++++++----- .../fullJdk/synchronized/finally.kt | 25 +++++++++++ .../synchronized/nestedDifferentObjects.kt | 13 ++++++ .../fullJdk/synchronized/nestedSameObject.kt | 11 +++++ .../fullJdk/{ => synchronized}/sync.kt | 0 .../fullJdk/synchronized/wait.kt | 16 +++++++ ...lackBoxWithStdlibCodegenTestGenerated.java | 43 +++++++++++++++++-- .../src/kotlin/jvm/internal/Intrinsics.java | 3 ++ libraries/stdlib/src/kotlin/JLangJVM.kt | 11 ++++- .../src/kotlin/jvm/internal/unsafe/monitor.kt | 25 +++++++++++ 11 files changed, 170 insertions(+), 16 deletions(-) rename compiler/backend/src/org/jetbrains/jet/codegen/intrinsics/{StupidSync.java => MonitorInstruction.java} (71%) create mode 100644 compiler/testData/codegen/boxWithStdlib/fullJdk/synchronized/finally.kt create mode 100644 compiler/testData/codegen/boxWithStdlib/fullJdk/synchronized/nestedDifferentObjects.kt create mode 100644 compiler/testData/codegen/boxWithStdlib/fullJdk/synchronized/nestedSameObject.kt rename compiler/testData/codegen/boxWithStdlib/fullJdk/{ => synchronized}/sync.kt (100%) create mode 100644 compiler/testData/codegen/boxWithStdlib/fullJdk/synchronized/wait.kt create mode 100644 libraries/stdlib/src/kotlin/jvm/internal/unsafe/monitor.kt diff --git a/compiler/backend/src/org/jetbrains/jet/codegen/intrinsics/IntrinsicMethods.java b/compiler/backend/src/org/jetbrains/jet/codegen/intrinsics/IntrinsicMethods.java index 008f80b1f86..0e2df56943c 100644 --- a/compiler/backend/src/org/jetbrains/jet/codegen/intrinsics/IntrinsicMethods.java +++ b/compiler/backend/src/org/jetbrains/jet/codegen/intrinsics/IntrinsicMethods.java @@ -71,7 +71,8 @@ public class IntrinsicMethods { namedMethods.put("kotlin.javaClass.property", new JavaClassProperty()); namedMethods.put("kotlin.arrays.array", new JavaClassArray()); namedMethods.put("kotlin.collections.copyToArray", new CopyToArray()); - namedMethods.put("kotlin.synchronized", new StupidSync()); + namedMethods.put("kotlin.jvm.internal.unsafe.monitorEnter", MonitorInstruction.MONITOR_ENTER); + namedMethods.put("kotlin.jvm.internal.unsafe.monitorExit", MonitorInstruction.MONITOR_EXIT); ImmutableList primitiveCastMethods = OperatorConventions.NUMBER_CONVERSIONS.asList(); for (Name method : primitiveCastMethods) { diff --git a/compiler/backend/src/org/jetbrains/jet/codegen/intrinsics/StupidSync.java b/compiler/backend/src/org/jetbrains/jet/codegen/intrinsics/MonitorInstruction.java similarity index 71% rename from compiler/backend/src/org/jetbrains/jet/codegen/intrinsics/StupidSync.java rename to compiler/backend/src/org/jetbrains/jet/codegen/intrinsics/MonitorInstruction.java index 2abfecee7cd..5d4150152d7 100644 --- a/compiler/backend/src/org/jetbrains/jet/codegen/intrinsics/StupidSync.java +++ b/compiler/backend/src/org/jetbrains/jet/codegen/intrinsics/MonitorInstruction.java @@ -1,5 +1,5 @@ /* - * Copyright 2010-2013 JetBrains s.r.o. + * Copyright 2010-2014 JetBrains s.r.o. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -19,25 +19,35 @@ package org.jetbrains.jet.codegen.intrinsics; import com.intellij.psi.PsiElement; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; -import org.jetbrains.org.objectweb.asm.Type; -import org.jetbrains.org.objectweb.asm.commons.InstructionAdapter; import org.jetbrains.jet.codegen.ExpressionCodegen; import org.jetbrains.jet.codegen.StackValue; import org.jetbrains.jet.lang.psi.JetCallExpression; import org.jetbrains.jet.lang.psi.JetExpression; import org.jetbrains.jet.lang.resolve.BindingContext; import org.jetbrains.jet.lang.resolve.calls.model.ResolvedCall; +import org.jetbrains.org.objectweb.asm.Opcodes; +import org.jetbrains.org.objectweb.asm.Type; +import org.jetbrains.org.objectweb.asm.commons.InstructionAdapter; import java.util.Arrays; import java.util.List; -import static org.jetbrains.jet.lang.resolve.java.AsmTypeConstants.FUNCTION0_TYPE; import static org.jetbrains.jet.lang.resolve.java.AsmTypeConstants.OBJECT_TYPE; -public class StupidSync extends IntrinsicMethod { +public class MonitorInstruction extends IntrinsicMethod { + + public static final MonitorInstruction MONITOR_ENTER = new MonitorInstruction(Opcodes.MONITORENTER); + public static final MonitorInstruction MONITOR_EXIT = new MonitorInstruction(Opcodes.MONITOREXIT); + + private final int opcode; + + private MonitorInstruction(int opcode) { + this.opcode = opcode; + } + @NotNull @Override - public Type generateImpl( + protected Type generateImpl( @NotNull ExpressionCodegen codegen, @NotNull InstructionAdapter v, @NotNull Type returnType, @@ -51,8 +61,14 @@ public class StupidSync extends IntrinsicMethod { assert resolvedCall != null : "Resolved call for " + element.getText() + " should be not null"; - codegen.pushMethodArgumentsWithoutCallReceiver(resolvedCall, Arrays.asList(OBJECT_TYPE, FUNCTION0_TYPE), false, codegen.defaultCallGenerator); - v.invokestatic("kotlin/jvm/internal/Intrinsics", "stupidSync", Type.getMethodDescriptor(OBJECT_TYPE, OBJECT_TYPE, FUNCTION0_TYPE)); - return OBJECT_TYPE; + codegen.pushMethodArgumentsWithoutCallReceiver( + resolvedCall, + Arrays.asList(OBJECT_TYPE), + false, + codegen.defaultCallGenerator + ); + + v.visitInsn(opcode); + return Type.VOID_TYPE; } -} +} \ No newline at end of file diff --git a/compiler/testData/codegen/boxWithStdlib/fullJdk/synchronized/finally.kt b/compiler/testData/codegen/boxWithStdlib/fullJdk/synchronized/finally.kt new file mode 100644 index 00000000000..1c676b9936f --- /dev/null +++ b/compiler/testData/codegen/boxWithStdlib/fullJdk/synchronized/finally.kt @@ -0,0 +1,25 @@ +fun box(): String { + val obj = "" as java.lang.Object + + try { + synchronized (obj) { + throw Throwable() + } + } + catch (e: Throwable) { + // If monitorexit didn't happen (a finally block failed), this assertion would fail + assertThatThreadDoesNotOwnMonitor(obj) + } + + return "OK" +} + +fun assertThatThreadDoesNotOwnMonitor(obj: java.lang.Object) { + try { + obj.wait(1) + throw IllegalStateException("Not owning a monitor!") + } + catch (e: IllegalMonitorStateException) { + // OK + } +} \ No newline at end of file diff --git a/compiler/testData/codegen/boxWithStdlib/fullJdk/synchronized/nestedDifferentObjects.kt b/compiler/testData/codegen/boxWithStdlib/fullJdk/synchronized/nestedDifferentObjects.kt new file mode 100644 index 00000000000..fac1470096d --- /dev/null +++ b/compiler/testData/codegen/boxWithStdlib/fullJdk/synchronized/nestedDifferentObjects.kt @@ -0,0 +1,13 @@ +fun box(): String { + val obj = "" as java.lang.Object + val obj2 = "1" as java.lang.Object + + synchronized (obj) { + synchronized (obj2) { + obj.wait(1) + obj2.wait(1) + } + } + + return "OK" +} \ No newline at end of file diff --git a/compiler/testData/codegen/boxWithStdlib/fullJdk/synchronized/nestedSameObject.kt b/compiler/testData/codegen/boxWithStdlib/fullJdk/synchronized/nestedSameObject.kt new file mode 100644 index 00000000000..e66d981f95d --- /dev/null +++ b/compiler/testData/codegen/boxWithStdlib/fullJdk/synchronized/nestedSameObject.kt @@ -0,0 +1,11 @@ +fun box(): String { + val obj = "" as java.lang.Object + + synchronized (obj) { + synchronized (obj) { + obj.wait(1) + } + } + + return "OK" +} \ No newline at end of file diff --git a/compiler/testData/codegen/boxWithStdlib/fullJdk/sync.kt b/compiler/testData/codegen/boxWithStdlib/fullJdk/synchronized/sync.kt similarity index 100% rename from compiler/testData/codegen/boxWithStdlib/fullJdk/sync.kt rename to compiler/testData/codegen/boxWithStdlib/fullJdk/synchronized/sync.kt diff --git a/compiler/testData/codegen/boxWithStdlib/fullJdk/synchronized/wait.kt b/compiler/testData/codegen/boxWithStdlib/fullJdk/synchronized/wait.kt new file mode 100644 index 00000000000..c82a3259fe0 --- /dev/null +++ b/compiler/testData/codegen/boxWithStdlib/fullJdk/synchronized/wait.kt @@ -0,0 +1,16 @@ +fun box(): String { + val obj = "" as java.lang.Object + try { + obj.wait(1) + return "Fail: exception should have been thrown" + } + catch (e: IllegalMonitorStateException) { + // OK + } + + synchronized (obj) { + obj.wait(1) + } + + return "OK" +} \ No newline at end of file diff --git a/compiler/tests/org/jetbrains/jet/codegen/generated/BlackBoxWithStdlibCodegenTestGenerated.java b/compiler/tests/org/jetbrains/jet/codegen/generated/BlackBoxWithStdlibCodegenTestGenerated.java index 411dd47a771..983a5dc7bab 100644 --- a/compiler/tests/org/jetbrains/jet/codegen/generated/BlackBoxWithStdlibCodegenTestGenerated.java +++ b/compiler/tests/org/jetbrains/jet/codegen/generated/BlackBoxWithStdlibCodegenTestGenerated.java @@ -902,6 +902,7 @@ public class BlackBoxWithStdlibCodegenTestGenerated extends AbstractBlackBoxCode } @TestMetadata("compiler/testData/codegen/boxWithStdlib/fullJdk") + @InnerTestClasses({FullJdk.Synchronized.class}) public static class FullJdk extends AbstractBlackBoxCodegenTest { public void testAllFilesPresentInFullJdk() throws Exception { JetTestUtils.assertAllTestsPresentByMetadata(this.getClass(), "org.jetbrains.jet.generators.tests.TestsPackage", new File("compiler/testData/codegen/boxWithStdlib/fullJdk"), Pattern.compile("^(.+)\\.kt$"), true); @@ -947,11 +948,45 @@ public class BlackBoxWithStdlibCodegenTestGenerated extends AbstractBlackBoxCode doTestWithStdlib("compiler/testData/codegen/boxWithStdlib/fullJdk/kt434.kt"); } - @TestMetadata("sync.kt") - public void testSync() throws Exception { - doTestWithStdlib("compiler/testData/codegen/boxWithStdlib/fullJdk/sync.kt"); + @TestMetadata("compiler/testData/codegen/boxWithStdlib/fullJdk/synchronized") + public static class Synchronized extends AbstractBlackBoxCodegenTest { + public void testAllFilesPresentInSynchronized() throws Exception { + JetTestUtils.assertAllTestsPresentByMetadata(this.getClass(), "org.jetbrains.jet.generators.tests.TestsPackage", new File("compiler/testData/codegen/boxWithStdlib/fullJdk/synchronized"), Pattern.compile("^(.+)\\.kt$"), true); + } + + @TestMetadata("finally.kt") + public void testFinally() throws Exception { + doTestWithStdlib("compiler/testData/codegen/boxWithStdlib/fullJdk/synchronized/finally.kt"); + } + + @TestMetadata("nestedDifferentObjects.kt") + public void testNestedDifferentObjects() throws Exception { + doTestWithStdlib("compiler/testData/codegen/boxWithStdlib/fullJdk/synchronized/nestedDifferentObjects.kt"); + } + + @TestMetadata("nestedSameObject.kt") + public void testNestedSameObject() throws Exception { + doTestWithStdlib("compiler/testData/codegen/boxWithStdlib/fullJdk/synchronized/nestedSameObject.kt"); + } + + @TestMetadata("sync.kt") + public void testSync() throws Exception { + doTestWithStdlib("compiler/testData/codegen/boxWithStdlib/fullJdk/synchronized/sync.kt"); + } + + @TestMetadata("wait.kt") + public void testWait() throws Exception { + doTestWithStdlib("compiler/testData/codegen/boxWithStdlib/fullJdk/synchronized/wait.kt"); + } + } + public static Test innerSuite() { + TestSuite suite = new TestSuite("FullJdk"); + suite.addTestSuite(FullJdk.class); + suite.addTestSuite(Synchronized.class); + return suite; + } } @TestMetadata("compiler/testData/codegen/boxWithStdlib/hashPMap") @@ -1838,7 +1873,7 @@ public class BlackBoxWithStdlibCodegenTestGenerated extends AbstractBlackBoxCode suite.addTestSuite(Casts.class); suite.addTest(DataClasses.innerSuite()); suite.addTestSuite(Evaluate.class); - suite.addTestSuite(FullJdk.class); + suite.addTest(FullJdk.innerSuite()); suite.addTestSuite(HashPMap.class); suite.addTestSuite(JdkAnnotations.class); suite.addTestSuite(PlatformNames.class); diff --git a/core/runtime.jvm/src/kotlin/jvm/internal/Intrinsics.java b/core/runtime.jvm/src/kotlin/jvm/internal/Intrinsics.java index efaee67176e..05c7f80a0af 100644 --- a/core/runtime.jvm/src/kotlin/jvm/internal/Intrinsics.java +++ b/core/runtime.jvm/src/kotlin/jvm/internal/Intrinsics.java @@ -20,6 +20,7 @@ import kotlin.Function0; import kotlin.IntRange; import kotlin.KotlinNullPointerException; +import java.lang.Deprecated; import java.util.ArrayList; import java.util.Arrays; import java.util.HashSet; @@ -87,6 +88,8 @@ public class Intrinsics { return new IntRange(0, length - 1); } + // TODO: remove this function when ABI version is advanced + @Deprecated // A better implementation of synchronized is used now public static R stupidSync(Object lock, Function0 block) { synchronized (lock) { return block.invoke(); diff --git a/libraries/stdlib/src/kotlin/JLangJVM.kt b/libraries/stdlib/src/kotlin/JLangJVM.kt index 64a12e62433..8b0db79545d 100644 --- a/libraries/stdlib/src/kotlin/JLangJVM.kt +++ b/libraries/stdlib/src/kotlin/JLangJVM.kt @@ -2,6 +2,7 @@ package kotlin import java.lang.annotation.Retention import java.lang.annotation.RetentionPolicy +import kotlin.jvm.internal.unsafe.* import kotlin.jvm.internal.Intrinsic /** @@ -24,7 +25,15 @@ public annotation class throws(vararg val exceptionClasses: Class [Intrinsic("kotlin.javaClass.function")] fun javaClass() : Class = null as Class -[Intrinsic("kotlin.synchronized")] public fun synchronized(lock: Any, block: () -> R): R = block() +public inline fun synchronized(lock: Any, block: () -> R): R { + monitorEnter(lock) + try { + return block() + } + finally { + monitorExit(lock) + } +} public fun T.annotationType() : Class = (this as java.lang.annotation.Annotation).annotationType() as Class diff --git a/libraries/stdlib/src/kotlin/jvm/internal/unsafe/monitor.kt b/libraries/stdlib/src/kotlin/jvm/internal/unsafe/monitor.kt new file mode 100644 index 00000000000..5b9223c1964 --- /dev/null +++ b/libraries/stdlib/src/kotlin/jvm/internal/unsafe/monitor.kt @@ -0,0 +1,25 @@ +/* + * Copyright 2010-2014 JetBrains s.r.o. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package kotlin.jvm.internal.unsafe + +import kotlin.jvm.internal.Intrinsic + +[Intrinsic("kotlin.jvm.internal.unsafe.monitorEnter")] +public fun monitorEnter(monitor: Any): Unit = throw UnsupportedOperationException("This function can only be used privately") + +[Intrinsic("kotlin.jvm.internal.unsafe.monitorExit")] +public fun monitorExit(monitor: Any): Unit = throw UnsupportedOperationException("This function can only be used privately")