Review request (M): 8003720: NPG: Method in interpreter stack frame can be deallocated

[Stefan Karlsson]

David,

Thanks for taking a look at this.

I've added a comment regarding the cld_f:

     // GC support
     // Apply "f->do_oop" to all root oops in "this".
*+   // Apply "cld_f->do_cld" to CLDs that are otherwise not kept alive.*
*+   //   Used by JavaThread::oops_do.*
     // Apply "cf->do_code_blob" (if !NULL) to all code blobs active in frames
*!    virtual void oops_do(OopClosure* f,_CLDToOopClosure* cld_f,_CodeBlobClosure* cf);*

http://cr.openjdk.java.net/~stefank/8003720/webrev.01

thanks,
StefanK

On 11/23/2012 01:01 AM, David Holmes wrote:
> Hi Stefan,
>
> Not my area so a minor comment not a review :)
>
> thread.hpp
>
>  481   // GC support
>  482   // Apply "f->do_oop" to all root oops in "this".
>  483   // Apply "cf->do_code_blob" (if !NULL) to all code blobs active 
> in frames
>  484   virtual void oops_do(OopClosure* f, CLDToOopClosure* cld_f, 
> CodeBlobClosure* cf);
>
> can you add a comment that cld_f is not used/needed by Thread::oops_do
>
> Thanks,
> David
>
> On 23/11/2012 12:14 AM, Stefan Karlsson wrote:
>> http://cr.openjdk.java.net/~stefank/8003720/webrev/
>>
>> Description from CR:
>> In virtual calls the Method pointer in the interpreter stack frame is
>> not kept alive by anything other than the "this" pointers to that
>> method. If bytecodes overwrite the "this" pointer, then call a full GC,
>> the class loader containing the Method* can be unloaded and the Method*
>> deallocated.
>>
>> This is also a problem with JSR292 MethodHandle static code because the
>> MethodHandle containing the mirror for the interpreted method Method* is
>> not on the stack if a GC occurs.
>>
>> Fix proposal:
>> The "obvious" solution to this problem would be to apply the root
>> scanning OopClosure to the Klass::_java_mirror field of the method in
>> the interpreted frame. However, doing this might cause us to scan the
>> same metadata oop location more than once, which is not allowed by some
>> of the HotSpot GCs. We currently solve similar situations by always
>> "claiming" and start scanning from the ClassLoaderData and then proceed
>> down into the Klasses of that class loader.
>>
>> For this bug we do the same. All old collections, where class unloading
>> can occur, pass down a closure that is applied to the ClassLoaderData of
>> the Klass of the Method in the interpreted frame. This closure does the
>> claiming and proceeds to scan the class metadata. Note that during young
>> collections, where we don't do class unloading, all classes are already
>> used as strong roots and we don't have to apply this new closure in the
>> interpreted frame.
>>
>> Testing:
>> The added test was initially written by John Rose. I only ported it to
>> JTreg and made some artistic cleanups to it.
>>
>> thanks,
>> StefanK