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

[Chris Plummer]

On Wed, 8 May 2024 15:47:09 GMT, Serguei Spitsyn <sspitsyn at openjdk.org> wrote:

>> Chris Plummer has updated the pull request incrementally with two additional commits since the last revision:
>> 
>>  - Fix jcheck extra whitespace.
>>  - Fix comment typo.
>
> src/jdk.jdwp.agent/share/native/libjdwp/util.c line 1297:
> 
>> 1295:     // We must hold the dbgRawMonitor when calling verifyMonitorRank()
>> 1296: 
>> 1297:     // Iterate over all the monitors and makes sure we don't already hold one that
> 
> Nit: Typo: "makes sure" => "make sure".

Fixed.

> src/jdk.jdwp.agent/share/native/libjdwp/util.c line 1343:
> 
>> 1341:         }
>> 1342:     }
>> 1343: }
> 
> Nit: I'd suggest to consider two separate loops instead of one, something like below:
> 
> 
> static void
> assertOrderFailure(jthread thread,  DebugRawMonitorRank rank, DebugRawMonitor* dbg_monitor, char* msg)
> {
>     char* threadName = getThreadName(thread);
>      tty_message("DebugRawMonitor %s failure: (%s: %d > %d) for thread (%s)",
>                  msg, dbg_monitor->name, dbg_monitor->rank, rank, threadName);
>      jvmtiDeallocate(threadName);
>      JDI_ASSERT(JNI_FALSE);
> }
> 
> static void
> verifyMonitorRank(JNIEnv *env, DebugRawMonitorRank rank, jthread thread)
> {
>     DebugRawMonitorRank i;
> 
>     // 1. Verify the held monitor's rank is lower than the rank of the monitor we are trying to enter.
>     for (i = rank + 1; i < FIRST_LEAF_DEBUG_RAW_MONITOR; i++) {
>         DebugRawMonitor* dbg_monitor = &dbg_monitors[i];
>         if (dbg_monitor->ownerThread != NULL &&
>             isSameObject(env, dbg_monitor->ownerThread, thread)) {
>             // the lower ranked monitor is already owned by this thread
>             assertOrderFailure(thread, rank, dbg_monitor, "RANK ORDER");
>         }
>     }
>     // 2. Verify there are no other leaf monitors owned but this thread.
>     for (i = FIRST_LEAF_DEBUG_RAW_MONITOR; i < NUM_DEBUG_RAW_MONITORS; i++) {
>         DebugRawMonitor* dbg_monitor = &dbg_monitors[i];
>         if (i != rank && isSameObject(env, dbg_monitor->ownerThread, thread)) {
>             // the leaf ranked monitor is already owned by this thread
>             assertOrderFailure(thread, rank, dbg_monitor, "LEAF RANK");
>         }
>     }
> 
> I hope, this can be tweaked to adopt the `popFrame` locks correction.

The special popFrame check needs to go in the first loop only, so it shouldn't be a problem or add any complexity that we don't already have.

One things to resolve is if we enter a regular monitor while holding a leaf monitor, is this a "rank" failure or "leaf" failure. Currently in the code it is a "rank" failure. Your change would make it a "leaf" failure.

I'm not sure why you added the "i != rank" check with the leaf check. We should never be re-entering a leaf monitor. The same "dbg_monitor->ownerThread != NULL" check as in the first loop should be used.

assertOrderFailure() won't be able to print as descriptive of a message as what I currently have.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/19044#discussion_r1594526975
PR Review Comment: https://git.openjdk.org/jdk/pull/19044#discussion_r1594525608