On Wed, 18 Dec 2024 03:28:30 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: > > review: removed unneeded check for JvmtiExport::can_post_frame_pop() src/hotspot/share/prims/jvmti.xml line 3096: > 3094: event will not be generated for any frames. > 3095: See the <eventlink id="FramePop"></eventlink> event for details. > 3096: <p/> I think `The specified thread must be suspended or must be the current thread.` should be added (as we have in `NotifyFramePop` description) src/hotspot/share/prims/jvmtiEnvThreadState.cpp line 79: > 77: _pops->remove_at(idx); > 78: } > 79: assert(_pops->length() == 0, "sanity check"); Suggestion: _pops->clear(); src/hotspot/share/prims/jvmtiEnvThreadState.cpp line 261: > 259: Thread *current = Thread::current(); > 260: #endif > 261: assert(get_thread() == nullptr || > get_thread()->is_handshake_safe_for(current), Suggestion: assert(get_thread() == nullptr || get_thread()->is_handshake_safe_for(Thread::current()), test/hotspot/jtreg/serviceability/jvmti/events/FramePop/ClearAllFramePops/libClearAllFramePops.cpp line 44: > 42: static > 43: bool isTestThread(JNIEnv *jni, jvmtiEnv *jvmti, jthread thr) { > 44: jvmtiThreadInfo inf = get_thread_info(jvmti, jni, thr); Only thread name is required, `get_thread_name` can be used test/hotspot/jtreg/serviceability/jvmti/events/FramePop/ClearAllFramePops/libClearAllFramePops.cpp line 55: > 53: jclass cls; > 54: char *mname, *msig, *csig; > 55: jvmtiThreadInfo inf = get_thread_info(jvmti, jni, thr); Only thread name is required, `get_thread_name` can be used test/hotspot/jtreg/serviceability/jvmti/events/FramePop/ClearAllFramePops/libClearAllFramePops.cpp line 81: > 79: jclass cls; > 80: char *csig; > 81: jvmtiThreadInfo inf = get_thread_info(jvmti, jni, thr); Looks like this is not needed (and inf.name not deallocated) test/hotspot/jtreg/serviceability/jvmti/events/FramePop/ClearAllFramePops/libClearAllFramePops.cpp line 137: > 135: LOG("(GetCapabilities) unexpected error: %s (%d)\n", > TranslateError(err), err); > 136: return JNI_ERR; > 137: } I don't think this is needed ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/22744#discussion_r1890964500 PR Review Comment: https://git.openjdk.org/jdk/pull/22744#discussion_r1890974928 PR Review Comment: https://git.openjdk.org/jdk/pull/22744#discussion_r1890976875 PR Review Comment: https://git.openjdk.org/jdk/pull/22744#discussion_r1890987527 PR Review Comment: https://git.openjdk.org/jdk/pull/22744#discussion_r1890987617 PR Review Comment: https://git.openjdk.org/jdk/pull/22744#discussion_r1890984746 PR Review Comment: https://git.openjdk.org/jdk/pull/22744#discussion_r1890985653
