On Thu, 30 Jul 2026 01:38:45 GMT, David Holmes <[email protected]> wrote:
>> This fixes the crash when printing deadlocks when the only lock held for the >> thread in the deadlock is the JVMTI raw monitor lock. This also fixes >> deadlock detection for only raw monitor locks and adds a test for it. >> >> make test TEST="serviceability/dcmd/thread >> vmTestbase/nsk/monitoring/ThreadMXBean/ThreadInfo/Deadlock >> serviceability/jvmti/SuspendWithRawMonitorEnter >> serviceability/jvmti/vthread/RawMonitorTest" >> >> Tested with jvmti deadlock detection tests above, and tier 1-4 in progress. >> >> --------- >> - [x] I confirm that I make this contribution in accordance with the >> [OpenJDK Interim AI Policy](https://openjdk.org/legal/ai). > > src/hotspot/share/services/threadService.cpp line 520: > >> 518: previousThread = currentThread; >> 519: waitingToLockMonitor = >> (ObjectMonitor*)currentThread->current_pending_monitor(); >> 520: waitingToLockRawMonitor = >> currentThread->current_pending_raw_monitor(); > > This seems reasonable, but I do worry a little because of the comment: > > // JVM TI raw monitors can also be involved in deadlocks, and we can be > // waiting to lock both a raw monitor and ObjectMonitor at the same time. > // It isn't clear how to make deadlock detection work correctly if that > // happens. > > Updating the raw monitor here might change something - but I struggle to see > exactly how this cycle detection is working anyway. Not updating the current thread's raw monitor seems to have been an omission in your JDK-8231289 change and not deliberate. The comment I think is trying to say that there can be deadlocks with both raw monitors and Java monitors, and below it says that it picks raw monitor deadlock to report in the cycle. It doesn't seem to justify keeping a stale raw monitor pointer through each iteration of the loop. > src/hotspot/share/services/threadService.cpp line 1054: > >> 1052: continue; >> 1053: } >> 1054: st->print_cr("%s \"%s\"", owner_desc, currentThread->name()); > > I was puzzled why you moved and duplicated this, but if I am reading things > correctly the old code would cause two similar print outs for the raw monitor > case. The line followed the if {} else {} statement because both printed that line, so I had to copy it because the raw monitor-only case did not want to copy it. It came out looking like this: Found one Java-level deadlock: ============================= "Thread-1": waiting to lock JVM TI raw monitor 0x00007fda347cb460, which is held by "Thread-2" , which is held by "Thread-2" > test/hotspot/jtreg/serviceability/dcmd/thread/PrintRawMonitorLockTest.java > line 41: > >> 39: * @test >> 40: * @bug 8253442 >> 41: * @summary Test of diagnostic command Thread.print with only JVMTI raw >> monitor. > > That doesn't seem to summarise this test I'll update the comment. > test/hotspot/jtreg/serviceability/dcmd/thread/PrintRawMonitorLockTest.java > line 53: > >> 51: >> 52: static private void log(String s) { System.out.println(s); } >> 53: static private String AGENT_LIB = "PrintRawMonitorLockTest";; > > Suggestion: > > private static void log(String s) { System.out.println(s); } > private static String AGENT_LIB = "PrintRawMonitorLockTest";; Adding. I wish your suggestions didn't add ^M to the lines. > test/hotspot/jtreg/serviceability/dcmd/thread/PrintRawMonitorLockTest.java > line 97: > >> 95: log("entered lock1"); >> 96: >> 97: /* Hold lock on "lock" to show up in thread dump */ > > Comment from an older version? The name "lock" is not used in the test. I'll update the comment. > test/hotspot/jtreg/serviceability/dcmd/thread/PrintRawMonitorLockTest.java > line 106: > >> 104: "retCode=" + retCode); >> 105: } >> 106: log("tried to enter lock2"); > > If you get here you succeeded. Which it won't do. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/32092#discussion_r3682759696 PR Review Comment: https://git.openjdk.org/jdk/pull/32092#discussion_r3682644213 PR Review Comment: https://git.openjdk.org/jdk/pull/32092#discussion_r3682660844 PR Review Comment: https://git.openjdk.org/jdk/pull/32092#discussion_r3682648454 PR Review Comment: https://git.openjdk.org/jdk/pull/32092#discussion_r3682657347 PR Review Comment: https://git.openjdk.org/jdk/pull/32092#discussion_r3682651237
