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

[serguei.spitsyn at oracle.com]

Hi Andreas,

Sorry I did not reply on this early.
I assumed, it is a thumbs up from me.
Just wanted make it clean now. :)

Thanks,
Serguei

On 10/13/14 3:09 AM, Andreas Eriksson wrote:
> 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
>>
>