8318895: Deoptimization results in incorrect lightweight locking stack

rkennke · xmas92 · reinrich · rkennke · commit ea1ffa341924 · 2023-11-10T15:28:27.000Z
Co-authored-by: Axel Boldt-Christmas &lt;aboldtch@openjdk.org&gt;
Co-authored-by: Richard Reingruber &lt;rrich@openjdk.org&gt;
Reviewed-by: dlong, rrich
diff --git a/src/hotspot/share/runtime/deoptimization.cpp b/src/hotspot/share/runtime/deoptimization.cpp
@@ -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"
@@ -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");
+        }
       }
     }
   }
diff --git a/test/jdk/com/sun/jdi/EATests.java b/test/jdk/com/sun/jdi/EATests.java
@@ -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
@@ -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
@@ -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
@@ -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.
@@ -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();
@@ -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);
@@ -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 {
 
@@ -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.

Original file line number	Diff line number	Diff line change
`@@ -83,6 +83,7 @@`
`83`	`83`	`#include "runtime/stackValue.hpp"`
`84`	`84`	`#include "runtime/stackWatermarkSet.hpp"`
`85`	`85`	`#include "runtime/stubRoutines.hpp"`
	`86`	`+#include "runtime/synchronizer.hpp"`
`86`	`87`	`#include "runtime/threadSMR.hpp"`
`87`	`88`	`#include "runtime/threadWXSetters.inline.hpp"`
`88`	`89`	`#include "runtime/vframe.hpp"`
`@@ -1636,9 +1637,19 @@ bool Deoptimization::relock_objects(JavaThread* thread, GrowableArray<MonitorInf`
`1636`	`1637`	`}`
`1637`	`1638`	`}`
`1638`	`1639`	`}`
`1639`		`- BasicLock* lock = mon_info->lock();`
`1640`		`- ObjectSynchronizer::enter(obj, lock, deoptee_thread);`
`1641`		`- assert(mon_info->owner()->is_locked(), "object must be locked now");`
	`1640`	`+ if (LockingMode == LM_LIGHTWEIGHT && exec_mode == Unpack_none) {`
	`1641`	`+ // We have lost information about the correct state of the lock stack.`
	`1642`	`+ // Inflate the locks instead. Enter then inflate to avoid races with`
	`1643`	`+ // deflation.`
	`1644`	`+ ObjectSynchronizer::enter(obj, nullptr, deoptee_thread);`
	`1645`	`+ assert(mon_info->owner()->is_locked(), "object must be locked now");`
	`1646`	`+ ObjectMonitor* mon = ObjectSynchronizer::inflate(deoptee_thread, obj(), ObjectSynchronizer::inflate_cause_vm_internal);`
	`1647`	`+ assert(mon->owner() == deoptee_thread, "must be");`
	`1648`	`+ } else {`
	`1649`	`+ BasicLock* lock = mon_info->lock();`
	`1650`	`+ ObjectSynchronizer::enter(obj, lock, deoptee_thread);`
	`1651`	`+ assert(mon_info->owner()->is_locked(), "object must be locked now");`
	`1652`	`+ }`
`1642`	`1653`	`}`
`1643`	`1654`	`}`
`1644`	`1655`	`}`