FYI: Jdk8 backport, Was: Re: RFR(M): 8057043: Type annotations not retained during class redefine / retransform
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Tue Nov 4 23:37:52 UTC 2014
Hi Andreas,
If the port is straightforward then referring to the jdk 9 reviewers
should be enough.
You still need to get an approval.
Thanks,
Serguei
On 11/4/14 9:04 AM, Andreas Eriksson wrote:
> 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