RFR: 8328866: Add raw monitor rank support to the debug agent. [v2]

[Chris Plummer]

On Thu, 2 May 2024 21:09:42 GMT, Chris Plummer <cjplummer at openjdk.org> wrote:

>> src/jdk.jdwp.agent/share/native/libjdwp/util.c line 1243:
>> 
>>> 1241:     error = JVMTI_FUNC_PTR(gdata->jvmti,GetThreadInfo)
>>> 1242:             (gdata->jvmti, thread, &info);
>>> 1243:     return info.name;
>> 
>> Need to delete JNI reference info.thread_group and info.jthreadGroup (if not NULL)
>
> Did you mean info.context_class_loader, not info.jthreadGroup?
> 
> It looks like elsewhere when we call GetThreadInfo it is bracketed with the WITH_LOCAL_REFS macro to automatically push/pop a local refs frame, although it is only setting the size to 1, which seems like a bug. I'll do the same here.

Fixed. I also filed [JDK-8331747](https://bugs.openjdk.org/browse/JDK-8331747) to fix pre-existing cases of calling GetThreadInfo() and not freeing up the local refs.

>> src/jdk.jdwp.agent/share/native/libjdwp/util.c line 1262:
>> 
>>> 1260:     for (i = 0; i < NUM_DEBUG_RAW_MONITORS; i++) {
>>> 1261:         DebugRawMonitor* dbg_monitor = &dbg_monitors[i];
>>> 1262:         if (dbg_monitor == NULL) {
>> 
>> Should be `dbg_monitor->monitor`
>
> ok

Fixed.

>> src/jdk.jdwp.agent/share/native/libjdwp/util.c line 1279:
>> 
>>> 1277:         return; // This assert is not reliable if the VM is exiting
>>> 1278:     }
>>> 1279:     jthread current_thread = threadControl_currentThread();
>> 
>> Looks like all callers of the `assertIsCurrentThread()` have reference to the current thread. Maybe pass it as argument?
>
> Ok. There were a few changes w.r.t. when this API is called, as was when the callers have access to the current thread, so I that's why I ended up not passing it in.

Fixed.

>> src/jdk.jdwp.agent/share/native/libjdwp/util.c line 1281:
>> 
>>> 1279:     jthread current_thread = threadControl_currentThread();
>>> 1280:     if (!isSameObject(env, thread, current_thread)) {
>>> 1281:         tty_message("ERROR: Threads not the same: %p %p\n", thread, current_thread);
>> 
>> Not sure the addresses provide useful information. Maybe print thread names?
>
> ok

Fixed.

>> src/jdk.jdwp.agent/share/native/libjdwp/util.c line 1294:
>> 
>>> 1292:     // has a higher rank than the monitor we are about to enter.
>>> 1293:     DebugRawMonitorRank i;
>>> 1294:     for (i = 0; i < NUM_DEBUG_RAW_MONITORS; i++) {
>> 
>> No need to check monitors from the beginning.
>> Should be enough to check starting from `min(rank, FIRST_LEAF_DEBUG_RAW_MONITOR)`
>
> I originally started at `rank`, but then when I added the 2nd of the two checks below I switched to 0, but your min suggestion should be optimal.

Fixed.

>> src/jdk.jdwp.agent/share/native/libjdwp/util.c line 1470:
>> 
>>> 1468: 
>>> 1469:     // Assert that the current thread owns this monitor.
>>> 1470:     assertIsCurrentThread(getEnv(), dbg_monitor->ownerThread);
>> 
>> reading ownerThread (and other) fields without dbgRawMonitor is not MT-safe (it's not atomic)
>
> It is safe if you are the owner of the monitor.  So what that means is that this assert could potentially crash, but only if you are not the owner, which means if it didn't crash then it would have asserted. However, even when asserts are disabled, isSameObject() is still executed, and that's were the crash would happen. assertIsCurrentThread() really should be a no-op if asserts are not enabled. But even if that change is not made, it still would only crash if something bad was going on since the current thread should always own the monitor.

I fixed the issue with executing assertIsCurrentThread even when asserts are disabled.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/19044#discussion_r1591577188
PR Review Comment: https://git.openjdk.org/jdk/pull/19044#discussion_r1591577385
PR Review Comment: https://git.openjdk.org/jdk/pull/19044#discussion_r1591577559
PR Review Comment: https://git.openjdk.org/jdk/pull/19044#discussion_r1591577701
PR Review Comment: https://git.openjdk.org/jdk/pull/19044#discussion_r1591577876
PR Review Comment: https://git.openjdk.org/jdk/pull/19044#discussion_r1591578574