FYI: Jdk8 backport, Was: Re: RFR(M): 8057043: Type annotations not retained during class redefine / retransform
Andreas Eriksson
andreas.eriksson at oracle.com
Wed Nov 5 10:27:30 UTC 2014
On 2014-11-05 01:15, David Holmes wrote:
> On 5/11/2014 9:37 AM, serguei.spitsyn at oracle.com wrote:
>> Hi Andreas,
>>
>> If the port is straightforward then referring to the jdk 9 reviewers
>> should be enough.
>> You still need to get an approval.
>
> You don't need an approval in sense of requesting approval from
> jdk8u-dev at ojn. You need a RFR of the backport on the hotspot lists
> before pushing to jdk8u/hs-dev. Alejandro will then request a bulk
> approval (on jdk8u-dev at ojn) when hs-dev syncs up.
>
> Andreas: similar to approval requests the RFR should cover whether the
> changesets were imported directly without change, or whether
> modifications were needed. For the former include a link to the 9
> changesets; for the latter a link to the 8u webrev.
Alright, thanks.
/Andreas
>
> David
>
>> 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