On Fri, 11 Oct 2024 09:26:42 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:
>
> minor comment tweak
The fix looks good.
Added notes about the test
test/hotspot/jtreg/serviceability/jvmti/events/NotifyFramePopStressTest/NotifyFramePopStressTest.java
line 103:
> 101: }
> 102: }
> 103: if (waitCount > 50) {
Is 50 is enough here? (like in "Xcomp" mode) the cycle above exits after 1000
iterations.
test/hotspot/jtreg/serviceability/jvmti/events/NotifyFramePopStressTest/NotifyFramePopStressTest.java
line 109:
> 107: }
> 108: }
> 109: System.out.println("control has finished: " + notifyCount);
Could you please update logging to use `log` or `System.out.println` in all
cases?
test/hotspot/jtreg/serviceability/jvmti/events/NotifyFramePopStressTest/libNotifyFramePopStressTest.cpp
line 43:
> 41: #endif
> 42:
> 43: #endif
This is copy of some old .c code. you don't need it in .cpp
test/hotspot/jtreg/serviceability/jvmti/events/NotifyFramePopStressTest/libNotifyFramePopStressTest.cpp
line 45:
> 43: #endif
> 44:
> 45: static jvmtiEnv *jvmti = NULL;
NULL -> nullptr
test/hotspot/jtreg/serviceability/jvmti/events/NotifyFramePopStressTest/libNotifyFramePopStressTest.cpp
line 49:
> 47: static jvmtiEventCallbacks callbacks;
> 48: static volatile jint popCount = 0;
> 49: static char* lastNotifyMethod;
it's accessed from different threads. should be volative at least (but better
would be be use atomic). The same for `popCount` and `failed`
test/hotspot/jtreg/serviceability/jvmti/events/NotifyFramePopStressTest/libNotifyFramePopStressTest.cpp
line 76:
> 74: jclass cls = NULL;
> 75: char* csig = NULL;
> 76: char* name = NULL;
NULL -> nullptr
test/hotspot/jtreg/serviceability/jvmti/events/NotifyFramePopStressTest/libNotifyFramePopStressTest.cpp
line 83:
> 81: check_jvmti_status(jni, err, "FramePop: Failed in JVMTI
> GetMethodDeclaringClass");
> 82:
> 83: err =jvmti->GetClassSignature(cls, &csig, NULL);
NULL -> nullptr
test/hotspot/jtreg/serviceability/jvmti/events/NotifyFramePopStressTest/libNotifyFramePopStressTest.cpp
line 93:
> 91: LOG("ERROR: FramePop event is for wrong method: expected %s, got
> %s\n", lastNotifyMethod, name);
> 92: failed = JNI_TRUE;
> 93: fatal(jni, "DBG: FramePop event in wrong method\n");
looks like "DBG" is leftover from other test?
test/hotspot/jtreg/serviceability/jvmti/events/NotifyFramePopStressTest/libNotifyFramePopStressTest.cpp
line 161:
> 159: if (isMain) {
> 160: if (seenMain) {
> 161: return JNI_FALSE; // Only do NotifyFramePop once for main()
`deallocate(jvmti, jni, name);`
test/hotspot/jtreg/serviceability/jvmti/events/NotifyFramePopStressTest/libNotifyFramePopStressTest.cpp
line 169:
> 167: err= jvmti->NotifyFramePop(thread, 0);
> 168: if (err == JVMTI_ERROR_OPAQUE_FRAME || err == JVMTI_ERROR_DUPLICATE) {
> 169: return JNI_FALSE;
`deallocate(jvmti, jni, name);`
-------------
PR Review: https://git.openjdk.org/jdk/pull/21468#pullrequestreview-2370871906
PR Review Comment: https://git.openjdk.org/jdk/pull/21468#discussion_r1802134354
PR Review Comment: https://git.openjdk.org/jdk/pull/21468#discussion_r1802134738
PR Review Comment: https://git.openjdk.org/jdk/pull/21468#discussion_r1802135343
PR Review Comment: https://git.openjdk.org/jdk/pull/21468#discussion_r1802136401
PR Review Comment: https://git.openjdk.org/jdk/pull/21468#discussion_r1802138245
PR Review Comment: https://git.openjdk.org/jdk/pull/21468#discussion_r1802138433
PR Review Comment: https://git.openjdk.org/jdk/pull/21468#discussion_r1802138857
PR Review Comment: https://git.openjdk.org/jdk/pull/21468#discussion_r1802140636
PR Review Comment: https://git.openjdk.org/jdk/pull/21468#discussion_r1802144857
PR Review Comment: https://git.openjdk.org/jdk/pull/21468#discussion_r1802145015
