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

[David Holmes]

On Fri, 1 Mar 2024 11:19:21 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 with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains four additional commits since the last revision:
> 
>  - Merge
>  - Merge
>  - fix specs: JDWP ThreadReference.CurrentContendedMonitor command, JDI ThreadReference.currentContendedMonitor method
>  - 8256314: JVM TI GetCurrentContendedMonitor is implemented incorrectly

The main functional fix seems fine.

The JDWP and JDI spec changes seem fine - just a minor nit on wording.

I don't know enough about how the tests work to comment on those.

src/hotspot/share/prims/jvmtiEnvBase.cpp line 941:

> 939:     bool is_virtual = java_lang_VirtualThread::is_instance(thread_oop);
> 940:     jint state = is_virtual ? JvmtiEnvBase::get_vthread_state(thread_oop, java_thread)
> 941:                             : JvmtiEnvBase::get_thread_state(thread_oop, java_thread);

I've seen this in a couple of your PRs now. It would be nice to have a utility function to get the JVMTI thread state of any java.lang.Thread instance.

src/java.se/share/data/jdwp/jdwp.spec line 1985:

> 1983:         "thread may be waiting to enter the object's monitor, or in "
> 1984:         "java.lang.Object.wait waiting to re-enter the monitor after being "
> 1985:         "notified, interrupted, or timeout."

`timed-out` would be the correct sense here.

src/jdk.jdi/share/classes/com/sun/jdi/ThreadReference.java line 314:

> 312:      * synchronized method, the synchronized statement, or
> 313:      * {@link Object#wait} waiting to re-enter the monitor
> 314:      * after being notified, interrupted, or timeout.

Again `timed-out`.

-------------

Marked as reviewed by dholmes (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/17944#pullrequestreview-1913493587
PR Review Comment: https://git.openjdk.org/jdk/pull/17944#discussion_r1510673055
PR Review Comment: https://git.openjdk.org/jdk/pull/17944#discussion_r1510665704
PR Review Comment: https://git.openjdk.org/jdk/pull/17944#discussion_r1510666362