RFR: 8311102: Write annotations in the classfile dumped by SA
Chris Plummer
cjplummer at openjdk.org
Tue Jul 11 05:25:13 UTC 2023
On Fri, 30 Jun 2023 14:32:03 GMT, Ashutosh Mehra <duke at openjdk.org> wrote:
> Please review this PR that enables ClassWriter to write annotations to the class file being dumped.
>
> The fields annotations are stored in `Annotations::_fields_annotations` which is of type `Array<Array<u1>*>`. There is no class in SA that can represent it. I have added ArrayOfU1Array to correspond to the type `Array<Array<u1>*>` and it works. I believe there are better approaches but that would require a bit more refactoring of the classes representing Array types. I will leave that for future work for now.
>
> Testing: `test/hotspot/jtreg/serviceability/sa` and `test/jdk/sun/tools/jhsdb`
> Tested it manually by dumping j.l.String class and comparing the annotations with the original class using javap.
> The test case mentioned in [JDK-8311101](https://bugs.openjdk.org/browse/JDK-8311101) would provide better coverage.
Overall the changes look good. Just a few minor suggestions.
src/hotspot/share/runtime/vmStructs.cpp line 972:
> 970: unchecked_nonstatic_field(Array<Klass*>, _data, sizeof(Klass*)) \
> 971: unchecked_nonstatic_field(Array<ResolvedIndyEntry>, _data, sizeof(ResolvedIndyEntry)) \
> 972: unchecked_nonstatic_field(Array<Array<u1>*>, _data, sizeof(Array<u1>*)) \
Fix alignment of the _data column.
src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/oops/Annotations.java line 74:
> 72: public U1Array getFieldAnnotations(int fieldIndex) {
> 73: Address addr = fieldsAnnotations.getValue(getAddress());
> 74: ArrayOfU1Array annotationsArray = VMObjectFactory.newObject(ArrayOfU1Array.class, addr);
How about caching this result so you don't need to allocate a new object every time this API is called. Same thing in `getFieldTypeAnnotations()`.
src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/oops/ConstMethod.java line 451:
> 449: offset += 1;
> 450: }
> 451: Address addr = getAddress().getAddressAt((getSize() - offset) * VM.getVM().getAddressSize());
A comment on the address computation would be useful here and in the changes below.
-------------
Changes requested by cjplummer (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/14735#pullrequestreview-1507639071
PR Review Comment: https://git.openjdk.org/jdk/pull/14735#discussion_r1248137517
PR Review Comment: https://git.openjdk.org/jdk/pull/14735#discussion_r1259190786
PR Review Comment: https://git.openjdk.org/jdk/pull/14735#discussion_r1259199965
More information about the serviceability-dev
mailing list