[GR-50428] Fix underflow in MethodHandlePlugin

PullRequest: graal/16824
oracle · Feb 6, 2024 · 3b98b92 · 3b98b92
2 parents 29f2d1f + 9b3e867
commit 3b98b92
Show file tree

Hide file tree

Showing 5 changed files with 92 additions and 17 deletions.
diff --git a/...dk.graal.compiler.test/src/jdk/graal/compiler/replacements/test/MethodHandleImplTest.java b/...dk.graal.compiler.test/src/jdk/graal/compiler/replacements/test/MethodHandleImplTest.java
@@ -27,9 +27,20 @@
 import java.lang.invoke.MethodHandle;
 import java.lang.invoke.MethodHandles;
 import java.lang.invoke.MethodType;
+import java.lang.invoke.VarHandle;
 
 import org.junit.Test;
 
+import jdk.graal.compiler.nodes.DeoptimizeNode;
+import jdk.graal.compiler.nodes.ValueNode;
+import jdk.graal.compiler.nodes.graphbuilderconf.GraphBuilderConfiguration;
+import jdk.graal.compiler.nodes.graphbuilderconf.GraphBuilderContext;
+import jdk.graal.compiler.nodes.graphbuilderconf.NodePlugin;
+import jdk.vm.ci.meta.DeoptimizationAction;
+import jdk.vm.ci.meta.DeoptimizationReason;
+import jdk.vm.ci.meta.ResolvedJavaField;
+import jdk.vm.ci.meta.ResolvedJavaMethod;
+
 public class MethodHandleImplTest extends MethodSubstitutionTest {
 
     static final MethodHandle squareHandle;
@@ -63,4 +74,48 @@ public void testIsCompileConstant() {
         testGraph("invokeSquare");
     }
 
+    @Override
+    protected GraphBuilderConfiguration editGraphBuilderConfiguration(GraphBuilderConfiguration conf) {
+        conf.getPlugins().prependNodePlugin(new NodePlugin() {
+            @Override
+            public boolean handleLoadField(GraphBuilderContext b, ValueNode object, ResolvedJavaField field) {
+                if (field.getName().equals("fieldOffset") && b.getGraph().method().getName().equals("incrementThreadCount")) {
+                    // Force a deopt in the testFlock test case
+                    b.add(new DeoptimizeNode(DeoptimizationAction.InvalidateRecompile, DeoptimizationReason.Unresolved));
+                    return true;
+                }
+                return false;
+            }
+        });
+        return super.editGraphBuilderConfiguration(conf);
+    }
+
+    public static class ThreadFlock {
+        private static final VarHandle THREAD_COUNT;
+
+        static {
+            try {
+                MethodHandles.Lookup l = MethodHandles.lookup();
+                THREAD_COUNT = l.findVarHandle(ThreadFlock.class, "threadCount", int.class);
+            } catch (Exception e) {
+                throw new InternalError(e);
+            }
+        }
+
+        @SuppressWarnings("unused") private volatile int threadCount;
+
+        public void incrementThreadCount() {
+            THREAD_COUNT.getAndAdd(this, 1);
+        }
+    }
+
+    @Test
+    public void testFlock() {
+        ThreadFlock flock = new ThreadFlock();
+        for (int i = 0; i < 10_000; i++) {
+            flock.incrementThreadCount();
+        }
+        ResolvedJavaMethod method = getResolvedJavaMethod(ThreadFlock.class, "incrementThreadCount");
+        getCode(method);
+    }
 }
diff --git a/compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/java/BytecodeParser.java b/compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/java/BytecodeParser.java
@@ -1889,7 +1889,9 @@ protected Invokable appendInvoke(InvokeKind initialInvokeKind, ResolvedJavaMetho
         if (!parsingIntrinsic() && DeoptALot.getValue(options)) {
             append(new DeoptimizeNode(DeoptimizationAction.None, RuntimeConstraint));
             JavaKind resultType = initialTargetMethod.getSignature().getReturnKind();
-            frameState.pushReturn(resultType, ConstantNode.defaultForKind(resultType, graph));
+            if (resultType != JavaKind.Void) {
+                frameState.pushReturn(resultType, ConstantNode.defaultForKind(resultType, graph));
+            }
             return null;
         }
 
@@ -2951,6 +2953,11 @@ private <T extends Node> void updateLastInstruction(T v) {
         }
     }
 
+    @Override
+    public boolean hasParseTerminated() {
+        return lastInstr == null;
+    }
+
     private AbstractBeginNode updateWithExceptionNode(WithExceptionNode withExceptionNode) {
         if (withExceptionNode.exceptionEdge() == null) {
             AbstractBeginNode exceptionEdge = handleException(null, bci(), false);

diff --git a/...jdk.graal.compiler/src/jdk/graal/compiler/nodes/graphbuilderconf/GraphBuilderContext.java b/...jdk.graal.compiler/src/jdk/graal/compiler/nodes/graphbuilderconf/GraphBuilderContext.java
@@ -301,6 +301,13 @@ default boolean parsingIntrinsic() {
         return getIntrinsic() != null;
     }
 
+    /**
+     * Returns true if control flow has terminated in some fashion, such as a deoptimization.
+     */
+    default boolean hasParseTerminated() {
+        throw GraalError.unimplementedParent(); // ExcludeFromJacocoGeneratedReport
+    }
+
     /**
      * Determines if a graph builder plugin is enabled under current context.
      */

diff --git a/...ler/src/jdk.graal.compiler/src/jdk/graal/compiler/replacements/IntrinsicGraphBuilder.java b/...ler/src/jdk.graal.compiler/src/jdk/graal/compiler/replacements/IntrinsicGraphBuilder.java
@@ -195,6 +195,11 @@ private <T extends Node> void updateLastInstruction(T v) {
         }
     }
 
+    @Override
+    public boolean hasParseTerminated() {
+        return lastInstr == null;
+    }
+
     @Override
     public AbstractBeginNode genExplicitExceptionEdge(BytecodeExceptionNode.BytecodeExceptionKind exceptionKind, ValueNode... exceptionArguments) {
         BytecodeExceptionNode exceptionNode = graph.add(new BytecodeExceptionNode(getMetaAccess(), exceptionKind, exceptionArguments));

diff --git a/compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/replacements/MethodHandlePlugin.java b/compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/replacements/MethodHandlePlugin.java
@@ -42,7 +42,6 @@
 import jdk.graal.compiler.replacements.nodes.MacroNode;
 import jdk.graal.compiler.replacements.nodes.MethodHandleNode;
 import jdk.graal.compiler.replacements.nodes.ResolvedMethodHandleCallTargetNode;
-
 import jdk.vm.ci.meta.JavaKind;
 import jdk.vm.ci.meta.MethodHandleAccessProvider;
 import jdk.vm.ci.meta.MethodHandleAccessProvider.IntrinsicMethod;
@@ -129,21 +128,23 @@ public <T extends ValueNode> T add(T node) {
                     }
                 }
 
-                /*
-                 * After handleReplacedInvoke, a return type according to the signature of
-                 * targetMethod has been pushed. That can be different than the type expected by the
-                 * method handle invoke. Since there cannot be any implicit type conversion, the
-                 * only safe option actually is that the return type is not used at all. If there is
-                 * any other expected return type, the bytecodes are wrong. The JavaDoc of
-                 * MethodHandle.invokeBasic states that this "could crash the JVM", so bailing out
-                 * of compilation seems like a good idea.
-                 */
-                JavaKind invokeReturnKind = invokeReturnStamp.getTrustedStamp().getStackKind();
-                JavaKind targetMethodReturnKind = targetMethod.getSignature().getReturnKind().getStackKind();
-                if (invokeReturnKind != targetMethodReturnKind) {
-                    b.pop(targetMethodReturnKind);
-                    if (invokeReturnKind != JavaKind.Void) {
-                        throw b.bailout("Cannot do any type conversion when invoking method handle, so return value must remain popped");
+                if (!b.hasParseTerminated()) {
+                    /*
+                     * After handleReplacedInvoke, a return type according to the signature of
+                     * targetMethod has been pushed. That can be different than the type expected by
+                     * the method handle invoke. Since there cannot be any implicit type conversion,
+                     * the only safe option actually is that the return type is not used at all. If
+                     * there is any other expected return type, the bytecodes are wrong. The JavaDoc
+                     * of MethodHandle.invokeBasic states that this "could crash the JVM", so
+                     * bailing out of compilation seems like a good idea.
+                     */
+                    JavaKind invokeReturnKind = invokeReturnStamp.getTrustedStamp().getStackKind();
+                    JavaKind targetMethodReturnKind = targetMethod.getSignature().getReturnKind().getStackKind();
+                    if (invokeReturnKind != targetMethodReturnKind) {
+                        b.pop(targetMethodReturnKind);
+                        if (invokeReturnKind != JavaKind.Void) {
+                            throw b.bailout("Cannot do any type conversion when invoking method handle, so return value must remain popped");
+                        }
                     }
                 }
             }