On Thu, 2 May 2024 21:09:42 GMT, Chris Plummer <cjplum...@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
