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

[serguei.spitsyn at oracle.com]

On 10/15/14 6:03 AM, Daniel D. Daugherty wrote:
>
> On 10/15/14 7:02 AM, Daniel D. Daugherty wrote:
>> On 10/15/14 5: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)) {
>>
>> The above should be indented like this:
>>
>> 2335     if 
>> (!rewrite_cp_refs_in_type_annotations_typeArray(method_type_annotations,
>> 2335 byte_i, "method_info",
>> 2336 THREAD)) {
>
> Wow. Thunderbird ate all the white space indenting that I did...

Funny. :)

Thanks,
Serguei

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