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

[Daniel D. Daugherty]

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

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