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 17:04:42 UTC 2014


Or do I need to send out a real review for the backport?
I'm not sure what the process is here.

Thanks,
Andreas

On 2014-11-04 17:57, Andreas Eriksson wrote:
> 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