FYI: Jdk8 backport, Was: Re: RFR(M): 8057043: Type annotations not retained during class redefine / retransform
Andreas Eriksson
andreas.eriksson at oracle.com
Tue Nov 4 16:57:22 UTC 2014
Hi,
Just wanted to let the list know that I'm about to backport this change
to jdk8.
Regards,
Andreas
On 2014-10-22 13:54, Andreas Eriksson wrote:
> 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