On Mon, 16 Dec 2024 23:25:49 GMT, Serguei Spitsyn <sspit...@openjdk.org> wrote:
>> New JVMTI function `ClearAllFramePops` will help to speedup debugger single >> stepping in some cases. >> Additionally, the JVMTI `NotifyFramePop` implementation was fixed to return >> `JVMTI_ERROR_DUPLICATE` to make it consistent with the `SetBreakpoint` which >> also returns this error. >> >> The JDWP agent fix will be needed to make use of this new JVMTI function. >> The corresponding debugger bug is: >> [8229012](https://bugs.openjdk.org/browse/JDK-8229012): When single >> stepping, the debug agent can cause the thread to remain in interpreter mode >> after single stepping completes >> >> CSR: [8346144](https://bugs.openjdk.org/browse/JDK-8346144): add >> ClearAllFramePops function to speedup debugger single stepping in some cases >> >> Testing: >> - mach5 tiers 1-6 were run to make sure this fix caused no regressions >> - Chris tested the JVMTI patch with his JDWP fix of >> [8229012](https://bugs.openjdk.org/browse/JDK-8229012). > > Serguei Spitsyn has updated the pull request incrementally with one > additional commit since the last revision: > > added extra check to post_method_exit_inner before clear_frame_pop to avoid > assert All my debugger testing is passing now. A few comments inline. src/hotspot/share/interpreter/interpreterRuntime.cpp line 580: > 578: // notify debugger of an exception catch > 579: // (this is good for exceptions caught in native methods as well) > 580: if (JvmtiExport::can_post_on_exceptions() || > JvmtiExport::can_post_frame_pop()) { This doesn't seem like it is related to ClearAllFramePops. src/hotspot/share/prims/jvmtiEnvBase.cpp line 1365: > 1363: if (ets->is_frame_pop(frame_number)) { > 1364: return JVMTI_ERROR_DUPLICATE; > 1365: } It seems that this change is unrelated to ClearAllFramePops and perhaps deserves it's own CR, especially since it is a behavior change. src/hotspot/share/prims/jvmtiEnvThreadState.cpp line 263: > 261: #ifdef ASSERT > 262: Thread *current = Thread::current(); > 263: #endif I think you can just get rid of these lines and inline the Thread.current() call below. test/hotspot/jtreg/serviceability/jvmti/vthread/MethodExitTest/libMethodExitTest.cpp line 505: > 503: err = jvmti->NotifyFramePop(thread, 0); > 504: check_jvmti_status(jni, err, "VirtualThreadUnmount: error in JVMTI > NotifyFramePop"); > 505: I assume this is needed as the result of your fix to return DUPLICATE for NotifyFramePop. Do we know that it always returns DUPLICATE for this particular call site? ------------- Changes requested by cjplummer (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/22744#pullrequestreview-2507765531 PR Review Comment: https://git.openjdk.org/jdk/pull/22744#discussion_r1887841702 PR Review Comment: https://git.openjdk.org/jdk/pull/22744#discussion_r1887845533 PR Review Comment: https://git.openjdk.org/jdk/pull/22744#discussion_r1887849471 PR Review Comment: https://git.openjdk.org/jdk/pull/22744#discussion_r1887851575
