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

[Andreas Eriksson]

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?

/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
>>>
>>
>