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