On Wed, 13 Mar 2024 01:02:50 GMT, Alex Menkov <amen...@openjdk.org> wrote:
>> RecordComponent class has _attributes_count field.
>> The only user of the field is JvmtiClassFileReconstituter. Incorrect value
>> of the field causes producing incorrect data for Record attribute.
>> Parsing Record attribute ClassFileParser skips unknown attributes and may
>> skip RuntimeInvisibleAnnotations/RuntimeInvisibleTypeAnnotations.
>> Also annotations can be changed (added/removed) by class redefinition.
>> The fix removes attributes_count from RecordComponent;
>> JvmtiClassFileReconstituter calculates correct attributes_count generating
>> class bytes.
>>
>> Testing:
>> - tier1,tier2,hs-tier5-svc;
>> - redefineClasses/retransformClasses tests:
>> - test/jdk/java/lang/instrument
>> - test/hotspot/jtreg/serviceability/jvmti/RedefineClasses
>> - test/hotspot/jtreg/vmTestbase/nsk/jvmti/RedefineClasses
>> - test/hotspot/jtreg/vmTestbase/nsk/jvmti/RetransformClasses
>
> Alex Menkov has updated the pull request incrementally with one additional
> commit since the last revision:
>
> removed attributes_count from RecordComponent
This looks good in general.
I've posted a couple of minor requests.
Aside of the fix itself...
It is interesting that if an invisible attribute of a `RecordComponent` was not
ignored because the `PreserveAllAnnotations` is enabled then it the
`JvmtiClassFileReconstituter` treats it as a visible attribute. Not sure, if we
should treat it as a bug.
Marked as reviewed by sspitsyn (Reviewer).
src/hotspot/share/prims/jvmtiClassFileReconstituter.cpp line 516:
> 514: + component->annotations() != nullptr ? 1 : 0
> 515: + component->type_annotations() != nullptr ? 1 :
> 0;
> 516: write_u2(attributes_count);
Nit: I would suggest to define this function in the `RecordComponent` class:
u2 attributes_count() const {
u2 attributes_count = generic_signature_index() != 0 ? 1 : 0
+ annotations() != nullptr ? 1 : 0
+ type_annotations() != nullptr ? 1 : 0;
return attributes_count;
}
test/jdk/java/lang/instrument/RetransformRecordAnnotation.java line 96:
> 94: {
> 95: log("Test: retransform to null");
> 96: retransform(null);
Q: Could you add a comment why it is needed?
-------------
Marked as reviewed by sspitsyn (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/18161#pullrequestreview-1933129468
PR Review: https://git.openjdk.org/jdk/pull/18161#pullrequestreview-1933147886
PR Review Comment: https://git.openjdk.org/jdk/pull/18161#discussion_r1522481272
PR Review Comment: https://git.openjdk.org/jdk/pull/18161#discussion_r1522494266
