RFR(M): 8057043: Type annotations not retained during class redefine / retransform

[Andreas Eriksson]

Great, thanks!

/Andreas

On 2014-10-14 15:45, Coleen Phillimore wrote:
>
> Andreas,  This is ok about the case numbers then.  I agree it's not 
> worth creating new enum type for them since they are all commented. 
> (sorry for top-posting but this is my only comment).
>
> You have my code review.  Thank you for adding this.
>
> Coleen
>
> On 10/13/14, 6:03 AM, Andreas Eriksson wrote:
>> Hi Coleen, thanks for looking at this!
>> See answers inline.
>>
>> On 2014-10-10 16:48, Coleen Phillimore wrote:
>>>
>>> Andreas,
>>> This is excellent work!  It looks really good.  Some comments.
>>>
>>> http://cr.openjdk.java.net/~aeriksso/8057043/webrev.00/src/share/vm/prims/jvmtiRedefineClasses.cpp.udiff.html 
>>>
>>>
>>> +  u1 target_type = type_annotations_typeArray->at(byte_i_ref);
>>> +  byte_i_ref += 1;
>>> +  RC_TRACE_WITH_THREAD(0x02000000, THREAD, ("target_type=0x%.2x", 
>>> target_type));
>>> +  RC_TRACE_WITH_THREAD(0x02000000, THREAD, ("location=%s", 
>>> location_mesg));
>>> +
>>> +  // Skip over target_info
>>> +  switch (target_type) {
>>> +    case 0x00:
>>> +    // kind: type parameter declaration of generic class or interface
>>> +    // location: ClassFile
>>> +    case 0x01:
>>> +    // kind: type parameter declaration of generic method or 
>>> constructor
>>> +    // location: method_info
>>>
>>> Are there any #defines that make these numbers into some symbolic 
>>> values?
>>>
>>> These comments are good in case there aren't.
>>>
>>
>> Unfortunately, there are no #defines for these numbers that I could 
>> find.
>> I don't think creating symbolic names is worth it either, since the 
>> only unique way they are referred to in the VM spec is this verbose 
>> 'kind' field that I have included in the comments.
>> Location combined with target_info is unique for 0x00 to 0x17, but 
>> not for 0x40 and up. One could maybe create a define by combining 
>> location name, target_info name and parts of the kind text, but I'm 
>> not sure this would increase readability. I think having these 
>> comments is better.
>>
>>> + case 0x17:
>>> +    // kind: type in throws clause of method or constructor
>>> +    // location: method_info
>>>
>>>
>>> I can't find anything wrong.  I don't really understand the test 
>>> that well so hopefully someone that knows type annotations can look 
>>> at it also.   Maybe include lang tools and Joel and Eric?
>>>
>>
>> Joel has been looking at this review, and he gave me some feedback on 
>> the test offline. I'll rewrite parts of it and send out a new review 
>> when I'm done.
>>
>> Thanks,
>> Andreas
>>
>>> This code is needed because we don't replace the InstanceKlass when 
>>> redefining classes and rewrite the constant pool.  If we were able 
>>> to replace the InstanceKlass instead, we wouldn't need to do all 
>>> this work. We're evaluating this at the moment, but have no real 
>>> progress, so thank you for fixing this!
>>>
>>> Thanks,
>>> Coleen
>>>
>>> On 10/9/14, 9: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
>>>
>>
>