RFR: 8256314: JVM TI GetCurrentContendedMonitor is implemented incorrectly [v6]

[Leonid Mesnik]

On Tue, 5 Mar 2024 06:18:56 GMT, Serguei Spitsyn <sspitsyn at 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