Skip to content

Commit

Permalink
8318895: Deoptimization results in incorrect lightweight locking stack
Browse files Browse the repository at this point in the history
Backport-of: ea1ffa34192448317ce9a61a3588b0dee3a2ef44
  • Loading branch information
rkennke committed Nov 27, 2023
1 parent cae6460 commit 0f7bbe1
Show file tree
Hide file tree
Showing 2 changed files with 157 additions and 3 deletions.
17 changes: 14 additions & 3 deletions src/hotspot/share/runtime/deoptimization.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@
#include "runtime/stackValue.hpp"
#include "runtime/stackWatermarkSet.hpp"
#include "runtime/stubRoutines.hpp"
#include "runtime/synchronizer.hpp"
#include "runtime/threadSMR.hpp"
#include "runtime/threadWXSetters.inline.hpp"
#include "runtime/vframe.hpp"
Expand Down Expand Up @@ -1648,9 +1649,19 @@ bool Deoptimization::relock_objects(JavaThread* thread, GrowableArray<MonitorInf
}
}
}
BasicLock* lock = mon_info->lock();
ObjectSynchronizer::enter(obj, lock, deoptee_thread);
assert(mon_info->owner()->is_locked(), "object must be locked now");
if (LockingMode == LM_LIGHTWEIGHT && exec_mode == Unpack_none) {
// We have lost information about the correct state of the lock stack.
// Inflate the locks instead. Enter then inflate to avoid races with
// deflation.
ObjectSynchronizer::enter(obj, nullptr, deoptee_thread);
assert(mon_info->owner()->is_locked(), "object must be locked now");
ObjectMonitor* mon = ObjectSynchronizer::inflate(deoptee_thread, obj(), ObjectSynchronizer::inflate_cause_vm_internal);
assert(mon->owner() == deoptee_thread, "must be");
} else {
BasicLock* lock = mon_info->lock();
ObjectSynchronizer::enter(obj, lock, deoptee_thread);
assert(mon_info->owner()->is_locked(), "object must be locked now");
}
}
}
}
Expand Down
143 changes: 143 additions & 0 deletions test/jdk/com/sun/jdi/EATests.java
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@
* -XX:+WhiteBoxAPI
* -Xbatch
* -XX:+DoEscapeAnalysis -XX:+EliminateAllocations -XX:+EliminateLocks -XX:+EliminateNestedLocks
* -XX:LockingMode=1
* @run driver EATests
* -XX:+UnlockDiagnosticVMOptions
* -Xms256m -Xmx256m
Expand All @@ -50,6 +51,7 @@
* -XX:+WhiteBoxAPI
* -Xbatch
* -XX:+DoEscapeAnalysis -XX:+EliminateAllocations -XX:-EliminateLocks -XX:+EliminateNestedLocks
* -XX:LockingMode=1
* @run driver EATests
* -XX:+UnlockDiagnosticVMOptions
* -Xms256m -Xmx256m
Expand All @@ -58,6 +60,7 @@
* -XX:+WhiteBoxAPI
* -Xbatch
* -XX:+DoEscapeAnalysis -XX:-EliminateAllocations -XX:+EliminateLocks -XX:+EliminateNestedLocks
* -XX:LockingMode=1
* @run driver EATests
* -XX:+UnlockDiagnosticVMOptions
* -Xms256m -Xmx256m
Expand All @@ -66,6 +69,44 @@
* -XX:+WhiteBoxAPI
* -Xbatch
* -XX:-DoEscapeAnalysis -XX:-EliminateAllocations -XX:+EliminateLocks -XX:+EliminateNestedLocks
* -XX:LockingMode=1
*
* @run driver EATests
* -XX:+UnlockDiagnosticVMOptions
* -Xms256m -Xmx256m
* -Xbootclasspath/a:.
* -XX:CompileCommand=dontinline,*::dontinline_*
* -XX:+WhiteBoxAPI
* -Xbatch
* -XX:+DoEscapeAnalysis -XX:+EliminateAllocations -XX:+EliminateLocks -XX:+EliminateNestedLocks
* -XX:LockingMode=2
* @run driver EATests
* -XX:+UnlockDiagnosticVMOptions
* -Xms256m -Xmx256m
* -Xbootclasspath/a:.
* -XX:CompileCommand=dontinline,*::dontinline_*
* -XX:+WhiteBoxAPI
* -Xbatch
* -XX:+DoEscapeAnalysis -XX:+EliminateAllocations -XX:-EliminateLocks -XX:+EliminateNestedLocks
* -XX:LockingMode=2
* @run driver EATests
* -XX:+UnlockDiagnosticVMOptions
* -Xms256m -Xmx256m
* -Xbootclasspath/a:.
* -XX:CompileCommand=dontinline,*::dontinline_*
* -XX:+WhiteBoxAPI
* -Xbatch
* -XX:+DoEscapeAnalysis -XX:-EliminateAllocations -XX:+EliminateLocks -XX:+EliminateNestedLocks
* -XX:LockingMode=2
* @run driver EATests
* -XX:+UnlockDiagnosticVMOptions
* -Xms256m -Xmx256m
* -Xbootclasspath/a:.
* -XX:CompileCommand=dontinline,*::dontinline_*
* -XX:+WhiteBoxAPI
* -Xbatch
* -XX:-DoEscapeAnalysis -XX:-EliminateAllocations -XX:+EliminateLocks -XX:+EliminateNestedLocks
* -XX:LockingMode=2
*
* @comment Excercise -XX:+DeoptimizeObjectsALot. Mostly to prevent bit-rot because the option is meant to stress object deoptimization
* with non-synthetic workloads.
Expand Down Expand Up @@ -208,11 +249,13 @@ public static void main(String[] args) {

// Relocking test cases
new EARelockingSimpleTarget() .run();
new EARelockingSimpleWithAccessInOtherThreadTarget() .run();
new EARelockingRecursiveTarget() .run();
new EARelockingNestedInflatedTarget() .run();
new EARelockingNestedInflated_02Target() .run();
new EARelockingArgEscapeLWLockedInCalleeFrameTarget() .run();
new EARelockingArgEscapeLWLockedInCalleeFrame_2Target() .run();
new EARelockingArgEscapeLWLockedInCalleeFrameNoRecursiveTarget() .run();
new EAGetOwnedMonitorsTarget() .run();
new EAEntryCountTarget() .run();
new EARelockingObjectCurrentlyWaitingOnTarget() .run();
Expand Down Expand Up @@ -325,11 +368,13 @@ protected void runTests() throws Exception {

// Relocking test cases
new EARelockingSimple() .run(this);
new EARelockingSimpleWithAccessInOtherThread() .run(this);
new EARelockingRecursive() .run(this);
new EARelockingNestedInflated() .run(this);
new EARelockingNestedInflated_02() .run(this);
new EARelockingArgEscapeLWLockedInCalleeFrame() .run(this);
new EARelockingArgEscapeLWLockedInCalleeFrame_2() .run(this);
new EARelockingArgEscapeLWLockedInCalleeFrameNoRecursive() .run(this);
new EAGetOwnedMonitors() .run(this);
new EAEntryCount() .run(this);
new EARelockingObjectCurrentlyWaitingOn() .run(this);
Expand Down Expand Up @@ -1703,6 +1748,62 @@ public void dontinline_testMethod() {

/////////////////////////////////////////////////////////////////////////////

// The debugger reads and publishes an object with eliminated locking to an instance field.
// A 2nd thread in the debuggee finds it there and changes its state using a synchronized method.
// Without eager relocking the accesses are unsynchronized which can be observed.
class EARelockingSimpleWithAccessInOtherThread extends EATestCaseBaseDebugger {

public void runTestCase() throws Exception {
BreakpointEvent bpe = resumeTo(TARGET_TESTCASE_BASE_NAME, "dontinline_brkpt", "()V");
printStack(bpe.thread());
String l1ClassName = EARelockingSimpleWithAccessInOtherThreadTarget.SyncCounter.class.getName();
ObjectReference ctr = getLocalRef(bpe.thread().frame(1), l1ClassName, "l1");
setField(testCase, "sharedCounter", ctr);
terminateEndlessLoop();
}
}

class EARelockingSimpleWithAccessInOtherThreadTarget extends EATestCaseBaseTarget {

public static class SyncCounter {
private int val;
public synchronized int inc() { return val++; }
}

public volatile SyncCounter sharedCounter;

@Override
public void setUp() {
super.setUp();
doLoop = true;
Thread.ofPlatform().daemon().start(() -> {
while (doLoop) {
SyncCounter ctr = sharedCounter;
if (ctr != null) {
ctr.inc();
}
}
});
}

public void dontinline_testMethod() {
SyncCounter l1 = new SyncCounter();
synchronized (l1) { // Eliminated locking
l1.inc();
dontinline_brkpt(); // Debugger publishes l1 to sharedCounter.
iResult = l1.inc(); // Changes by the 2nd thread will be observed if l1
// was not relocked before passing it to the debugger.
}
}

@Override
public int getExpectedIResult() {
return 1;
}
}

/////////////////////////////////////////////////////////////////////////////

// Test recursive locking
class EARelockingRecursiveTarget extends EATestCaseBaseTarget {

Expand Down Expand Up @@ -1901,6 +2002,48 @@ public int getExpectedIResult() {

/////////////////////////////////////////////////////////////////////////////

/**
* Similar to {@link EARelockingArgEscapeLWLockedInCalleeFrame_2Target}. It does
* not use recursive locking and exposed a bug in the lightweight-locking implementation.
*/
class EARelockingArgEscapeLWLockedInCalleeFrameNoRecursive extends EATestCaseBaseDebugger {

public void runTestCase() throws Exception {
BreakpointEvent bpe = resumeTo(TARGET_TESTCASE_BASE_NAME, "dontinline_brkpt", "()V");
printStack(bpe.thread());
@SuppressWarnings("unused")
ObjectReference o = getLocalRef(bpe.thread().frame(2), XYVAL_NAME, "l1");
}
}

class EARelockingArgEscapeLWLockedInCalleeFrameNoRecursiveTarget extends EATestCaseBaseTarget {

@Override
public void setUp() {
super.setUp();
testMethodDepth = 2;
}

public void dontinline_testMethod() {
XYVal l1 = new XYVal(1, 1); // NoEscape, scalar replaced
XYVal l2 = new XYVal(4, 2); // NoEscape, scalar replaced
XYVal l3 = new XYVal(5, 3); // ArgEscape
synchronized (l1) { // eliminated
synchronized (l2) { // eliminated
l3.dontinline_sync_method(this); // l3 escapes
}
}
iResult = l2.x + l2.y;
}

@Override
public int getExpectedIResult() {
return 6;
}
}

/////////////////////////////////////////////////////////////////////////////

/**
* Test relocking eliminated (nested) locks of an object on which the
* target thread currently waits.
Expand Down

1 comment on commit 0f7bbe1

@openjdk-notifier
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please sign in to comment.