RFR: JDK-8317636: Improve heap walking API tests to verify correctness of field indexes [v2]

[Alex Menkov]

On Thu, 1 Feb 2024 00:17:57 GMT, Chris Plummer <cjplummer at openjdk.org> 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