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

[Chris Plummer]

On Mon, 16 Dec 2024 23:25:49 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:
> 
>   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