On Sat, 25 Oct 2025 02:00:48 GMT, Leonid Mesnik <[email protected]> wrote:
>> The field access/modification events set interp only mode and compiled frame
>> is not expected. However JNI might call `post_field_access_by_jni` while the
>> last java frame is compiled.
>>
>> 1) The thread switched to interponly mode while it is in JNI code. The
>> deoptimization is triggered but each frame is really changed only execution
>> returns to it. So last java frame was not executed and thus is still
>> compiled.
>> 2) The JNI accessed field from the thread where field events are not
>> enabled. So the `post_field_access_by_jni` is called in threads in
>> interp_only mode.
>>
>> The original example doesn't reproduce issue because of JDK changes and I
>> don't know of it is 1) or 2)I. I implemented regression test for both
>> problems.
>>
>> The location should be zero for JNI access.
>
> Leonid Mesnik has updated the pull request incrementally with one additional
> commit since the last revision:
>
> counter are updated
The fix looks good.
There are some minor notes about the test
test/hotspot/jtreg/serviceability/jvmti/events/FieldAccess/FieldEventsFromJNI/libFieldEventsFromJNI.cpp
line 33:
> 31: jvmtiEnv* jvmti_env;
> 32:
> 33: // The event counters are used to check events from differen threads.
Suggestion:
// The event counters are used to check events from different threads.
test/hotspot/jtreg/serviceability/jvmti/events/FieldAccess/FieldEventsFromJNI/libFieldEventsFromJNI.cpp
line 44:
> 42: static const char* ACCESS_METHOD_NAME = "enableEventsAndAccessField";
> 43: static const char* MODIFY_FIELD_NAME = "modifyField";
> 44: static const char* MODIFY_FIELD_VALUE = "modifyFieldValue";
Not used (TBH I don't see any value in checking ACCESS_FIELD_VALUE)
test/hotspot/jtreg/serviceability/jvmti/events/FieldAccess/FieldEventsFromJNI/libFieldEventsFromJNI.cpp
line 116:
> 114: Agent_OnLoad(JavaVM *vm, char *options, void *reserved) {
> 115: jvmtiEnv *jvmti = nullptr;
> 116: jint res = vm->GetEnv((void **) &jvmti, JVMTI_VERSION_21);
Suggestion:
jint res = vm->GetEnv((void **)&jvmti, JVMTI_VERSION_21);
test/hotspot/jtreg/serviceability/jvmti/events/FieldAccess/FieldEventsFromJNI/libFieldEventsFromJNI.cpp
line 122:
> 120: jvmtiError err = JVMTI_ERROR_NONE;
> 121: jvmtiCapabilities capabilities;
> 122: (void) memset(&capabilities, 0, sizeof (capabilities));
Suggestion:
(void)memset(&capabilities, 0, sizeof(capabilities));
test/hotspot/jtreg/serviceability/jvmti/events/FieldAccess/FieldEventsFromJNI/libFieldEventsFromJNI.cpp
line 128:
> 126: check_jvmti_error(err, "AddCapabilities");
> 127: jvmtiEventCallbacks callbacks;
> 128: (void) memset(&callbacks, 0, sizeof (callbacks));
Suggestion:
(void)memset(&callbacks, 0, sizeof(callbacks));
test/hotspot/jtreg/serviceability/jvmti/events/FieldAccess/FieldEventsFromJNI/libFieldEventsFromJNI.cpp
line 131:
> 129: callbacks.FieldAccess = &cbFieldAccess;
> 130: callbacks.FieldModification = &cbFieldModification;
> 131: err = jvmti->SetEventCallbacks(&callbacks, (int) sizeof
> (jvmtiEventCallbacks));
Suggestion:
err = jvmti->SetEventCallbacks(&callbacks, (jint)sizeof(jvmtiEventCallbacks));
test/hotspot/jtreg/serviceability/jvmti/events/FieldAccess/FieldEventsFromJNI/libFieldEventsFromJNI.cpp
line 160:
> 158: check_jvmti_error(err, "SetFieldAccessWatch");
> 159:
> 160: jstring jname = (jstring)jni->GetObjectField(self, fieldToRead);
The name is misleading. it's the field value, not name
test/hotspot/jtreg/serviceability/jvmti/events/FieldAccess/FieldEventsFromJNI/libFieldEventsFromJNI.cpp
line 167:
> 165: check_jvmti_error(err, "SetEventNotificationMode");
> 166:
> 167: const char* name_str = jni->GetStringUTFChars(jname, nullptr);
`value_str`?
test/hotspot/jtreg/serviceability/jvmti/events/FieldAccess/FieldEventsFromJNI/libFieldEventsFromJNI.cpp
line 174:
> 172:
> 173: if (access_cnt != numOfEventsExpected) {
> 174: fatal(jni, "The field access count is incorrect.");
For error analysis it would be useful to log actual and expected values
test/hotspot/jtreg/serviceability/jvmti/events/FieldAccess/FieldEventsFromJNI/libFieldEventsFromJNI.cpp
line 207:
> 205:
> 206: if (modify_cnt != numOfEventsExpected) {
> 207: fatal(jni, "The field access count should be 1.");
numOfEventsExpected can be 0
Would be useful to log actual and expected values
-------------
PR Review: https://git.openjdk.org/jdk/pull/27584#pullrequestreview-3396914324
PR Review Comment: https://git.openjdk.org/jdk/pull/27584#discussion_r2476050208
PR Review Comment: https://git.openjdk.org/jdk/pull/27584#discussion_r2476017561
PR Review Comment: https://git.openjdk.org/jdk/pull/27584#discussion_r2476030439
PR Review Comment: https://git.openjdk.org/jdk/pull/27584#discussion_r2476030963
PR Review Comment: https://git.openjdk.org/jdk/pull/27584#discussion_r2476031549
PR Review Comment: https://git.openjdk.org/jdk/pull/27584#discussion_r2476033084
PR Review Comment: https://git.openjdk.org/jdk/pull/27584#discussion_r2476043111
PR Review Comment: https://git.openjdk.org/jdk/pull/27584#discussion_r2476044112
PR Review Comment: https://git.openjdk.org/jdk/pull/27584#discussion_r2476045472
PR Review Comment: https://git.openjdk.org/jdk/pull/27584#discussion_r2476049693
