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

[serguei.spitsyn at oracle.com]

Hi Andreas,

On 10/15/14 4:22 AM, Andreas Eriksson wrote:
> Thanks Serguei.
>
> I have a question about the if-blocks that had the wrong indent:
>
> 2335     if (!rewrite_cp_refs_in_type_annotations_typeArray(method_type_annotations,
> 2336           byte_i, "method_info", THREAD)) {
> How should I indent them?

Normal indent is:

2335     if (!rewrite_cp_refs_in_type_annotations_typeArray(method_type_annotations,
2336         byte_i, "method_info", THREAD)) {


Thanks,
Serguei


>
> /Andreas
>
> On 2014-10-15 07:00, serguei.spitsyn at oracle.com wrote:
>> 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
>>>>
>>>
>>
>