RFR: JDK-8328137: PreserveAllAnnotations can cause failure of class retransformation

[Alex Menkov]

On Fri, 29 Mar 2024 07:08:58 GMT, Serguei Spitsyn <sspitsyn at 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