RFR(M): 8057043: Type annotations not retained during class redefine / retransform
Andreas Eriksson
andreas.eriksson at oracle.com
Tue Oct 21 19:54:34 UTC 2014
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