On Wed, 8 May 2024 15:47:09 GMT, Serguei Spitsyn <sspit...@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
