On Tue, 5 Mar 2024 06:18:56 GMT, Serguei Spitsyn <sspit...@openjdk.org> wrote:
>> The implementation of the JVM TI `GetCurrentContendedMonitor()` does not >> match the spec. It can sometimes return an incorrect information about the >> contended monitor. Such a behavior does not match the function spec. >> With this update the `GetCurrentContendedMonitor()` is returning the monitor >> only when the specified thread is waiting to enter or re-enter the monitor, >> and the monitor is not returned when the specified thread is waiting in the >> `java.lang.Object.wait` to be notified. >> >> The implementations of the JDWP `ThreadReference.CurrentContendedMonitor` >> command and JDI `ThreadReference.currentContendedMonitor()` method are based >> and depend on this JVMTI function. The JDWP command and the JDI method were >> specified incorrectly and had incorrect behavior. The fix slightly corrects >> both the JDWP and JDI specs. The JDWP and JDI implementations have been also >> fixed because they use this JVM TI update. Please, see and review the >> related CSR and Release-Note. >> >> CSR: [8326024](https://bugs.openjdk.org/browse/JDK-8326024): JVM TI >> GetCurrentContendedMonitor is implemented incorrectly >> RN: [8326038](https://bugs.openjdk.org/browse/JDK-8326038): Release Note: >> JVM TI GetCurrentContendedMonitor is implemented incorrectly >> >> Testing: >> - tested with the mach5 tiers 1-6 > > Serguei Spitsyn has updated the pull request incrementally with one > additional commit since the last revision: > > review: added new internal function > JvmtiEnvBase::get_thread_or_vthread_state The test changes look good, need just small doc update. The jvmti test serviceability/jvmti/thread/GetCurrentContendedMonitor/contmon01/contmon01.java is correct and don't check wait monitors. I wonder if it makes sense to add test which verify that thread waiting in Object.wait() is not included into the result. test/hotspot/jtreg/vmTestbase/nsk/jdi/ObjectReference/waitingThreads/waitingthreads004a.java line 106: > 104: display("entered and notifyAll: synchronized > (lockingObject) {}"); > 105: lockingObject.notifyAll(); > 106: Please update test documentation in TestDescription. line: - An object with threads waiting in Object.wait(long) method. should be updated/or another one added. test/hotspot/jtreg/vmTestbase/nsk/jdwp/ThreadReference/CurrentContendedMonitor/curcontmonitor001a.java line 88: > 86: // ensure that tested thread is waiting for monitor object > 87: synchronized (TestedClass.thread.monitor) { > 88: TestedClass.thread.monitor.notifyAll(); You need to update test documentation in TestDescription.java to explicitly say that test not "waiting" but exit from wait and waiting for monitor (contended). ------------- Marked as reviewed by lmesnik (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17944#pullrequestreview-1917844453 PR Review Comment: https://git.openjdk.org/jdk/pull/17944#discussion_r1513326686 PR Review Comment: https://git.openjdk.org/jdk/pull/17944#discussion_r1513323999
