Skip to content

Commit 8983aa7

Browse files
mur47x111mur47x111
committed
[GR-64446] Materializing virtual input with lock now causes materialization of virtual objects with lower-depth locks.
PullRequest: graal/20790
2 parents a788cfe + 4a01817 commit 8983aa7

File tree

5 files changed

+77
-23
lines changed

5 files changed

+77
-23
lines changed

‎compiler/src/jdk.graal.compiler.test/src/jdk/graal/compiler/hotspot/test/MonitorPEATest.java

Lines changed: 37 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -130,13 +130,12 @@ public void testSnippet3() {
130130
test("snippet3", new Object(), true);
131131
}
132132

133-
static class A {
134-
Object o = new Object();
133+
record A(Object o) {
135134
}
136135

137136
@SuppressWarnings("unused")
138137
public static void snippet4(Object external, boolean flag, boolean flag1) {
139-
A escaped = new A();
138+
A escaped = new A(new Object());
140139

141140
synchronized (escaped) {
142141
synchronized (external) {
@@ -465,4 +464,39 @@ public static void snippet20(Object o) {
465464
public void testSnippet20() {
466465
test("snippet20", new Object());
467466
}
467+
468+
public static void snippet21() {
469+
Object l1 = new Object();
470+
Object l2 = new Object();
471+
synchronized (l1) {
472+
synchronized (l2) {
473+
staticObj = new Object[]{l2};
474+
synchronized (A.class) {
475+
}
476+
}
477+
}
478+
}
479+
480+
@Test
481+
public void testSnippet21() {
482+
test("snippet21");
483+
}
484+
485+
public static void snippet22() {
486+
Object l2 = new Object();
487+
Object l1 = new A(l2);
488+
489+
synchronized (l1) {
490+
synchronized (l2) {
491+
staticObj = new Object[]{l2};
492+
synchronized (A.class) {
493+
}
494+
}
495+
}
496+
}
497+
498+
@Test
499+
public void testSnippet22() {
500+
test("snippet22");
501+
}
468502
}

‎compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/hotspot/replacements/MonitorSnippets.java

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -110,7 +110,6 @@
110110
import jdk.graal.compiler.core.common.type.ObjectStamp;
111111
import jdk.graal.compiler.core.common.type.StampFactory;
112112
import jdk.graal.compiler.core.common.type.StampPair;
113-
import jdk.graal.compiler.debug.Assertions;
114113
import jdk.graal.compiler.debug.GraalError;
115114
import jdk.graal.compiler.graph.Node.ConstantNodeParameter;
116115
import jdk.graal.compiler.graph.Node.NodeIntrinsic;
@@ -887,7 +886,7 @@ private static boolean isVirtualLock(FrameState frameState, int lockIdx) {
887886
}
888887

889888
private boolean verifyLockOrder(MonitorEnterNode monitorenterNode) {
890-
if (Assertions.assertionsEnabled() && requiresStrictLockOrder) {
889+
if (requiresStrictLockOrder) {
891890
FrameState state = monitorenterNode.stateAfter();
892891
boolean subsequentLocksMustBeEliminated = false;
893892
for (int lockIdx = 0; lockIdx < state.locksSize(); lockIdx++) {

‎compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/virtual/phases/ea/ObjectState.java

Lines changed: 19 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@
3131
import jdk.graal.compiler.debug.Assertions;
3232
import jdk.graal.compiler.debug.CounterKey;
3333
import jdk.graal.compiler.debug.DebugContext;
34+
import jdk.graal.compiler.debug.GraalError;
3435
import jdk.graal.compiler.nodes.ValueNode;
3536
import jdk.graal.compiler.nodes.java.MonitorIdNode;
3637
import jdk.graal.compiler.nodes.virtual.EscapeObjectState;
@@ -207,6 +208,8 @@ public void updateMaterializedValue(ValueNode value) {
207208
}
208209

209210
public void addLock(MonitorIdNode monitorId) {
211+
GraalError.guarantee(locks == null || locks.monitorId.getLockDepth() < monitorId.getLockDepth(),
212+
"Adding lock %d to locks %s", monitorId.getLockDepth(), locks);
210213
locks = new LockState(monitorId, locks);
211214
}
212215

@@ -226,10 +229,25 @@ public boolean hasLocks() {
226229
return locks != null;
227230
}
228231

229-
public int getLockDepth() {
232+
public int getMaximumLockDepth() {
233+
// Assume locks are ordered by their nesting depth in descending order (highest first).
230234
return locks.monitorId.getLockDepth();
231235
}
232236

237+
public int getMinimumLockDepth() {
238+
// Assume locks are ordered by their nesting depth in descending order (highest first).
239+
LockState current = locks;
240+
int currentLockDepth = current.monitorId.getLockDepth();
241+
while (current.next != null) {
242+
int nextLockDepth = current.next.monitorId.getLockDepth();
243+
GraalError.guarantee(currentLockDepth > nextLockDepth,
244+
"Current: %s; Next: %s", current, current.next);
245+
current = current.next;
246+
currentLockDepth = nextLockDepth;
247+
}
248+
return current.monitorId.getLockDepth();
249+
}
250+
233251
public boolean locksEqual(ObjectState other) {
234252
LockState a = locks;
235253
LockState b = other.locks;

‎compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/virtual/phases/ea/PartialEscapeBlockState.java

Lines changed: 17 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -206,14 +206,14 @@ public void updateMaterializedValue(int object, ValueNode value) {
206206
* entries.
207207
*/
208208
@SuppressWarnings("try")
209-
public void materializeBefore(FixedNode fixed, VirtualObjectNode virtual, GraphEffectList materializeEffects) {
209+
public void materializeBefore(FixedNode fixed, VirtualObjectNode virtual, boolean requiresStrictLockOrder, ArrayList<VirtualObjectNode> virtualObjects, GraphEffectList materializeEffects) {
210210
PartialEscapeClosure.COUNTER_MATERIALIZATIONS.increment(fixed.getDebug());
211211
List<AllocatedObjectNode> objects = new ArrayList<>(2);
212212
List<ValueNode> values = new ArrayList<>(8);
213213
List<List<MonitorIdNode>> locks = new ArrayList<>();
214214
List<ValueNode> otherAllocations = new ArrayList<>(2);
215215
List<Boolean> ensureVirtual = new ArrayList<>(2);
216-
materializeWithCommit(fixed, virtual, objects, locks, values, ensureVirtual, otherAllocations, materializeEffects);
216+
materializeWithCommit(fixed, virtual, objects, locks, values, ensureVirtual, otherAllocations, requiresStrictLockOrder, virtualObjects, materializeEffects);
217217
/*
218218
* because all currently virtualized allocations will be materialized in 1 commit alloc node
219219
* with barriers, we ignore other allocations as we only process new instance and commit
@@ -290,7 +290,7 @@ void format(StringBuilder str) {
290290
}
291291

292292
private void materializeWithCommit(FixedNode fixed, VirtualObjectNode virtual, List<AllocatedObjectNode> objects, List<List<MonitorIdNode>> locks, List<ValueNode> values,
293-
List<Boolean> ensureVirtual, List<ValueNode> otherAllocations, GraphEffectList materializeEffects) {
293+
List<Boolean> ensureVirtual, List<ValueNode> otherAllocations, boolean requiresStrictLockOrder, ArrayList<VirtualObjectNode> virtualObjects, GraphEffectList materializeEffects) {
294294
ObjectState obj = getObjectState(virtual);
295295

296296
ValueNode[] entries = obj.getEntries();
@@ -311,14 +311,27 @@ private void materializeWithCommit(FixedNode fixed, VirtualObjectNode virtual, L
311311
VirtualObjectNode entryVirtual = (VirtualObjectNode) entries[i];
312312
ObjectState entryObj = getObjectState(entryVirtual);
313313
if (entryObj.isVirtual()) {
314-
materializeWithCommit(fixed, entryVirtual, objects, locks, values, ensureVirtual, otherAllocations, materializeEffects);
314+
materializeWithCommit(fixed, entryVirtual, objects, locks, values, ensureVirtual, otherAllocations, requiresStrictLockOrder, virtualObjects, materializeEffects);
315315
entryObj = getObjectState(entryVirtual);
316316
}
317317
values.set(pos + i, entryObj.getMaterializedValue());
318318
} else {
319319
values.set(pos + i, entries[i]);
320320
}
321321
}
322+
323+
if (requiresStrictLockOrder && obj.hasLocks()) {
324+
int lockDepth = obj.getMaximumLockDepth();
325+
for (VirtualObjectNode other : virtualObjects) {
326+
if (other != virtual && hasObjectState(other.getObjectId())) {
327+
ObjectState otherState = getObjectState(other);
328+
if (otherState.isVirtual() && otherState.hasLocks() && otherState.getMinimumLockDepth() < lockDepth) {
329+
materializeWithCommit(fixed, other, objects, locks, values, ensureVirtual, otherAllocations, requiresStrictLockOrder, virtualObjects, materializeEffects);
330+
}
331+
}
332+
}
333+
}
334+
322335
objectMaterialized(virtual, (AllocatedObjectNode) representation, values.subList(pos, pos + entries.length));
323336
} else {
324337
VirtualUtil.trace(options, debug, "materialized %s as %s", virtual, representation);

‎compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/virtual/phases/ea/PartialEscapeClosure.java

Lines changed: 3 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -225,7 +225,7 @@ protected void processStateBeforeLoopOnOverflow(BlockT initialState, FixedNode m
225225
for (int i = 0; i < initialState.getStateCount(); i++) {
226226
if (initialState.hasObjectState(i) && initialState.getObjectState(i).isVirtual()) {
227227
VirtualObjectNode virtual = virtualObjects.get(i);
228-
initialState.materializeBefore(materializeBefore, virtual, effects);
228+
initialState.materializeBefore(materializeBefore, virtual, requiresStrictLockOrder, virtualObjects, effects);
229229
}
230230
}
231231
}
@@ -559,17 +559,7 @@ protected boolean ensureMaterialized(PartialEscapeBlockState<?> state, int objec
559559
VirtualObjectNode virtual = virtualObjects.get(object);
560560

561561
GraalError.guarantee(objectState.isVirtual(), "%s is not virtual", objectState);
562-
if (requiresStrictLockOrder && objectState.hasLocks()) {
563-
materializeVirtualLocksBefore(state, materializeBefore, effects, counter, objectState.getLockDepth());
564-
}
565-
/*
566-
* At this point, the current object may have been materialized due to materializing
567-
* lower depth locks that have a data dependency to the current object.
568-
*/
569-
objectState = state.getObjectState(object);
570-
if (objectState.isVirtual()) {
571-
state.materializeBefore(materializeBefore, virtual, effects);
572-
}
562+
state.materializeBefore(materializeBefore, virtual, requiresStrictLockOrder, virtualObjects, effects);
573563

574564
assert !updateStatesForMaterialized(state, virtual, state.getObjectState(object).getMaterializedValue()) : "method must already have been called before";
575565
return true;
@@ -648,7 +638,7 @@ private void materializeVirtualLocksBefore(PartialEscapeBlockState<?> state, Fix
648638
int otherID = other.getObjectId();
649639
if (state.hasObjectState(otherID)) {
650640
ObjectState otherState = state.getObjectState(other);
651-
if (otherState.isVirtual() && otherState.hasLocks() && otherState.getLockDepth() < lockDepth) {
641+
if (otherState.isVirtual() && otherState.hasLocks() && otherState.getMinimumLockDepth() < lockDepth) {
652642
ensureMaterialized(state, other.getObjectId(), materializeBefore, effects, counter);
653643
}
654644
}

0 commit comments

Comments
 (0)