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

[Coleen Phillimore]

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
>>
>