8234401: ConstantCallSite may stuck in non-frozen state

Reviewed-by: psandoz

src/java.base/share/classes/java/lang/invoke/CallSite.java

@@ -87,9 +87,10 @@ private static CallSite bootstrapDynamic(MethodHandles.Lookup caller, String nam
 abstract
 public class CallSite {
 
-    // The actual payload of this call site:
+    // The actual payload of this call site.
+    // Can be modified using {@link MethodHandleNatives#setCallSiteTargetNormal} or {@link MethodHandleNatives#setCallSiteTargetVolatile}.
     /*package-private*/
-    MethodHandle target;    // Note: This field is known to the JVM.  Do not change.
+    final MethodHandle target;  // Note: This field is known to the JVM.
 
     /**
      * Make a blank call site object with the given method type.
@@ -129,11 +130,11 @@ public class CallSite {
      */
     /*package-private*/
     CallSite(MethodType targetType, MethodHandle createTargetHook) throws Throwable {
-        this(targetType);
+        this(targetType); // need to initialize target to make CallSite.type() work in createTargetHook
         ConstantCallSite selfCCS = (ConstantCallSite) this;
         MethodHandle boundTarget = (MethodHandle) createTargetHook.invokeWithArguments(selfCCS);
-        checkTargetChange(this.target, boundTarget);
+        setTargetNormal(boundTarget); // ConstantCallSite doesn't publish CallSite.target
-        this.target = boundTarget;
+        UNSAFE.storeStoreFence(); // barrier between target and isFrozen updates
     }
 
     /**
@@ -190,12 +191,13 @@ public class CallSite {
      */
     public abstract void setTarget(MethodHandle newTarget);
 
-    void checkTargetChange(MethodHandle oldTarget, MethodHandle newTarget) {
+    private void checkTargetChange(MethodHandle newTarget) {
-        MethodType oldType = oldTarget.type();
+        MethodType oldType = target.type(); // target is always present
         MethodType newType = newTarget.type();  // null check!
-        if (!newType.equals(oldType))
+        if (newType != oldType) {
             throw wrongTargetType(newTarget, oldType);
         }
+    }
 
     private static WrongMethodTypeException wrongTargetType(MethodHandle target, MethodType type) {
         return new WrongMethodTypeException(String.valueOf(target)+" should be of type "+type);
@@ -217,7 +219,7 @@ public class CallSite {
      */
     public abstract MethodHandle dynamicInvoker();
 
-    /*non-public*/
+    /*package-private*/
     MethodHandle makeDynamicInvoker() {
         MethodHandle getTarget = getTargetHandle().bindArgumentL(0, this);
         MethodHandle invoker = MethodHandles.exactInvoker(this.type());
@@ -283,19 +285,24 @@ public class CallSite {
     }
 
     /*package-private*/
-    void setTargetNormal(MethodHandle newTarget) {
+    final void setTargetNormal(MethodHandle newTarget) {
+        checkTargetChange(newTarget);
         MethodHandleNatives.setCallSiteTargetNormal(this, newTarget);
     }
+
     /*package-private*/
-    MethodHandle getTargetVolatile() {
+    final MethodHandle getTargetVolatile() {
         return (MethodHandle) UNSAFE.getReferenceVolatile(this, getTargetOffset());
     }
+
     /*package-private*/
-    void setTargetVolatile(MethodHandle newTarget) {
+    final void setTargetVolatile(MethodHandle newTarget) {
+        checkTargetChange(newTarget);
         MethodHandleNatives.setCallSiteTargetVolatile(this, newTarget);
     }
 
     // this implements the upcall from the JVM, MethodHandleNatives.linkCallSite:
+    /*package-private*/
     static CallSite makeSite(MethodHandle bootstrapMethod,
                              // Callee information:
                              String name, MethodType type,

src/java.base/share/classes/java/lang/invoke/ConstantCallSite.java

@@ -25,6 +25,9 @@
 
 package java.lang.invoke;
 
+import jdk.internal.misc.Unsafe;
+import jdk.internal.vm.annotation.Stable;
+
 /**
  * A {@code ConstantCallSite} is a {@link CallSite} whose target is permanent, and can never be changed.
  * An {@code invokedynamic} instruction linked to a {@code ConstantCallSite} is permanently
@@ -33,7 +36,10 @@ package java.lang.invoke;
  * @since 1.7
  */
 public class ConstantCallSite extends CallSite {
-    private final boolean isFrozen;
+    private static final Unsafe UNSAFE = Unsafe.getUnsafe();
+
+    @Stable // should NOT be constant folded during instance initialization (isFrozen == false)
+    /*final*/ private boolean isFrozen;
 
     /**
      * Creates a call site with a permanent target.
@@ -43,6 +49,7 @@ public class ConstantCallSite extends CallSite {
     public ConstantCallSite(MethodHandle target) {
         super(target);
         isFrozen = true;
+        UNSAFE.storeStoreFence(); // properly publish isFrozen update
     }
 
     /**
@@ -79,8 +86,9 @@ public class ConstantCallSite extends CallSite {
      * @throws Throwable anything else thrown by the hook function
      */
     protected ConstantCallSite(MethodType targetType, MethodHandle createTargetHook) throws Throwable {
-        super(targetType, createTargetHook);
+        super(targetType, createTargetHook); // "this" instance leaks into createTargetHook
         isFrozen = true;
+        UNSAFE.storeStoreFence(); // properly publish isFrozen
     }
 
     /**

src/java.base/share/classes/java/lang/invoke/MutableCallSite.java

@@ -152,7 +152,6 @@ public class MutableCallSite extends CallSite {
      * @see #getTarget
      */
     @Override public void setTarget(MethodHandle newTarget) {
-        checkTargetChange(this.target, newTarget);
         setTargetNormal(newTarget);
     }
 

src/java.base/share/classes/java/lang/invoke/VolatileCallSite.java

@@ -96,7 +96,6 @@ public class VolatileCallSite extends CallSite {
      * @see #getTarget
      */
     @Override public void setTarget(MethodHandle newTarget) {
-        checkTargetChange(getTargetVolatile(), newTarget);
         setTargetVolatile(newTarget);
     }
 

test/jdk/java/lang/invoke/CallSiteTest.java

@@ -24,10 +24,11 @@
 /**
  * @test
  * @summary smoke tests for CallSite
+ * @library /test/lib
  *
  * @build indify.Indify
  * @compile CallSiteTest.java
- * @run main/othervm/timeout=3600 -XX:+IgnoreUnrecognizedVMOptions -XX:-VerifyDependencies
+ * @run main/othervm/timeout=3600 -XX:+IgnoreUnrecognizedVMOptions -XX:-VerifyDependencies -Xbatch
  *      indify.Indify
  *      --expand-properties --classpath ${test.classes}
  *      --java test.java.lang.invoke.CallSiteTest
@@ -40,6 +41,7 @@ import java.io.*;
 import java.lang.invoke.*;
 import static java.lang.invoke.MethodHandles.*;
 import static java.lang.invoke.MethodType.*;
+import static jdk.test.lib.Asserts.*;
 
 public class CallSiteTest {
     private static final Class<?> CLASS = CallSiteTest.class;
@@ -51,16 +53,19 @@ public class CallSiteTest {
 
     static {
         try {
+
             mh_foo = lookup().findStatic(CLASS, "foo", methodType(int.class, int.class, int.class));
             mh_bar = lookup().findStatic(CLASS, "bar", methodType(int.class, int.class, int.class));
             mcs = new MutableCallSite(mh_foo);
             vcs = new VolatileCallSite(mh_foo);
         } catch (Exception e) {
             e.printStackTrace();
+            throw new Error(e);
         }
     }
 
     public static void main(String... av) throws Throwable {
+        testConstantCallSite();
         testMutableCallSite();
         testVolatileCallSite();
     }
@@ -69,9 +74,61 @@ public class CallSiteTest {
     private static final int RESULT1 = 762786192;
     private static final int RESULT2 = -21474836;
 
-    private static void assertEquals(int expected, int actual) {
+    static final CallSite     MCS         = new MutableCallSite(methodType(void.class));
-        if (expected != actual)
+    static final MethodHandle MCS_INVOKER = MCS.dynamicInvoker();
-            throw new AssertionError("expected: " + expected + ", actual: " + actual);
+
+    static void test(boolean shouldThrow) {
+        try {
+            MCS_INVOKER.invokeExact();
+            if (shouldThrow) {
+                throw new AssertionError("should throw");
+            }
+        } catch (IllegalStateException ise) {
+            if (!shouldThrow) {
+                throw new AssertionError("should not throw", ise);
+            }
+        } catch (Throwable e) {
+            throw new Error(e);
+        }
+    }
+
+    static class MyCCS extends ConstantCallSite {
+        public MyCCS(MethodType targetType, MethodHandle createTargetHook) throws Throwable {
+            super(targetType, createTargetHook);
+        }
+    }
+
+    private static MethodHandle testConstantCallSiteHandler(CallSite cs, CallSite[] holder) throws Throwable {
+        holder[0] = cs; // capture call site instance for subsequent checks
+
+        MethodType csType = cs.type(); // should not throw on partially constructed instance
+
+        // Truly dynamic invoker for constant call site
+        MethodHandle getTarget = lookup().findVirtual(CallSite.class, "getTarget", MethodType.methodType(MethodHandle.class))
+                                         .bindTo(cs);
+        MethodHandle invoker = MethodHandles.exactInvoker(csType);
+        MethodHandle target = MethodHandles.foldArguments(invoker, getTarget);
+
+        MCS.setTarget(target);
+        // warmup
+        for (int i = 0; i < 20_000; i++) {
+            test(true); // should throw IllegalStateException
+        }
+
+        return MethodHandles.empty(csType); // initialize cs with an empty method handle
+    }
+
+    private static void testConstantCallSite() throws Throwable {
+        CallSite[] holder = new CallSite[1];
+        MethodHandle handler = lookup().findStatic(CLASS, "testConstantCallSiteHandler", MethodType.methodType(MethodHandle.class, CallSite.class, CallSite[].class));
+        handler = MethodHandles.insertArguments(handler, 1, new Object[] { holder } );
+
+        CallSite ccs = new MyCCS(MCS.type(), handler); // trigger call to handler
+
+        if (ccs != holder[0]) {
+            throw new AssertionError("different call site instances");
+        }
+        test(false); // should not throw
     }
 
     private static void testMutableCallSite() throws Throwable {
@@ -83,11 +140,11 @@ public class CallSiteTest {
         for (int n = 0; n < 2; n++) {
             mcs.setTarget(mh_foo);
             for (int i = 0; i < 5; i++) {
-                assertEquals(RESULT1, runMutableCallSite());
+                assertEQ(RESULT1, runMutableCallSite());
             }
             mcs.setTarget(mh_bar);
             for (int i = 0; i < 5; i++) {
-                assertEquals(RESULT2, runMutableCallSite());
+                assertEQ(RESULT2, runMutableCallSite());
             }
         }
     }
@@ -100,11 +157,11 @@ public class CallSiteTest {
         for (int n = 0; n < 2; n++) {
             vcs.setTarget(mh_foo);
             for (int i = 0; i < 5; i++) {
-                assertEquals(RESULT1, runVolatileCallSite());
+                assertEQ(RESULT1, runVolatileCallSite());
             }
             vcs.setTarget(mh_bar);
             for (int i = 0; i < 5; i++) {
-                assertEquals(RESULT2, runVolatileCallSite());
+                assertEQ(RESULT2, runVolatileCallSite());
             }
         }
     }