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

[serguei.spitsyn at oracle.com]

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