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