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. Hi @ashu-mehra, looks mostly good. Some nits inline. I tried it out and it works fine. What would be needed to make the Annotations appear in the "printall" command? I was somehow expecting to see at least something like "Annotation@xxxx". Cheers, Thomas src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/oops/ConstMethod.java line 470: > 468: if (hasParameterAnnotations()) { > 469: offset += 1; > 470: } Code here and in other places could be tightened: int offset = (hasMethodAnnotations() ? 1 : 0) + (hasParameterAnnotations() ? 1 : 0) + (hasTypeAnnotations() ? 1 : 0); src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/oops/ConstMethod.java line 494: > 492: offset += 1; > 493: } > 494: Address addr = getAddress().getAddressAt((getSize() - offset) * > VM.getVM().getAddressSize()); Factor this address calculation out, and as @plummercj remarked, comment it? src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/oops/InstanceKlass.java line 874: > 872: return null; > 873: } > 874: } Would calling these functions even be valid to call if Annotations are not present? If yes, maybe return Optional? But since the rest of the code does not use Optional either, maybe rather keep the code the same. Up to you, feel free to ignore this. ------------- PR Review: https://git.openjdk.org/jdk/pull/14735#pullrequestreview-1523513001 PR Review Comment: https://git.openjdk.org/jdk/pull/14735#discussion_r1259248538 PR Review Comment: https://git.openjdk.org/jdk/pull/14735#discussion_r1259249446 PR Review Comment: https://git.openjdk.org/jdk/pull/14735#discussion_r1259259800
