On Fri, 29 Mar 2024 07:08:58 GMT, Serguei Spitsyn <sspit...@openjdk.org> wrote:
> > Correct solution would be to store additional information about
> > RuntimeInvisibleAnnotations and restore them exactly as they were in the
> > original class (this should be done for all annotations:
> > RuntimeInvisibleAnnotations/RuntimeInvisibleTypeAnnotations for class,
> > fields and records,
> > RuntimeInvisibleAnnotations/RuntimeInvisibleTypeAnnotations/RuntimeInvisibleParameterAnnotations
> > for methods; need to ensure the information is correctly updated during
> > class redefinition & retransformation).
> > I think it doesn't make sense to add all the complexity for almost no value
> > (I doubt anyone uses PreserveAllAnnotations, the flag looks like
> > experimental, we don't have any tests for it).
>
> This solution looks okay in general but still is a little bit confusing. It
> feels like saving just one bit would not add much complexity but would even
> simplify the implementation as it will become straight-forward. At least,
> there would be no need in this additional function with the
> `fallback_attr_name` parameter.
It's a bit more complicated than storing 1 bit.
Lets consider ClassFileParser::parse_classfile_record_attribute as an example.
There are 2 type of annotations for records: RuntimeVisibleAnnotations and
RuntimeVisibleTypeAnnotations.
For each type if PreserveAllAnnotations is set ClassFileParser additionally
handles invisible annotations
(RuntimeInvisibleAnnotations/RuntimeInvisibleTypeAnnotations).
Then it "assembles" annotations (concatenates visible and invisible annotations
into single array).
To restore original annotations we need to keep number of visible annotations
in the annotation array (so jvmtiClassReconstituter writes part of the
annotation array as visible, and the rest as invisible).
And we need to store this number for each annotation type (Annotations and
TypeAnnotations for records, Annotations and TypeAnnotations for class,
Annotations and TypeAnnotations for fields,
Annotations/TypeAnnotations/ParameterAnnotations for methods).
I considered another way to implement the workaround - add method
`bool is_symbol_in_cp(const char* symbol)`
But then each writing would become very verbose and more confusing (needs some
explanation in each place):
if (is_symbol_in_cp("RuntimeVisibleTypeAnnotations")) {
write_annotations_attribute("RuntimeVisibleTypeAnnotations",
component->type_annotations());
} else {
write_annotations_attribute("RuntimeInvisibleTypeAnnotations",
component->type_annotations());
}
-------------
PR Comment: https://git.openjdk.org/jdk/pull/18540#issuecomment-2027647051
