Re: RFR: JDK-8317636: Improve heap walking API tests to verify correctness of field indexes [v3]
On Sat, 3 Feb 2024 00:44:42 GMT, Serguei Spitsyn wrote:
>> Alex Menkov has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> indent
>
> test/hotspot/jtreg/serviceability/jvmti/FollowReferences/FieldIndices/libFieldIndicesTest.cpp
> line 64:
>
>> 62: static bool is_static_field(JNIEnv* env, jclass klass, jfieldID fid) {
>> 63: enum {
>> 64:ACC_STATIC= 0x0008,
>
> Nit: Indent is 1 instead of 2.
fixed.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17580#discussion_r1476916568
Re: RFR: JDK-8317636: Improve heap walking API tests to verify correctness of field indexes [v4]
> The fix adds new test for FollowReferences JVMTI function to verify > correctness of reported field indexes. Alex Menkov has updated the pull request incrementally with one additional commit since the last revision: indent - Changes: - all: https://git.openjdk.org/jdk/pull/17580/files - new: https://git.openjdk.org/jdk/pull/17580/files/a0fffef3..42e79303 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17580&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17580&range=02-03 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/17580.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17580/head:pull/17580 PR: https://git.openjdk.org/jdk/pull/17580
Re: RFR: JDK-8317636: Improve heap walking API tests to verify correctness of field indexes [v3]
On Fri, 2 Feb 2024 23:10:32 GMT, Alex Menkov wrote:
>> The fix adds new test for FollowReferences JVMTI function to verify
>> correctness of reported field indexes.
>
> Alex Menkov has updated the pull request incrementally with one additional
> commit since the last revision:
>
> indent
Marked as reviewed by sspitsyn (Reviewer).
test/hotspot/jtreg/serviceability/jvmti/FollowReferences/FieldIndices/libFieldIndicesTest.cpp
line 64:
> 62: static bool is_static_field(JNIEnv* env, jclass klass, jfieldID fid) {
> 63: enum {
> 64:ACC_STATIC= 0x0008,
Nit: Indent is 1 instead of 2.
-
PR Review: https://git.openjdk.org/jdk/pull/17580#pullrequestreview-1860688173
PR Review Comment: https://git.openjdk.org/jdk/pull/17580#discussion_r1476897942
Re: RFR: JDK-8317636: Improve heap walking API tests to verify correctness of field indexes [v2]
On Fri, 2 Feb 2024 06:47:22 GMT, Serguei Spitsyn wrote:
>> Alex Menkov has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> feedback
>
> test/hotspot/jtreg/serviceability/jvmti/FollowReferences/FieldIndices/libFieldIndicesTest.cpp
> line 512:
>
>> 510: JNIEXPORT jboolean JNICALL
>> 511: Java_FieldIndicesTest_testFailed(JNIEnv *env, jclass cls) {
>> 512: return test_failed ? JNI_TRUE : JNI_FALSE;
>
> The indent for native files has to be 2, not 4. Even though there are still
> some tests with wrong indents I'd suggest for new files to follow this rule.
Fixed. Going to integrate the change after sanity testing
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17580#discussion_r1476865492
Re: RFR: JDK-8317636: Improve heap walking API tests to verify correctness of field indexes [v3]
> The fix adds new test for FollowReferences JVMTI function to verify > correctness of reported field indexes. Alex Menkov has updated the pull request incrementally with one additional commit since the last revision: indent - Changes: - all: https://git.openjdk.org/jdk/pull/17580/files - new: https://git.openjdk.org/jdk/pull/17580/files/c5e7b787..a0fffef3 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17580&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17580&range=01-02 Stats: 391 lines in 1 file changed: 56 ins; 56 del; 279 mod Patch: https://git.openjdk.org/jdk/pull/17580.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17580/head:pull/17580 PR: https://git.openjdk.org/jdk/pull/17580
Re: RFR: JDK-8317636: Improve heap walking API tests to verify correctness of field indexes [v2]
On Thu, 1 Feb 2024 01:38:21 GMT, Alex Menkov wrote:
>> The fix adds new test for FollowReferences JVMTI function to verify
>> correctness of reported field indexes.
>
> Alex Menkov has updated the pull request incrementally with one additional
> commit since the last revision:
>
> feedback
Nice test, thank you for developing it!
Looks good.
test/hotspot/jtreg/serviceability/jvmti/FollowReferences/FieldIndices/libFieldIndicesTest.cpp
line 512:
> 510: JNIEXPORT jboolean JNICALL
> 511: Java_FieldIndicesTest_testFailed(JNIEnv *env, jclass cls) {
> 512: return test_failed ? JNI_TRUE : JNI_FALSE;
The indent for native files has to be 2, not 4. Even though there are still
some tests with wrong indents I'd suggest for new files to follow this rule.
-
Marked as reviewed by sspitsyn (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/17580#pullrequestreview-1858460187
PR Review Comment: https://git.openjdk.org/jdk/pull/17580#discussion_r1475611992
Re: RFR: JDK-8317636: Improve heap walking API tests to verify correctness of field indexes [v2]
On Thu, 1 Feb 2024 01:38:21 GMT, Alex Menkov wrote: >> The fix adds new test for FollowReferences JVMTI function to verify >> correctness of reported field indexes. > > Alex Menkov has updated the pull request incrementally with one additional > commit since the last revision: > > feedback Marked as reviewed by cjplummer (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/17580#pullrequestreview-1855244184
Re: RFR: JDK-8317636: Improve heap walking API tests to verify correctness of field indexes [v2]
On Thu, 1 Feb 2024 00:17:57 GMT, Chris Plummer wrote:
>> Alex Menkov has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> feedback
>
> test/hotspot/jtreg/serviceability/jvmti/FollowReferences/FieldIndices/libFieldIndicesTest.cpp
> line 93:
>
>> 91:
>> 92: /*
>> 93: Per jvmtiHeapReferenceInfoField spec:
>
> I think you should also include mention that this is for
> JVMTI_HEAP_REFERENCE_STATIC_FIELD.
>
> Indentation should be fixed to match our C++ comment style, and the "bullets"
> from the spec should be included to make it clearer that these are two lists
> of steps.
Fixed.
> test/hotspot/jtreg/serviceability/jvmti/FollowReferences/FieldIndices/libFieldIndicesTest.cpp
> line 108:
>
>> 106: where n is the count of the fields in all the superinterfaces of I.
>> 107:
>> 108: 'Klass' struct contains all required data to calculate field indeces.
>
> "indices"
Fixed.
> test/hotspot/jtreg/serviceability/jvmti/FollowReferences/FieldIndices/libFieldIndicesTest.cpp
> line 153:
>
>> 151:
>> 152: /*
>> 153: For each test object 'Object' struct is created and pointer to it set
>> as a tag for jobject.
>
> Suggestion:
>
> For each test object, the 'Object' struct is created and a pointer to it is
> set as the jobject's tag.
Fixed.
Updated comment for 'Klass' class the same way
> test/hotspot/jtreg/serviceability/jvmti/FollowReferences/FieldIndices/libFieldIndicesTest.cpp
> line 225:
>
>> 223:
>> 224: // Explores all interfaces implemented by 'klass' sorting out duplicates
>> 225: // and store them in the 'arr' starting from 'index'.
>
> Suggestion:
>
> // Explores all interfaces implemented by 'klass', sorts out duplicates,
> // and stores the interfaces in the 'arr' starting from 'index'.
Fixed.
> test/hotspot/jtreg/serviceability/jvmti/FollowReferences/FieldIndices/libFieldIndicesTest.cpp
> line 254:
>
>> 252:
>> 253: // And explore its superinterfaces.
>> 254: count += fill_interfaces(arr, index + count, env,
>> interfaces[i]);
>
> I assume here we are always dealing with test classes that are known not to
> be deeply nested.
Yes, we explore only test classes/interfaces
> test/hotspot/jtreg/serviceability/jvmti/FollowReferences/FieldIndices/libFieldIndicesTest.cpp
> line 348:
>
>> 346:
>> 347: jclass obj_klass = env->GetObjectClass(obj);
>> 348:
>
> Unnecessary newline.
removed.
> test/hotspot/jtreg/serviceability/jvmti/FollowReferences/FieldIndices/libFieldIndicesTest.cpp
> line 350:
>
>> 348:
>> 349: Klass* klass = Klass::explore(env, obj_klass);
>> 350:
>
> Unnecessary newline.
removed
> test/hotspot/jtreg/serviceability/jvmti/FollowReferences/FieldIndices/libFieldIndicesTest.cpp
> line 492:
>
>> 490: Java_FieldIndicesTest_prepare(JNIEnv *env, jclass cls, jobject testObj)
>> {
>> 491: Object::explore(env, testObj);
>> 492: fflush(0);
>
> stdout?
fflush(0) flashes all opened streams (including stout and stderr).
It's to handle the case when some shared test routines log to stderr.
But flash() expects `FILE *`, so I updated all calls to use `nullptr`
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17580#discussion_r1473693579
PR Review Comment: https://git.openjdk.org/jdk/pull/17580#discussion_r1473695968
PR Review Comment: https://git.openjdk.org/jdk/pull/17580#discussion_r1473696254
PR Review Comment: https://git.openjdk.org/jdk/pull/17580#discussion_r1473696356
PR Review Comment: https://git.openjdk.org/jdk/pull/17580#discussion_r1473698778
PR Review Comment: https://git.openjdk.org/jdk/pull/17580#discussion_r1473701344
PR Review Comment: https://git.openjdk.org/jdk/pull/17580#discussion_r1473701389
PR Review Comment: https://git.openjdk.org/jdk/pull/17580#discussion_r1473701268
Re: RFR: JDK-8317636: Improve heap walking API tests to verify correctness of field indexes [v2]
> The fix adds new test for FollowReferences JVMTI function to verify > correctness of reported field indexes. Alex Menkov has updated the pull request incrementally with one additional commit since the last revision: feedback - Changes: - all: https://git.openjdk.org/jdk/pull/17580/files - new: https://git.openjdk.org/jdk/pull/17580/files/6276448d..c5e7b787 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17580&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17580&range=00-01 Stats: 27 lines in 1 file changed: 1 ins; 3 del; 23 mod Patch: https://git.openjdk.org/jdk/pull/17580.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17580/head:pull/17580 PR: https://git.openjdk.org/jdk/pull/17580
Re: RFR: JDK-8317636: Improve heap walking API tests to verify correctness of field indexes
On Thu, 25 Jan 2024 23:05:19 GMT, Alex Menkov wrote:
> The fix adds new test for FollowReferences JVMTI function to verify
> correctness of reported field indexes.
test/hotspot/jtreg/serviceability/jvmti/FollowReferences/FieldIndices/libFieldIndicesTest.cpp
line 93:
> 91:
> 92: /*
> 93: Per jvmtiHeapReferenceInfoField spec:
I think you should also include mention that this is for
JVMTI_HEAP_REFERENCE_STATIC_FIELD.
Indentation should be fixed to match our C++ comment style, and the "bullets"
from the spec should be included to make it clearer that these are two lists of
steps.
test/hotspot/jtreg/serviceability/jvmti/FollowReferences/FieldIndices/libFieldIndicesTest.cpp
line 108:
> 106: where n is the count of the fields in all the superinterfaces of I.
> 107:
> 108: 'Klass' struct contains all required data to calculate field indeces.
"indices"
test/hotspot/jtreg/serviceability/jvmti/FollowReferences/FieldIndices/libFieldIndicesTest.cpp
line 153:
> 151:
> 152: /*
> 153: For each test object 'Object' struct is created and pointer to it set as
> a tag for jobject.
Suggestion:
For each test object, the 'Object' struct is created and a pointer to it is set
as the jobject's tag.
test/hotspot/jtreg/serviceability/jvmti/FollowReferences/FieldIndices/libFieldIndicesTest.cpp
line 225:
> 223:
> 224: // Explores all interfaces implemented by 'klass' sorting out duplicates
> 225: // and store them in the 'arr' starting from 'index'.
Suggestion:
// Explores all interfaces implemented by 'klass', sorts out duplicates,
// and stores the interfaces in the 'arr' starting from 'index'.
test/hotspot/jtreg/serviceability/jvmti/FollowReferences/FieldIndices/libFieldIndicesTest.cpp
line 254:
> 252:
> 253: // And explore its superinterfaces.
> 254: count += fill_interfaces(arr, index + count, env, interfaces[i]);
I assume here we are always dealing with test classes that are known not to be
deeply nested.
test/hotspot/jtreg/serviceability/jvmti/FollowReferences/FieldIndices/libFieldIndicesTest.cpp
line 348:
> 346:
> 347: jclass obj_klass = env->GetObjectClass(obj);
> 348:
Unnecessary newline.
test/hotspot/jtreg/serviceability/jvmti/FollowReferences/FieldIndices/libFieldIndicesTest.cpp
line 350:
> 348:
> 349: Klass* klass = Klass::explore(env, obj_klass);
> 350:
Unnecessary newline.
test/hotspot/jtreg/serviceability/jvmti/FollowReferences/FieldIndices/libFieldIndicesTest.cpp
line 492:
> 490: Java_FieldIndicesTest_prepare(JNIEnv *env, jclass cls, jobject testObj) {
> 491: Object::explore(env, testObj);
> 492: fflush(0);
stdout?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17580#discussion_r1473643948
PR Review Comment: https://git.openjdk.org/jdk/pull/17580#discussion_r1473644156
PR Review Comment: https://git.openjdk.org/jdk/pull/17580#discussion_r1473646542
PR Review Comment: https://git.openjdk.org/jdk/pull/17580#discussion_r1473647766
PR Review Comment: https://git.openjdk.org/jdk/pull/17580#discussion_r1473649198
PR Review Comment: https://git.openjdk.org/jdk/pull/17580#discussion_r1473651074
PR Review Comment: https://git.openjdk.org/jdk/pull/17580#discussion_r1473651163
PR Review Comment: https://git.openjdk.org/jdk/pull/17580#discussion_r1473650827
RFR: JDK-8317636: Improve heap walking API tests to verify correctness of field indexes
The fix adds new test for FollowReferences JVMTI function to verify correctness of reported field indexes. - Commit messages: - jcheck - new test Changes: https://git.openjdk.org/jdk/pull/17580/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=17580&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8317636 Stats: 645 lines in 2 files changed: 645 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/17580.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17580/head:pull/17580 PR: https://git.openjdk.org/jdk/pull/17580
