RFR: 8346143: add ClearAllFramePops function to speedup debugger single stepping in some cases [v5]

[Alex Menkov]

On Wed, 18 Dec 2024 03:28:30 GMT, Serguei Spitsyn <sspitsyn at 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