On Sat, 18 Oct 2025 17:24:43 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:
>
> fixed comment
src/hotspot/share/prims/jvmtiExport.cpp line 2226:
> 2224: javaVFrame *jvf = thread->last_java_vframe(®_map);
> 2225: method = jvf->method();
> 2226: address = jvf->method()->code_base();
Nit: I'm thinking if this code can be simplified a little bit if the same
approach with `reg_map` can be used for interpreter frame as well.
src/hotspot/share/prims/jvmtiExport.cpp line 2332:
> 2330: javaVFrame *jvf = thread->last_java_vframe(®_map);
> 2331: method = jvf->method();
> 2332: address = jvf->method()->code_base();
Nit: Same question as above on a potential simplification by unifying the
interpreter and compile frame cases.
test/hotspot/jtreg/serviceability/jvmti/events/FieldAccess/FieldEventsFromJNI/TestFieldsEventsFromJNI.java
line 29:
> 27: * @bug 8224852
> 28: * @run main/othervm/native -agentlib:JvmtiFieldEventsFromJNI
> TestFieldsEventsFromJNI
> 29: * @run main/othervm/native -agentlib:JvmtiFieldEventsFromJNI -Xcomp
> TestFieldsEventsFromJNI
Nit: It is convenient when the test folder and the Java file have the same
name. Many tests in the `serviceability/jvmti` folder follow this rule. So, I'd
suggest to get rid of the `Test` prefix in this class name. An additional
advantage would be that the class name is shorter. :)
test/hotspot/jtreg/serviceability/jvmti/events/FieldAccess/FieldEventsFromJNI/TestFieldsEventsFromJNI.java
line 48:
> 46:
> 47: public static void main(String[] args) throws InterruptedException {
> 48: System.loadLibrary("JvmtiFieldEventsFromJNI");
Nit: Is this really needed? In fact, we have it in many tests though.
test/hotspot/jtreg/serviceability/jvmti/events/FieldAccess/FieldEventsFromJNI/libJvmtiFieldEventsFromJNI.cpp
line 43:
> 41: jni->DeleteLocalRef(object_class);
> 42: return obj_class_name;
> 43: }
Nit: Would it be good to add this into `jvmti_common.hpp`?
test/hotspot/jtreg/serviceability/jvmti/events/FieldAccess/FieldEventsFromJNI/libJvmtiFieldEventsFromJNI.cpp
line 102:
> 100: deallocate(jvmti,jni, f_name);
> 101:
> 102:
Nit: Unneeded empty lines: 68, 102 and 137.
test/lib/jdk/test/lib/jvmti/jvmti_common.hpp line 335:
> 333: check_jvmti_status(jni, err, "get_field_name: error in JVMTI
> GetFieldName call");
> 334: deallocate(jvmti,jni, signature);
> 335: deallocate(jvmti,jni, generic);
Nit: The lines 334 and 335 miss space after comma in the argument lists. Also,
it is better to pass `nullptr` instead of `&generic`, so there would not be
need to have this local and to deallocate the returned string.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/27584#discussion_r2447046252
PR Review Comment: https://git.openjdk.org/jdk/pull/27584#discussion_r2447054396
PR Review Comment: https://git.openjdk.org/jdk/pull/27584#discussion_r2447066803
PR Review Comment: https://git.openjdk.org/jdk/pull/27584#discussion_r2447075224
PR Review Comment: https://git.openjdk.org/jdk/pull/27584#discussion_r2447101802
PR Review Comment: https://git.openjdk.org/jdk/pull/27584#discussion_r2447108290
PR Review Comment: https://git.openjdk.org/jdk/pull/27584#discussion_r2447094326
