RFR: 8340698: JVMTI FRAME_POP event is sometimes missed if NotifyFramePop is called as a method is returning [v2]

[Alex Menkov]

On Fri, 11 Oct 2024 09:26:42 GMT, Serguei Spitsyn <sspitsyn at openjdk.org> 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