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

Wed Feb 21 22:48:59 UTC 2024

On Tue, 20 Feb 2024 23:48:06 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 incrementally with one additional commit since the last revision:
> 
>   review: addressed minor issue with use of []; corrected the test desctiption

Changes requested by lmesnik (Reviewer).

test/hotspot/jtreg/serviceability/jvmti/GetObjectMonitorUsage/ObjectMonitorUsage.java line 42:

> 40:  *       - all the above is checked for both platform and virtual threads
> 41:  * @requires vm.jvmti
> 42:  * @compile ObjectMonitorUsage.java

No need to have explicit compile for the test code.

test/hotspot/jtreg/serviceability/jvmti/GetObjectMonitorUsage/ObjectMonitorUsage.java line 48:

> 46: public class ObjectMonitorUsage {
> 47: 
> 48:     final static int JCK_STATUS_BASE = 95;

JCK_STATUS_BASE is not used, need to be removed.

test/hotspot/jtreg/serviceability/jvmti/GetObjectMonitorUsage/ObjectMonitorUsage.java line 72:

> 70:      * - zero threads waiting to be notified
> 71:      */
> 72:     static void test1(boolean isVirtual) throws Error {

no need to add throws for unchecked excption

test/hotspot/jtreg/serviceability/jvmti/GetObjectMonitorUsage/ObjectMonitorUsage.java line 204:

> 202: 
> 203:         // test virtual threads
> 204:         test1(false);

shouldn't be true here?

test/hotspot/jtreg/serviceability/jvmti/GetObjectMonitorUsage/libObjectMonitorUsage.cpp line 37:

> 35: static jvmtiCapabilities caps;
> 36: static jint result = PASSED;
> 37: static jboolean printdump = JNI_FALSE;

if there are not too much logging, better just to enable log by default and remove this variable

test/hotspot/jtreg/serviceability/jvmti/GetObjectMonitorUsage/libObjectMonitorUsage.cpp line 105:

> 103:   err = jvmti->GetObjectMonitorUsage(obj, &inf);
> 104:   if (err == JVMTI_ERROR_MUST_POSSESS_CAPABILITY && !caps.can_get_monitor_info) {
> 105:     return; /* Ok, it's expected */

I think we don't need this path.

test/hotspot/jtreg/serviceability/jvmti/GetObjectMonitorUsage/libObjectMonitorUsage.cpp line 107:

> 105:     return; /* Ok, it's expected */
> 106:   } else if (err != JVMTI_ERROR_NONE) {
> 107:     LOG("(GetMonitorInfo#%d) unexpected error: %s (%d)\n",

you could use check_jvmti_status

test/hotspot/jtreg/serviceability/jvmti/GetObjectMonitorUsage/libObjectMonitorUsage.cpp line 117:

> 115:       LOG(">>> [%2d]    owner: none (0x0)\n", count);
> 116:     } else {
> 117:       err = jvmti->GetThreadInfo(inf.owner, &tinf);

need to check err status.

test/hotspot/jtreg/serviceability/jvmti/GetObjectMonitorUsage/libObjectMonitorUsage.cpp line 126:

> 124:         LOG(">>>       waiters:\n");
> 125:         for (j = 0; j < inf.waiter_count; j++) {
> 126:           err = jvmti->GetThreadInfo(inf.waiters[j], &tinf);

need to check err.

test/hotspot/jtreg/vmTestbase/nsk/jvmti/GetObjectMonitorUsage/objmonusage003.java line 31:

> 29: 
> 30:     final static int JCK_STATUS_BASE = 95;
> 31:     final static int NUMBER_OF_THREADS = 16;

Better to remove the test if it already converted to avoid mess.

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

PR Review: https://git.openjdk.org/jdk/pull/17680#pullrequestreview-1894528980
PR Review Comment: https://git.openjdk.org/jdk/pull/17680#discussion_r1498395120
PR Review Comment: https://git.openjdk.org/jdk/pull/17680#discussion_r1498385978
PR Review Comment: https://git.openjdk.org/jdk/pull/17680#discussion_r1498396928
PR Review Comment: https://git.openjdk.org/jdk/pull/17680#discussion_r1498388601
PR Review Comment: https://git.openjdk.org/jdk/pull/17680#discussion_r1498391687
PR Review Comment: https://git.openjdk.org/jdk/pull/17680#discussion_r1498392633
PR Review Comment: https://git.openjdk.org/jdk/pull/17680#discussion_r1498393662
PR Review Comment: https://git.openjdk.org/jdk/pull/17680#discussion_r1498393291
PR Review Comment: https://git.openjdk.org/jdk/pull/17680#discussion_r1498404333
PR Review Comment: https://git.openjdk.org/jdk/pull/17680#discussion_r1498394741