RFR(M): 8057043: Type annotations not retained during class redefine / retransform
Andreas Eriksson
andreas.eriksson at oracle.com
Wed Oct 15 16:01:55 UTC 2014
On 2014-10-15 16:50, serguei.spitsyn at oracle.com wrote:
> 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.
No problem. :)
>
>
> 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