On Wed, 22 Oct 2025 05:28:37 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:
>
> the updated comments
The fix looks pretty good. I've posted several nits though.
test/hotspot/jtreg/serviceability/jvmti/events/FieldAccess/FieldsEventsFromJNI/FieldsEventsFromJNI.java
line 48:
> 46: public static void main(String[] args) throws InterruptedException {
> 47: System.loadLibrary("FieldsEventsFromJNI");
> 48: FieldsEventsFromJNI c = new FieldsEventsFromJNI();
Nit: Better to give the local some meaningful name instead of `c`. :)
test/hotspot/jtreg/serviceability/jvmti/events/FieldAccess/FieldsEventsFromJNI/FieldsEventsFromJNI.java
line 62:
> 60: synchronized(lock) {
> 61: anotherThread.start();
> 62: while(!isAnotherThreadStarted) {
Nit: Missed space after `while` and `synchronized` at lines: 53, 56, 60 and 62.
test/hotspot/jtreg/serviceability/jvmti/events/FieldAccess/FieldsEventsFromJNI/libFieldsEventsFromJNI.cpp
line 135:
> 133: fatal(jni, "No class found");
> 134: }
> 135: jfieldID fieldToRead = jni->GetFieldID(cls, "accessField",
> "Ljava/lang/String;");
Nit: It is better to define a constant for the field name `"accessField"`. Also
it used in two places at lines: 52 and 135. The same is true for the
`"SetFieldAccessWatch"`.
test/hotspot/jtreg/serviceability/jvmti/events/FieldAccess/FieldsEventsFromJNI/libFieldsEventsFromJNI.cpp
line 193:
> 191: check_jvmti_error(err, "SetEventNotificationMode");
> 192:
> 193: if (modify_cnt != isEventExpected) {
Nit: Comparing an `int` with a jboolean`. The same is at line 160. I'd suggest
to pass expected number of events instead of jboolean. Then it is better to
define `access_cnt` and `modify_cnt` as `jint`.
-------------
PR Review: https://git.openjdk.org/jdk/pull/27584#pullrequestreview-3375321998
PR Review Comment: https://git.openjdk.org/jdk/pull/27584#discussion_r2459466954
PR Review Comment: https://git.openjdk.org/jdk/pull/27584#discussion_r2459470649
PR Review Comment: https://git.openjdk.org/jdk/pull/27584#discussion_r2459533264
PR Review Comment: https://git.openjdk.org/jdk/pull/27584#discussion_r2459543963
