RFR: 8247972: incorrect implementation of JVM TI GetObjectMonitorUsage [v5]

[David Holmes]

On Tue, 13 Feb 2024 07:11:18 GMT, Serguei Spitsyn <sspitsyn at openjdk.org> wrote:

>> The implementation of the JVM TI `GetObjectMonitorUsage` does not match the spec.
>> The function returns the following structure:
>> 
>> 
>> typedef struct {
>>     jthread owner;
>>     jint entry_count;
>>     jint waiter_count;
>>     jthread* waiters;
>>     jint notify_waiter_count;
>>     jthread* notify_waiters;
>> } jvmtiMonitorUsage;
>> 
>> 
>> The following four fields are defined this way:
>> 
>> waiter_count  [jint] The number of threads waiting to own this monitor
>> waiters  [jthread*] The waiter_count waiting threads
>> notify_waiter_count  [jint]  The number of threads waiting to be notified by this monitor
>> notify_waiters  [jthread*] The notify_waiter_count threads waiting to be notified
>> 
>> The `waiters` has to include all threads waiting to enter the monitor or to re-enter it in `Object.wait()`.
>> The implementation also includes the threads waiting to be notified in `Object.wait()` which is wrong.
>> The `notify_waiters` has to include all threads waiting to be notified in `Object.wait()`.
>> The implementation also includes the threads waiting to re-enter the monitor in `Object.wait()` which is wrong.
>> This update makes it right.
>> 
>> The implementation of the JDWP command `ObjectReference.MonitorInfo (5)` is based on the JVM TI `GetObjectMonitorInfo()`. This update has a tweak to keep the existing behavior of this command.
>> 
>> The follwoing JVMTI vmTestbase tests are fixed to adopt to the `GetObjectMonitorUsage()` correct behavior:
>> 
>>   jvmti/GetObjectMonitorUsage/objmonusage001
>>   jvmti/GetObjectMonitorUsage/objmonusage003
>> 
>> 
>> The following JVMTI JCK tests have to be fixed to adopt to correct behavior:
>> 
>> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00101/gomu00101.html
>> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00101/gomu00101a.html
>> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00102/gomu00102.html
>> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00102/gomu00102a.html
>> 
>> 
>> 
>> A JCK bug will be filed and the tests have to be added into the JCK problem list located in the closed repository.
>> 
>> Also, please see and review the related CSR:
>>  [8324677](https://bugs.openjdk.org/browse/JDK-8324677): incorrect implementation of JVM TI GetObjectMonitorUsage
>> 
>> The Release-Note is:
>> [8325314](https://bugs.openjdk.org/browse/JDK-8325314): Release Note: incorrect implementation of JVM TI GetObjectMonitorUsage
>>  
>> Testing:
>>  - tested with 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 five additional commits since the last revision:
> 
>  - Merge
>  - review: fixed issues in get_object_monitor_usage; extended test coverage in objmonusage003
>  - Merge
>  - review: thread in notify waiter list can't be BLOCKED
>  - 8324677: Specification clarification needed for JVM TI GetObjectMonitorUsage

> Why do you think the virtual threads are handled incorrectly?

Sorry my mistake. The `thread_or_vthread` changes had me confused.

src/hotspot/share/runtime/threads.cpp line 1200:

> 1198:     if (pending == monitor ||
> 1199:         (waiting == monitor && JavaThreadStatus::BLOCKED_ON_MONITOR_ENTER ==
> 1200:          java_lang_Thread::get_thread_status(p->vthread_or_thread()))

This code seems extremely racy. Is there anything that is stopping the target thread from running while we attempt this inspection of the oop state? If it was waiting and was interrupted or timed-out then we could mistakenly think it was still pending entry to this monitor when in fact it is not.
It is also a shame that we have to reach up into the oop thread state to figure out if if it is blocked on re-entry ...

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

PR Review: https://git.openjdk.org/jdk/pull/17680#pullrequestreview-1879239973
PR Review Comment: https://git.openjdk.org/jdk/pull/17680#discussion_r1488808389