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

[Andreas Eriksson]

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
>