On Fri, 1 Mar 2024 11:19:21 GMT, Serguei Spitsyn <sspit...@openjdk.org> wrote:
>> The implementation of the JVM TI `GetCurrentContendedMonitor()` does not >> match the spec. It can sometimes return an incorrect information about the >> contended monitor. Such a behavior does not match the function spec. >> With this update the `GetCurrentContendedMonitor()` is returning the monitor >> only when the specified thread is waiting to enter or re-enter the monitor, >> and the monitor is not returned when the specified thread is waiting in the >> `java.lang.Object.wait` to be notified. >> >> The implementations of the JDWP `ThreadReference.CurrentContendedMonitor` >> command and JDI `ThreadReference.currentContendedMonitor()` method are based >> and depend on this JVMTI function. The JDWP command and the JDI method were >> specified incorrectly and had incorrect behavior. The fix slightly corrects >> both the JDWP and JDI specs. The JDWP and JDI implementations have been also >> fixed because they use this JVM TI update. Please, see and review the >> related CSR and Release-Note. >> >> CSR: [8326024](https://bugs.openjdk.org/browse/JDK-8326024): JVM TI >> GetCurrentContendedMonitor is implemented incorrectly >> RN: [8326038](https://bugs.openjdk.org/browse/JDK-8326038): Release Note: >> JVM TI GetCurrentContendedMonitor is implemented incorrectly >> >> Testing: >> - tested with the mach5 tiers 1-6 > > Serguei Spitsyn has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains four additional > commits since the last revision: > > - Merge > - Merge > - fix specs: JDWP ThreadReference.CurrentContendedMonitor command, JDI > ThreadReference.currentContendedMonitor method > - 8256314: JVM TI GetCurrentContendedMonitor is implemented incorrectly The main functional fix seems fine. The JDWP and JDI spec changes seem fine - just a minor nit on wording. I don't know enough about how the tests work to comment on those. src/hotspot/share/prims/jvmtiEnvBase.cpp line 941: > 939: bool is_virtual = java_lang_VirtualThread::is_instance(thread_oop); > 940: jint state = is_virtual ? > JvmtiEnvBase::get_vthread_state(thread_oop, java_thread) > 941: : JvmtiEnvBase::get_thread_state(thread_oop, > java_thread); I've seen this in a couple of your PRs now. It would be nice to have a utility function to get the JVMTI thread state of any java.lang.Thread instance. src/java.se/share/data/jdwp/jdwp.spec line 1985: > 1983: "thread may be waiting to enter the object's monitor, or in " > 1984: "java.lang.Object.wait waiting to re-enter the monitor after > being " > 1985: "notified, interrupted, or timeout." `timed-out` would be the correct sense here. src/jdk.jdi/share/classes/com/sun/jdi/ThreadReference.java line 314: > 312: * synchronized method, the synchronized statement, or > 313: * {@link Object#wait} waiting to re-enter the monitor > 314: * after being notified, interrupted, or timeout. Again `timed-out`. ------------- Marked as reviewed by dholmes (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17944#pullrequestreview-1913493587 PR Review Comment: https://git.openjdk.org/jdk/pull/17944#discussion_r1510673055 PR Review Comment: https://git.openjdk.org/jdk/pull/17944#discussion_r1510665704 PR Review Comment: https://git.openjdk.org/jdk/pull/17944#discussion_r1510666362
