On Tue, 21 Oct 2025 20:32:12 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 three
> additional commits since the last revision:
>
> - completed renaming
> - renamed test
> - updated after feedback
Thank you for the update! It looks pretty good to me.
I've posted several more suggestions though.
src/hotspot/share/prims/jvmtiExport.cpp line 2283:
> 2281: // In this case the frame is only marked for deoptimization but still
> remains compiled.
> 2282: // Also, the last frame might be compiled if events were not enabled
> for
> 2283: // this thread. The thread filtering is done later.
Q: The following part of comment is confusing: `" ... if events were not
enabled for this thread."
How do we post the events if they were not enabled? Do I miss anything?
test/hotspot/jtreg/serviceability/jvmti/events/FieldAccess/FieldEventsFromJNI/FieldsEventsFromJNI.java
line 30:
> 28: * @run main/othervm/native -agentlib:JvmtiFieldEventsFromJNI
> FieldsEventsFromJNI
> 29: */
> 30: public class FieldsEventsFromJNI {
Nit: The class name still does not match the test folder name:
`s/FieldsEventsFromJNI/FieldEventsFromJNI/g`
test/hotspot/jtreg/serviceability/jvmti/events/FieldAccess/FieldEventsFromJNI/libJvmtiFieldEventsFromJNI.cpp
line 44:
> 42: }
> 43: deallocate(jvmti,jni, m_name);
> 44:
Nit: Still unneeded empty line. :)
test/hotspot/jtreg/serviceability/jvmti/events/FieldAccess/FieldEventsFromJNI/libJvmtiFieldEventsFromJNI.cpp
line 167:
> 165: }
> 166:
> 167:
Nit: Still unneeded empty line. :)
test/lib/jdk/test/lib/jvmti/jvmti_common.hpp line 333:
> 331: jvmtiError err = jvmti->GetFieldName(field_class, field, &name,
> &signature, nullptr);
> 332: check_jvmti_status(jni, err, "get_field_name: error in JVMTI
> GetFieldName call");
> 333: deallocate(jvmti, jni, signature);
Nit: Sorry, I've not noticed that the signature is not really needed. Then
`nullptr` can be passed instead of `&signature` as well.
-------------
PR Review: https://git.openjdk.org/jdk/pull/27584#pullrequestreview-3363022782
PR Review Comment: https://git.openjdk.org/jdk/pull/27584#discussion_r2449945745
PR Review Comment: https://git.openjdk.org/jdk/pull/27584#discussion_r2449929078
PR Review Comment: https://git.openjdk.org/jdk/pull/27584#discussion_r2449926090
PR Review Comment: https://git.openjdk.org/jdk/pull/27584#discussion_r2449926933
PR Review Comment: https://git.openjdk.org/jdk/pull/27584#discussion_r2449924748
