RFR: 8004823: Add VM support for type annotation reflection

[Joel Borggrén-Franck]

Hi Ioi,

Good catch!

I agree the checks should be added to all method_* annotation arrays.

Most of the classes loaded for Hello world should not only have a null 
type_annotations, but also a null annotations from instanceKlass. I don't 
think that will ever happen as is.

I can fix this but I doubt I have the time before M6. If you have the time 
please feel free to fix this :)

cheers
/Joel

On 01/09/2013 11:21 PM, Ioi Lam wrote:
> Hi Joel,
>
> I found a possible issue with the new type annotation code (changset
> 35431a769282). Should this extra NULL check be added?
>
> diff -r 36171820ba15 src/share/vm/classfile/classFileParser.cpp
> --- a/src/share/vm/classfile/classFileParser.cpp    Mon Jan 07 13:10:37 2013
> -0800
> +++ b/src/share/vm/classfile/classFileParser.cpp    Wed Jan 09 13:48:39 2013
> -0800
> @@ -2467,11 +2467,13 @@
> MetadataFactory::new_array<AnnotationArray*>(loader_data, length, NULL,
> CHECK_NULL);
>         }
>         (*methods_default_annotations)->at_put(index,
> method_default_annotations);
> *+    if (method_type_annotations != NULL) {**
> *       if (*methods_type_annotations == NULL) {
>           *methods_type_annotations =
> MetadataFactory::new_array<AnnotationArray*>(loader_data, length, NULL,
> CHECK_NULL);
>         }
>         (*methods_type_annotations)->at_put(index, method_type_annotations);
> *+    }**
> *     }
>
> I am testing a HelloWorld program and all loaded classes (all of them from
> rt.jar) have a non-zero Annotations::type_annotation() field (all added up
> to about 64KB of data for about 400 loaded classes).
>
> After my patch above, the type_annotations take up zero bytes.
>
> Actually, the more I look at this block of code, it seems like a similar
> check should be done for methods_annotations, methods_parameter_annotations
> and methods_default_annotations as well (in classFileParser.cpp, just above
> the code quoted above).
>
> If we can avoid the allocation of these annotations arrays, for Eclipse, we
> can save about 4.2MB of allocations (with about 9000 loaded classes) on
> Linux/x64.
>
> Thanks
> - Ioi
>
>
> On 12/27/2012 04:57 AM, Joel Borggrén-Franck wrote:
>> Thanks for the review John!
>>
>> I had this pushed before I saw this. I'll make sure to implement the checks when I start to implement retransform for  type annotations in the VM.
>>
>> cheers
>> /Joel
>>
>> On Dec 20, 2012, at 10:10 PM, John Rose<john.r.rose at oracle.com>  wrote:
>>
>>> This is good work.  You can count me as a reviewer.
>>>
>>> One comment:  There are a couple of places where a field index is used as an annotation array index (one old one new).  I think it would be best to put an explicit range check like this:
>>>
>>> -  if (md == NULL)
>>> +  if (md == NULL || index() >= md->length())
>>>      return NULL;
>>>    return md->at(index());
>>>
>>> There are two reasons:  First, although it is likely this is a needless check, the code which ensures length equality for field arrays and annotation arrays is buried in another file (classFileParser) and hard to check.  Second, recent changes to classFileParser actually decouple the field array length from the value declared in the class file, by injecting extra fields into certain system classes.  Although it is (again) unlikely that this will cause a problem, putting a local range check in the code above guarantees an adequate level of safety.
>>>
>>> The "at" method includes an assertion check, which is also good, but assertions are (a) not enabled in product mode and (b) can crash the VM if they fail.
>>>
>>> — John
>>>
>>> On Dec 18, 2012, at 9:05 AM, Joel Borggrén-Franck wrote:
>>>
>>>> New webrev up:
>>>>
>>>> http://cr.openjdk.java.net/~jfranck/8004823/webrev.v7/
>>>>
>>>> - I had to rename the field name for the type annotation byte[] in Field, Constructor and Method.
>>>>
>>>> Also fixed:
>>>>
>>>> - I fixed the mapfile that got reindented.
>>>> - Added back a comment that I accidentally removed.
>