RFR(M): 8057043: Type annotations not retained during class redefine / retransform
Andreas Eriksson
andreas.eriksson at oracle.com
Wed Oct 22 11:54:18 UTC 2014
Thanks Serguei!
Regards,
Andreas
On 2014-10-21 22:30, serguei.spitsyn at oracle.com wrote:
> Hi Andreas,
>
> Very nice, thank you for the refactoring!
> Thumbs up.
>
> Thanks,
> Serguei
>
>
> On 10/21/14 12:54 PM, Andreas Eriksson wrote:
>> Hi Serguei,
>>
>> I split up the method into several, and made the verification before
>> and after retransform share logic.
>> Webrev: http://cr.openjdk.java.net/~aeriksso/8057043/webrev.02/
>>
>> Regards,
>> Andreas
>>
>> On 2014-10-20 21:54, serguei.spitsyn at oracle.com wrote:
>>> Hi Andreas,
>>>
>>> Sorry for the delay.
>>>
>>> On 10/16/14 4:19 AM, Andreas Eriksson wrote:
>>>>
>>>> On 2014-10-15 15:47, Daniel D. Daugherty wrote:
>>>>> On 10/15/14 7:34 AM, Coleen Phillimore wrote:
>>>>>>
>>>>>> There are lots of other rewrite_cp_refs_in* function calls.
>>>>>> Please indent your function like them, not differently.
>>>>>
>>>>> The above implies that my answer below was made without sufficient
>>>>> context... my apologies for that.
>>>>>
>>>>> The general rule is to follow the existing style in the file so
>>>>> if there are rewrite_cp_refs_in* function calls in the file, then
>>>>> please follow that style. Unless, of course, you want to fix all
>>>>> of them to follow the HotSpot style guideline:
>>>>>
>>>>> https://wiki.openjdk.java.net/display/HotSpot/StyleGuide
>>>>>
>>>>> > Use good taste to break lines and align corresponding tokens
>>>>> > on adjacent lines.
>>>>>
>>>>> but that may cause Coleen some heartburn :-)
>>>>
>>>> I fixed the calls to follow the already existing indent style.
>>>> I have also made changes to the test, which I hope Joel can take a
>>>> look at.
>>>>
>>>> New webrev:
>>>> http://cr.openjdk.java.net/~aeriksso/8057043/webrev.01/
>>>
>>> The fix looks good.
>>>
>>> A couple of comments about the test.
>>>
>>> The method testTransformAndVerify() is too big.
>>> At least, it looks like there are some ways to refactor it to make
>>> calls to smaller methods.
>>>
>>> There are two directions of doing it:
>>> - make a smaller method out of each block:
>>> 217-236, 238-260, 262-276, 311-329, 331-351, 353-367
>>> - some of the lines sequences looks very typical:
>>> 221 at = c.getDeclaredField("typeAnnotatedArray").getAnnotatedType();
>>> 222 arrayTA1 = at.getAnnotations()[0];
>>> 223 verifyTestAnnSite(arrayTA1, "array1");
>>> 224
>>> 225 at = ((AnnotatedArrayType) at).getAnnotatedGenericComponentType();
>>> 226 arrayTA2 = at.getAnnotations()[0];
>>> 227 verifyTestAnnSite(arrayTA2, "array2");
>>> 228
>>> 229 at = ((AnnotatedArrayType) at).getAnnotatedGenericComponentType();
>>> 230 arrayTA3 = at.getAnnotations()[0];
>>> 231 verifyTestAnnSite(arrayTA3, "array3");
>>> 232
>>> 233 at = ((AnnotatedArrayType) at).getAnnotatedGenericComponentType();
>>> 234 arrayTA4 = at.getAnnotations()[0];
>>> 235 verifyTestAnnSite(arrayTA4, "array4");
>>> But I leave it up to you.
>>>
>>> Another step to improve the readability is to add a short comment
>>> for each block of code saying what is done there.
>>>
>>> Thanks,
>>> Serguei
>>>
>>>
>>>>
>>>> Thanks,
>>>> Andreas
>>>>
>>>>>
>>>>> Dan
>>>>>
>>>>>
>>>>>>
>>>>>> Coleen
>>>>>>
>>>>>> On 10/15/14, 9: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)) {
>>>>>>>
>>>>>>> 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