RFR(M): 8057043: Type annotations not retained during class redefine / retransform
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Tue Oct 21 20:30:12 UTC 2014
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