RFR(M): 8057043: Type annotations not retained during class redefine / retransform

[Andreas Eriksson]

Hi Serguei, thanks for looking at this!

I'll make sure to fix the style problems.
For the symbolic names / #defines, please see my answer to Coleen.

Regards,
Andreas

On 2014-10-11 12:37, serguei.spitsyn at oracle.com wrote:
> Hi Andreas,
>
>
> Thank you for fixing this issue!
> The fix looks nice, I do not see any logical issues.
>
>
> Only minor comments...
>
>
> src/share/vm/prims/jvmtiRedefineClasses.cpp
>
> 2281 } // end rewrite_cp_refs_in_class_type_annotations(
> 2315 } // end rewrite_cp_refs_in_fields_type_annotations(
> 2345 } // end rewrite_cp_refs_in_methods_type_annotations()
> 2397 } // end rewrite_cp_refs_in_type_annotations_typeArray
> 2443 } // end rewrite_cp_refs_in_type_annotation_struct
> 2785 } // end skip_type_annotation_target
> 2844 } // end skip_type_annotation_type_path
>
> The ')' is missed at 2281, 2315.
> The 2397-2844 are inconsistent with the 2345 and other function-end 
> comments in the file.
>
>
> 2335     if (!rewrite_cp_refs_in_type_annotations_typeArray(method_type_annotations,
> 2336           byte_i, "method_info", THREAD)) {
> . . .
> 2378     if (!rewrite_cp_refs_in_type_annotation_struct(type_annotations_typeArray,
> 2379            byte_i_ref, location_mesg, THREAD)) {
> . . .
> 2427   if (!skip_type_annotation_target(type_annotations_typeArray,
> 2428         byte_i_ref, location_mesg, THREAD)) {
> 2429     return false;
> 2430   }
> 2431
> 2432   if (!skip_type_annotation_type_path(type_annotations_typeArray,
> 2433         byte_i_ref, THREAD)) {
> 2434     return false;
> 2435   }
> 2436
> 2437   if (!rewrite_cp_refs_in_annotation_struct(type_annotations_typeArray,
> 2438          byte_i_ref, THREAD)) {
> 2439     return false;
> Wrong indent at 2336, 2379, etc.
>
>
> I also concur with Coleen that it would be good to define and use
> symbolic names for the hexa-decimal constants used in the fix.
>
>
>
> test/runtime/RedefineTests/RedefineAnnotations.java
>
>   Java indent must be 4, not 2.
>
>   253       @TestAnn(site="returnTypeAnnotation") Class typeAnnotatedMethod(@TestAnn(site="formalParameterTypeAnnotation") TypeAnnotatedTestClass arg)
>
>  The line is too long.
>
>
>   143   }
>   144   public static void main(String argv[]) {
>   . . .
>   209   }
>   210   private static void checkAnnotations(AnnotatedType p) {
>   211     checkAnnotations(p.getAnnotations());
>   212   }
>   213   private static void checkAnnotations(AnnotatedType[] annoTypes) {
>   214     for (AnnotatedType p : annoTypes) checkAnnotations(p.getAnnotations());
>   215   }
>   216   private static void checkAnnotations(Class<TypeAnnotatedTestClass> c) {
>   . . .
>   257       }
>   258       public void run() {}
>
>    Adding empty lines between method definitions would improve 
> readability.
>
>
> Thanks,
> Serguei
>
>
> On 10/9/14 6:21 AM, Andreas Eriksson wrote:
>> Hi all,
>>
>> Please review this patch to RedefineClasses to allow type annotations 
>> to be preserved.
>>
>> Summary:
>> During redefine / retransform class the constant pool indexes can 
>> change.
>> Since annotations have indexes into the constant pool these indexes 
>> need to be rewritten.
>> This is already done for regular annotations, but not for type 
>> annotations.
>> This patch adds code to add this rewriting for the type annotations 
>> as well.
>> The patch also contains minor changes to ClassFileReconstituter, to 
>> make sure that type annotations are preserved during a redefine / 
>> retransform class operation.
>> It also has a test that uses asm to change constant pool indexes 
>> through a retransform, and then verifies that type annotations are 
>> preserved.
>>
>> Detail:
>> A type annotation struct consists of some target information and a 
>> type path, followed by a regular annotation struct.
>> Constant pool indexes are only present in the regular annotation struct.
>> The added code skips over the type annotation specific parts, then 
>> calls previously existing code to rewrite constant pool indexes in 
>> the regular annotation struct.
>> Please see the Java SE 8 Ed. VM Spec. section 4.7.20 for more info 
>> about the type annotation struct.
>>
>> JPRT with the new test passes without failures on all platforms.
>>
>> Webrev:
>> http://cr.openjdk.java.net/~aeriksso/8057043/webrev.00/
>>
>> Bug:
>> https://bugs.openjdk.java.net/browse/JDK-8057043
>>
>> Regards
>> Andreas
>