[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-serviceability-dev
Subject:    Re: RFR: JDK-8318626: GetClassFields does not filter out ConstantPool.constantPoolOop field
From:       Serguei Spitsyn <sspitsyn () openjdk ! org>
Date:       2023-10-28 1:03:36
Message-ID: 7_F4U-iyeatpOE7XcwLhFTQYj-ebsyPY86w8Tv9FJsA=.b7b76d6c-0ae3-4b4b-bc1f-849a10d59f40 () github ! com
[Download RAW message or body]

On Tue, 24 Oct 2023 00:46:54 GMT, Alex Menkov <amenkov@openjdk.org> wrote:

> FilteredFieldStream is intended to filter out some fields which does not represent \
> valid java objects. Currently the only filtered field is "constantPoolOop" from \
> jdk.internal.reflect.ConstantPool class. The change fixes FilteredFieldStream \
> implementation to handle cases when filtered fields is the last field of the class \
> ("constantPoolOop" is the only field of jdk.internal.reflect.ConstantPool) 
> Testing:
> - new test added that compares results of GetClassFields JVMTI function (it uses \
>                 FilteredFieldStream) with Class.getDeclaredFields();
> - test/hotspot/jtreg/vmTestbase/nsk/jvmti/GetClassFields tests

The fix itself looks good. I've posted a couple of nits on the new test.
Thanks,
Serguei

test/hotspot/jtreg/serviceability/FilteredFields/FilteredFieldsTest.java line 25:

> 23: 
> 24: /*
> 25:  * @test

@bug and @summary is needed as well.
The test folder has to be at least `test/hotspot/jtreg/serviceability/jvmti`.
It would be nice to check with Leonid on this.

test/hotspot/jtreg/serviceability/FilteredFields/libFilteredFieldsTest.cpp line 63:

> 61:     jfieldID *fields = nullptr;
> 62: 
> 63:     jvmtiError err = jvmti->GetClassFields(clazz, &fcount, &fields);

Nit: The function `check_jvmti_status()` or `check_jvmti_error()` from \
jvmti_common.h` can be used here. The function `fatal()` in other cases like at line \
54 can be used.

-------------

Marked as reviewed by sspitsyn (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/16328#pullrequestreview-1702664188
PR Review Comment: https://git.openjdk.org/jdk/pull/16328#discussion_r1375135273
PR Review Comment: https://git.openjdk.org/jdk/pull/16328#discussion_r1375137583


[prev in list] [next in list] [prev in thread] [next in thread]