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

[Coleen Phillimore]

On Thu, 28 Mar 2024 22:12:49 GMT, Alex Menkov <amenkov at openjdk.org> wrote:

> PreserveAllAnnotations option causes class file parser to preserve RuntimeInvisibleAnnotations so VM considers them as RuntimeVisibleAnnotations.
> For class retransformation JvmtiClassFileReconstituter restores all annotations as RuntimeVisibleAnnotations attributes.
> This can cause problem is the class contains only RuntimeInvisibleAnnotations, so corresponding RuntimeVisibleAnnotations attribute name is not present in the class constant pool.
> 
> 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).
> 
> The suggested fix adds workaround for this corner case - if "visible" attribute name is not in the CP, the annotations are restored with "invisible" attribute name.
> 
> Testing:
>   - tier1,tier2,hs-tier5-svc
>   - all java/lang/instrument tests;
>   - all RedefineClasses/RetransformClasses tests

At one point long ago, I was trying to understand why we have PreserveAllAnnotations and couldn't come up with a reason.  For a class file reconstitutor, restoring the invisible annotations to the classfile and then feeding it back to the JVM should have no effect, since the VM doesn't do anything with these annotations.

I see now why you get the original assert.   I think this looks like a reasonable workaround for this problem.

I wonder if we can deprecate PreserveAllAnnotations.  Wonder what it's for?  It would need a CSR because it's a product flag.

-------------

Marked as reviewed by coleenp (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/18540#pullrequestreview-1974754738