RFR(M): 8057043: Type annotations not retained during class redefine / retransform
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Wed Oct 15 14:50:08 UTC 2014
On 10/15/14 7:08 AM, Andreas Eriksson wrote:
>
> On 2014-10-15 16:03, serguei.spitsyn at oracle.com wrote:
>> On 10/15/14 6:05 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)) {
>>>>
>>>> How should I indent them?
>>>
>>> Trying again without the line numbers...
>>>
>>> if
>>> (!rewrite_cp_refs_in_type_annotations_typeArray(method_type_annotations,
>>>
>>> byte_i, "method_info",
>>> THREAD)) {
>>
>> Agreed. This is even better.
>
> Alright, I'll go with this style then.
> Thanks Serguei and Daniel.
Sorry, Andreas. My suggestion was wrong.
I've replied automatically and overlooked that fact the function
arguments are being indented.
Forgot to drink enough coffee in the morning. :)
Dan's suggestion is right.
Thanks,
Serguei
>
> /Andreas
>
>>
>> Thanks,
>> Serguei
>>
>>>
>>> Just in case, TB messes with the spacing again, the "byte_i" line and
>>> "THREAD" lines are aligned under "method_type_annotations".
>>>
>>> Dan
>>>
>>>
>>>>
>>>> /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
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
More information about the hotspot-dev
mailing list