On Fri, 30 Jun 2023 14:32:03 GMT, Ashutosh Mehra <d...@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
