Skip to content

Commit 3b98b92

Browse files
tkrodrigueztkrodriguez
committed
[GR-50428] Fix underflow in MethodHandlePlugin
PullRequest: graal/16824
2 parents 29f2d1f + 9b3e867 commit 3b98b92

File tree

5 files changed

+92
-17
lines changed

5 files changed

+92
-17
lines changed

‎compiler/src/jdk.graal.compiler.test/src/jdk/graal/compiler/replacements/test/MethodHandleImplTest.java

+55
Original file line numberDiff line numberDiff line change
@@ -27,9 +27,20 @@
2727
import java.lang.invoke.MethodHandle;
2828
import java.lang.invoke.MethodHandles;
2929
import java.lang.invoke.MethodType;
30+
import java.lang.invoke.VarHandle;
3031

3132
import org.junit.Test;
3233

34+
import jdk.graal.compiler.nodes.DeoptimizeNode;
35+
import jdk.graal.compiler.nodes.ValueNode;
36+
import jdk.graal.compiler.nodes.graphbuilderconf.GraphBuilderConfiguration;
37+
import jdk.graal.compiler.nodes.graphbuilderconf.GraphBuilderContext;
38+
import jdk.graal.compiler.nodes.graphbuilderconf.NodePlugin;
39+
import jdk.vm.ci.meta.DeoptimizationAction;
40+
import jdk.vm.ci.meta.DeoptimizationReason;
41+
import jdk.vm.ci.meta.ResolvedJavaField;
42+
import jdk.vm.ci.meta.ResolvedJavaMethod;
43+
3344
public class MethodHandleImplTest extends MethodSubstitutionTest {
3445

3546
static final MethodHandle squareHandle;
@@ -63,4 +74,48 @@ public void testIsCompileConstant() {
6374
testGraph("invokeSquare");
6475
}
6576

77+
@Override
78+
protected GraphBuilderConfiguration editGraphBuilderConfiguration(GraphBuilderConfiguration conf) {
79+
conf.getPlugins().prependNodePlugin(new NodePlugin() {
80+
@Override
81+
public boolean handleLoadField(GraphBuilderContext b, ValueNode object, ResolvedJavaField field) {
82+
if (field.getName().equals("fieldOffset") && b.getGraph().method().getName().equals("incrementThreadCount")) {
83+
// Force a deopt in the testFlock test case
84+
b.add(new DeoptimizeNode(DeoptimizationAction.InvalidateRecompile, DeoptimizationReason.Unresolved));
85+
return true;
86+
}
87+
return false;
88+
}
89+
});
90+
return super.editGraphBuilderConfiguration(conf);
91+
}
92+
93+
public static class ThreadFlock {
94+
private static final VarHandle THREAD_COUNT;
95+
96+
static {
97+
try {
98+
MethodHandles.Lookup l = MethodHandles.lookup();
99+
THREAD_COUNT = l.findVarHandle(ThreadFlock.class, "threadCount", int.class);
100+
} catch (Exception e) {
101+
throw new InternalError(e);
102+
}
103+
}
104+
105+
@SuppressWarnings("unused") private volatile int threadCount;
106+
107+
public void incrementThreadCount() {
108+
THREAD_COUNT.getAndAdd(this, 1);
109+
}
110+
}
111+
112+
@Test
113+
public void testFlock() {
114+
ThreadFlock flock = new ThreadFlock();
115+
for (int i = 0; i < 10_000; i++) {
116+
flock.incrementThreadCount();
117+
}
118+
ResolvedJavaMethod method = getResolvedJavaMethod(ThreadFlock.class, "incrementThreadCount");
119+
getCode(method);
120+
}
66121
}

‎compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/java/BytecodeParser.java

+8-1
Original file line numberDiff line numberDiff line change
@@ -1889,7 +1889,9 @@ protected Invokable appendInvoke(InvokeKind initialInvokeKind, ResolvedJavaMetho
18891889
if (!parsingIntrinsic() && DeoptALot.getValue(options)) {
18901890
append(new DeoptimizeNode(DeoptimizationAction.None, RuntimeConstraint));
18911891
JavaKind resultType = initialTargetMethod.getSignature().getReturnKind();
1892-
frameState.pushReturn(resultType, ConstantNode.defaultForKind(resultType, graph));
1892+
if (resultType != JavaKind.Void) {
1893+
frameState.pushReturn(resultType, ConstantNode.defaultForKind(resultType, graph));
1894+
}
18931895
return null;
18941896
}
18951897

@@ -2951,6 +2953,11 @@ private <T extends Node> void updateLastInstruction(T v) {
29512953
}
29522954
}
29532955

2956+
@Override
2957+
public boolean hasParseTerminated() {
2958+
return lastInstr == null;
2959+
}
2960+
29542961
private AbstractBeginNode updateWithExceptionNode(WithExceptionNode withExceptionNode) {
29552962
if (withExceptionNode.exceptionEdge() == null) {
29562963
AbstractBeginNode exceptionEdge = handleException(null, bci(), false);

‎compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/nodes/graphbuilderconf/GraphBuilderContext.java

+7
Original file line numberDiff line numberDiff line change
@@ -301,6 +301,13 @@ default boolean parsingIntrinsic() {
301301
return getIntrinsic() != null;
302302
}
303303

304+
/**
305+
* Returns true if control flow has terminated in some fashion, such as a deoptimization.
306+
*/
307+
default boolean hasParseTerminated() {
308+
throw GraalError.unimplementedParent(); // ExcludeFromJacocoGeneratedReport
309+
}
310+
304311
/**
305312
* Determines if a graph builder plugin is enabled under current context.
306313
*/

‎compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/replacements/IntrinsicGraphBuilder.java

+5
Original file line numberDiff line numberDiff line change
@@ -195,6 +195,11 @@ private <T extends Node> void updateLastInstruction(T v) {
195195
}
196196
}
197197

198+
@Override
199+
public boolean hasParseTerminated() {
200+
return lastInstr == null;
201+
}
202+
198203
@Override
199204
public AbstractBeginNode genExplicitExceptionEdge(BytecodeExceptionNode.BytecodeExceptionKind exceptionKind, ValueNode... exceptionArguments) {
200205
BytecodeExceptionNode exceptionNode = graph.add(new BytecodeExceptionNode(getMetaAccess(), exceptionKind, exceptionArguments));

‎compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/replacements/MethodHandlePlugin.java

+17-16
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,6 @@
4242
import jdk.graal.compiler.replacements.nodes.MacroNode;
4343
import jdk.graal.compiler.replacements.nodes.MethodHandleNode;
4444
import jdk.graal.compiler.replacements.nodes.ResolvedMethodHandleCallTargetNode;
45-
4645
import jdk.vm.ci.meta.JavaKind;
4746
import jdk.vm.ci.meta.MethodHandleAccessProvider;
4847
import jdk.vm.ci.meta.MethodHandleAccessProvider.IntrinsicMethod;
@@ -129,21 +128,23 @@ public <T extends ValueNode> T add(T node) {
129128
}
130129
}
131130

132-
/*
133-
* After handleReplacedInvoke, a return type according to the signature of
134-
* targetMethod has been pushed. That can be different than the type expected by the
135-
* method handle invoke. Since there cannot be any implicit type conversion, the
136-
* only safe option actually is that the return type is not used at all. If there is
137-
* any other expected return type, the bytecodes are wrong. The JavaDoc of
138-
* MethodHandle.invokeBasic states that this "could crash the JVM", so bailing out
139-
* of compilation seems like a good idea.
140-
*/
141-
JavaKind invokeReturnKind = invokeReturnStamp.getTrustedStamp().getStackKind();
142-
JavaKind targetMethodReturnKind = targetMethod.getSignature().getReturnKind().getStackKind();
143-
if (invokeReturnKind != targetMethodReturnKind) {
144-
b.pop(targetMethodReturnKind);
145-
if (invokeReturnKind != JavaKind.Void) {
146-
throw b.bailout("Cannot do any type conversion when invoking method handle, so return value must remain popped");
131+
if (!b.hasParseTerminated()) {
132+
/*
133+
* After handleReplacedInvoke, a return type according to the signature of
134+
* targetMethod has been pushed. That can be different than the type expected by
135+
* the method handle invoke. Since there cannot be any implicit type conversion,
136+
* the only safe option actually is that the return type is not used at all. If
137+
* there is any other expected return type, the bytecodes are wrong. The JavaDoc
138+
* of MethodHandle.invokeBasic states that this "could crash the JVM", so
139+
* bailing out of compilation seems like a good idea.
140+
*/
141+
JavaKind invokeReturnKind = invokeReturnStamp.getTrustedStamp().getStackKind();
142+
JavaKind targetMethodReturnKind = targetMethod.getSignature().getReturnKind().getStackKind();
143+
if (invokeReturnKind != targetMethodReturnKind) {
144+
b.pop(targetMethodReturnKind);
145+
if (invokeReturnKind != JavaKind.Void) {
146+
throw b.bailout("Cannot do any type conversion when invoking method handle, so return value must remain popped");
147+
}
147148
}
148149
}
149150
}

0 commit comments

Comments
 (0)