Skip to content

Commit

Permalink
8318895: Deoptimization results in incorrect lightweight locking stack
Browse files Browse the repository at this point in the history
Co-authored-by: Axel Boldt-Christmas <aboldtch@openjdk.org>
Co-authored-by: Richard Reingruber <rrich@openjdk.org>
Reviewed-by: dlong, rrich
  • Loading branch information
3 people committed Nov 10, 2023
1 parent c9657ca commit ea1ffa3
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 @@ -83,6 +83,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 @@ -1636,9 +1637,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 @@ -328,11 +371,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 @@ -1707,6 +1752,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 @@ -1905,6 +2006,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

3 comments on commit ea1ffa3

@openjdk-notifier
Copy link

Choose a reason for hiding this comment

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

@rkennke
Copy link
Contributor Author

Choose a reason for hiding this comment

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

/backport jdk21u

@openjdk
Copy link

@openjdk openjdk bot commented on ea1ffa3 Nov 24, 2023

Choose a reason for hiding this comment

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

@rkennke the backport was successfully created on the branch rkennke-backport-ea1ffa34 in my personal fork of openjdk/jdk21u. To create a pull request with this backport targeting openjdk/jdk21u:master, just click the following link:

➡️ Create pull request

The title of the pull request is automatically filled in correctly and below you find a suggestion for the pull request body:

Hi all,

This pull request contains a backport of commit ea1ffa34 from the openjdk/jdk repository.

The commit being backported was authored by Roman Kennke on 10 Nov 2023 and was reviewed by Dean Long and Richard Reingruber.

Thanks!

If you need to update the source branch of the pull then run the following commands in a local clone of your personal fork of openjdk/jdk21u:

$ git fetch https://github.com/openjdk-bots/jdk21u.git rkennke-backport-ea1ffa34:rkennke-backport-ea1ffa34
$ git checkout rkennke-backport-ea1ffa34
# make changes
$ git add paths/to/changed/files
$ git commit --message 'Describe additional changes made'
$ git push https://github.com/openjdk-bots/jdk21u.git rkennke-backport-ea1ffa34

Please sign in to comment.