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

[Christian Thalinger]

On Nov 23, 2012, at 12:19 AM, Stefan Karlsson <stefan.karlsson at oracle.com> wrote:

> 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

test/runtime/8003720/Asmator.java:

Two comments here:

1. The copyright header is missing
2. The ASM import should be updated as soon the new ASM package has hit our repositories (I track the 292 files with 8001885)

Otherwise this looks good.

-- Chris

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