On Tue, 17 Dec 2024 03:27:16 GMT, Chris Plummer <cjplum...@openjdk.org> wrote:
>> 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
>
> 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.
Okay. Removed the `JVMTI_ERROR_DUPLICATE` related changes. Will fix it
separately including the CSR.
> 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.
Okay, fixed.
> 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?
Yes, it is related to the DUPLICATE change.
The test calls JVMTI `NotifyFramePop` in the
`VirtualThreadMount`/`VirtualThreadUnmount` event handlers.
One of them is a dup for sure and not needed.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/22744#discussion_r1888942395
PR Review Comment: https://git.openjdk.org/jdk/pull/22744#discussion_r1888940655
PR Review Comment: https://git.openjdk.org/jdk/pull/22744#discussion_r1888939839
