On Tue, 21 Oct 2025 07:35:42 GMT, Serguei Spitsyn <[email protected]> wrote:
>> 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.
Thanks for suggestion, yes it works. The same approach might be used for
compiler/interpreter frame.
> 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. :)
We have all 3 different patterns:
jvmtiFieldEventsFromJNI/TestvmtiFieldEventsFromJNI.java
jvmtiFieldEventsFromJNI/jvmtiFieldEventsFromJNI.java
jvmtiFieldEventsFromJNI/jvmtiFieldEventsFromJNITest.java
and somtimes
jvmtiFieldEventsFromJNITest.java
TestvmtiFieldEventsFromJNI.java
while
`jvmtiFieldEventsFromJNI/`
directory contains native path only.
The 'Test' prefix or suffix useful to quickly find test entry if it has more
then on java file.
Do we want to use this convention for serviceability/jvmti tests?
jvmtiFieldEventsFromJNI/
jvmtiFieldEventsFromJNI/jvmtiFieldEventsFromJNI.java
jvmtiFieldEventsFromJNI/libJvmtiFieldEventsFromJNI.jcpp
If there are no objections, I'll rename the test.
> 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`?
Done.
> 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.
done
> test/hotspot/jtreg/serviceability/jvmti/events/FieldAccess/FieldsEventsFromJNI/FieldsEventsFromJNI.java
> line 48:
>
>> (failed to retrieve contents of file, check the PR for context)
> Nit: Is this really needed? In fact, we have it in many tests though.
Yes, it is needed for tests that have native methods to communicate with agent.
This test has:
private native void enableEventsAndAccessField(boolean isEventExpected,
Thread eventThread);
private native void enableEventsAndModifyField(boolean isEventExpected,
Thread eventThread);
> 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.
Thanks, updated.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/27584#discussion_r2448930076
PR Review Comment: https://git.openjdk.org/jdk/pull/27584#discussion_r2448918343
PR Review Comment: https://git.openjdk.org/jdk/pull/27584#discussion_r2448924000
PR Review Comment: https://git.openjdk.org/jdk/pull/27584#discussion_r2448925404
PR Review Comment: https://git.openjdk.org/jdk/pull/27584#discussion_r2448922851
PR Review Comment: https://git.openjdk.org/jdk/pull/27584#discussion_r2448926538
