FYI: Jdk8 backport, Was: Re: RFR(M): 8057043: Type annotations not retained during class redefine / retransform
David Holmes
david.holmes at oracle.com
Wed Nov 5 00:15:36 UTC 2014
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.
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