On Wed, 16 Oct 2024 02:28:28 GMT, Serguei Spitsyn <[email protected]> wrote:
>> There is a race between JVMTI NotifyFramePop function and FramePop event
>> posting code.
>> The fix is to return JVMTI_ERROR_OPAQUE_FRAME if if a FramePop event with
>> depth 0 is requested by NotifyFramePop at the time when the target frame is
>> in exit epilogue, and MethodExit/FramePop events are being posted for it.
>>
>> Testing:
>> - verified locally with new test (developed by Chris):
>> `serviceability/jvmti/events/NotifyFramePopStressTest`
>> - TBD: mach5 tiers 1-6
>
> Serguei Spitsyn has updated the pull request incrementally with one
> additional commit since the last revision:
>
> review: resolved comments from Alex and Chris
Have you verified that the test still detects the bug? In other words, if you
disabled the fix, does the test fail? I was just a bit worried that with all
the changes to it, it might not be still be properly detecting the bug, and I
looked in the mach5 history and don't see this test failing for a couple of
weeks now.
test/hotspot/jtreg/serviceability/jvmti/events/NotifyFramePopStressTest/NotifyFramePopStressTest.java
line 26:
> 24: /**
> 25: * @test
> 26: * @summary JVMTI FRAME_POP event is sometimes missed if NotifyFramePop
> is called as a method is returning
Would be good to also add an `@bug` statement.
test/hotspot/jtreg/serviceability/jvmti/events/NotifyFramePopStressTest/libNotifyFramePopStressTest.cpp
line 84:
> 82: deallocate(jvmti, jni, name);
> 83: deallocate(jvmti, jni, (void*)last_notify_method);
> 84: fatal(jni, "FramePop event in wrong method\n");
This is the main purpose for this test. It used to just set `failed` and then
continue to run to detect additional errors, and then java side of the test
calls `failed()` to detect the failure. Now you exit the test process when
there is a failure. There is actually no purpose served for the `failed` flag
anymore.
test/hotspot/jtreg/serviceability/jvmti/events/NotifyFramePopStressTest/libNotifyFramePopStressTest.cpp
line 99:
> 97: res = jvm->GetEnv((void **) &jvmti, JVMTI_VERSION_9);
> 98: if (res != JNI_OK || jvmti == nullptr) {
> 99: LOG("GetEnv(JVMTI_VERSION_9) failedL error(%d)", res);
Suggestion:
LOG("GetEnv(JVMTI_VERSION_9) failed: error(%d)", res);
-------------
PR Review: https://git.openjdk.org/jdk/pull/21468#pullrequestreview-2373700405
PR Review Comment: https://git.openjdk.org/jdk/pull/21468#discussion_r1803809612
PR Review Comment: https://git.openjdk.org/jdk/pull/21468#discussion_r1803839032
PR Review Comment: https://git.openjdk.org/jdk/pull/21468#discussion_r1803826652
