KT-4825 Implement "synchronized" properly via monitorenter/monitorexit

#KT-4825 Fixed

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<Name> primitiveCastMethods = OperatorConventions.NUMBER_CONVERSIONS.asList();
         for (Name method : primitiveCastMethods) {

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;
     }
-}
+}

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
+    }
+}

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"
+}

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"
+}

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"
+}

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);

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> R stupidSync(Object lock, Function0<R> block) {
         synchronized (lock) {
             return block.invoke();

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<out Throwable>
 
 [Intrinsic("kotlin.javaClass.function")] fun <reified T> javaClass() : Class<T> = null as Class<T>
 
-[Intrinsic("kotlin.synchronized")] public fun <R> synchronized(lock: Any, block: () -> R): R = block()
+public inline fun <R> synchronized(lock: Any, block: () -> R): R {
+    monitorEnter(lock)
+    try {
+        return block()
+    }
+    finally {
+        monitorExit(lock)
+    }
+}
 
 public fun <T : Annotation> T.annotationType() : Class<out T> =
     (this as java.lang.annotation.Annotation).annotationType() as Class<out T>

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")