Review request (L) 8003419: NPG: Clean up metadata created during class loading if failure

[Coleen Phillmore]

Hi Jon,

Thank you for starting to look at this.   The huge number of diffs in 
classFileParser.cpp is because I moved the code that laid out fields and 
diff matches oddly so it looks like I changed a lot more than I did.   I 
should have done this in the first place, but here are the changes I 
made without moving layout_fields.   I'll move it back (and remove the 
fac and parsed_allocations copying) after the review.

http://cr.openjdk.java.net/~coleenp/8003419_small/

This is much easier to read!

On 3/8/2013 7:19 PM, Jon Masamitsu wrote:
> Coleen,
>
> http://cr.openjdk.java.net/~coleenp/8003419/src/share/vm/classfile/classFileParser.hpp.udiff.html 
>
>
> *+    // metadata created before the instance klass is created. Must be*
> *+   // deallocated if classfile parsing returns an error.*
>
> Could this also have said
>
> "Must be deallocated if not transferred to the InstanceKlass upon 
> sucessful class loading
> in which case these pointers have been set to NULL."

I changed the comment (adding a 'c' to it) to yours.
>
> At first I didn't understand why the destructor didn't always 
> deallocate the
> metadata.
>
> Can all the fields with pointers to metadata be grouped together
> and in the destructor iterate through the pointers to see that they are
> all NULL?  Worth doing to catch leaks?

The deallocation uses template functions for the types of each pointer 
so iterating through pointers won't work.
>
>
> This file just had so many changes in it I quickly bogged down. I'll
> try again later.  So far only
>
> http://cr.openjdk.java.net/~coleenp/8003419/src/share/vm/classfile/classFileParser.cpp.udiff.html 
>
>
>   2991    // Assign allocations if needed
>
> allocations or annotations?
>

Annotations, fixed.
> Can you make this unsigned?
>
> 3288       int map_count = _super_klass->nonstatic_oop_map_count();
>

Yes, I changed it.  The field nonstatic_oop_map_count is unsigned.

Thanks!
Coleen

>
>
> Jon
>
>
> On 3/5/2013 11:30 AM, Coleen Phillimore wrote:
>>
>> Resend to hotspot-dev.   I don't think anyone reads 
>> hotspot-runtime-dev anymore.
>> Please review!!
>> Thanks,
>> Coleen
>>
>> On 03/04/2013 03:13 PM, Coleen Phillimore wrote:
>>> Summary: Store metadata on ClassFileParser instance to be cleaned up 
>>> by destructor.  This enabled some refactoring of the enormous 
>>> parseClassFile function.
>>>
>>> Note: I moved out code that lays out the java object into it's own 
>>> function, but the diffs made it look like I made more than minor 
>>> changes.  The changes I made was changing class_name to _class_name 
>>> and cp to _cp, and not more than that.   To see these changes, 
>>> you'll have to look at before/after code and not the diffs.  I also 
>>> tried moving this layout_fields() function around to it wouldn't 
>>> false match but I wanted to keep it next to parseClassFile() and not 
>>> move it too far away.
>>>
>>> Tested with runThese jck, jck8 lang and vm, NSK vm.quick.testlist, 
>>> java/lang/invoke jtreg tests and java/lang/instrument jtreg tests.
>>>
>>> open webrev at http://cr.openjdk.java.net/~coleenp/8003419/
>>> bug link at http://bugs.sun.com/view_bug.do?bug_id=8003419
>>>
>>> Thanks,
>>> Coleen
>>>
>>
>