RFR 8028497: SIGSEGV at ClassLoaderData::oops_do(OopClosure*, KlassClosure*, bool)

[Ioi Lam]

Coleen,

Why is this change necessary for this bug anyway?

http://cr.openjdk.java.net/~coleenp/8028497_2/src/share/vm/classfile/classFileParser.cpp.sdiff.html

ClassFileParser is not used when loading a class from CDS archive.

- Ioi

On 3/27/14, 10:11 AM, Erik Helin wrote:
> Hi Coleen,
>
> thanks for taking on this tricky bug! However, I think there is an 
> issue with your fix :(
>
> This case is for "normal" (non-CDS) class loading:
> 1. We create the mirror
> 2. We set up the link from the mirror to the Klass
> 3. We fail somewhere before setting the link from the Klass to the
>    mirror, for example when allocating the Java object for the init
>    lock.
> 4. The destructor for ClassFileParser is run and we add the Klass to the
>    deallocate list.
> 5. A CMS concurrent cycle is started, it will treat all objects in eden
>    + the survivor area as roots (including the mirror).
> 6. The above concurrent cycle enters the "final remark" phase and does
>    class unloading which will call free_deallocate_list() which will
>    return the memory for the Klass added to the deallocate list in step
>    4 to Metaspace.
> 7. The concurrent CMS cycle ends (the mirror is still in eden or young)
> 8. A new concurrent CMS cycle is started, the mirror will once again be
>    treated as a root since it is still in eden or young (no YC has been
>    done).
> 9. During the initial marking, CMS will now follow the pointer to the
>    Klass that the mirror is mirroring, but we returned this memory to
>    Metaspace in step 6.
> 10. We now crash :(
>
> Unfortunately, I don't have any suggestion on how to solve the above 
> scenario at the moment.
>
> Thanks,
> Erik
>
> On 2014-03-27 16:04, Coleen Phillimore wrote:
>>
>> On 3/27/14 8:39 AM, Stefan Karlsson wrote:
>>>
>>> On 2014-03-27 03:00, Coleen Phillimore wrote:
>>>>
>>>> I have a new version of this change based on code that Ioi noticed
>>>> that I was missing for making restoring the method's adapters
>>>> restartable if something fails during restore_unshareable_info, and
>>>> the constant pool resolved references array.
>>>> Please review this new code (only method.* and instanceKlass.cpp and
>>>> constantPool.cpp have differences).  Reran tests with CDS and CMS.
>>>>
>>>> http://cr.openjdk.java.net/~coleenp/8028497_2/
>>>
>>> This seems good. I'd prefer if someone with more experience with CDS
>>> also Reviews the change.
>>
>> Thanks, Ioi said he'd look at it.
>>
>>>
>>> Small comment:
>>>
>>> http://cr.openjdk.java.net/~coleenp/8028497_2/src/share/vm/oops/instanceKlass.cpp.udiff.html 
>>>
>>>
>>>
>>> -    methodHandle m(THREAD, methods->at(index2));
>>> -    m()->link_method(m, CHECK);
>>> -    // restore method's vtable by calling a virtual function
>>> -    m->restore_vtable();
>>> +    Method *m = methods->at(index2);
>>> +    m->restore_unshareable_info(CHECK);
>>>
>>>
>>> Do we really want to use a raw Method * here. I know that we wrap the
>>> method in a handle inside  m->restore_unshareable_info, but it's not
>>> clear from the this context. I'm thinking about this issue:
>>> https://bugs.openjdk.java.net/browse/JDK-8004815
>>>
>>> On the other hand, I guess we can't redefine a class before it has
>>> been defined. But maybe it would be good to not promote the usage of
>>> Method* over calls that might hit a Safepoint.
>>>
>>
>> I think you're right, we don't use 'm' after the potential safepoint and
>> we can't really redefine this class yet anyway but I'll restore the code
>> because we don't want to encourage uses of unhandled Methods unless
>> proven to be harmless.
>>
>> Thanks!
>> Coleen
>>> thanks,
>>> StefanK
>>>
>>>>
>>>> Thanks,
>>>> Coleen
>>>>
>>>> On 3/26/14 5:15 PM, Coleen Phillimore wrote:
>>>>>
>>>>> Hi Jon,
>>>>> Thank you for looking at the code changes.
>>>>>
>>>>> On 3/26/14 12:24 PM, Jon Masamitsu wrote:
>>>>>> Coleen,
>>>>>>
>>>>>> Did you remove Class_obj_allocate() because it was only
>>>>>> called in that one spot and  use obj_allocate() did most
>>>>>> of the work?
>>>>>
>>>>> I deleted it because GC doesn't need to know how these javaClasses
>>>>> are set up and I spent way to long looking for this code and didn't
>>>>> find it where it belongs, so I removed this. This Class_obj_allocate
>>>>> is an artifact of the permgen code.
>>>>>
>>>>>>
>>>>>> http://cr.openjdk.java.net/~coleenp/8028497/src/share/vm/oops/instanceMirrorKlass.cpp.frames.html 
>>>>>>
>>>>>>
>>>>>>
>>>>>>  373   // the size in the mirror size itself.
>>>>>>
>>>>>> Drop the second "size".  You know how we GC guys are
>>>>>> getting with our comments :-).
>>>>>
>>>>> Yes, I noticed that.  Thanks - fixed.
>>>>>>
>>>>>> http://cr.openjdk.java.net/~coleenp/8028497/src/share/vm/oops/klass.cpp.frames.html 
>>>>>>
>>>>>>
>>>>>>
>>>>>>  498 void Klass::restore_unshareable_info(TRAPS) {
>>>>>>  499   // If an exception happened during CDS restore, some of
>>>>>> these fields may already be
>>>>>>  500   // set.  We leave the class on the CLD list, even if
>>>>>> incomplete so that we don't
>>>>>>  501   // modify the CLD list outside a safepoint.
>>>>>>  502   if (class_loader_data() == NULL) {
>>>>>>  503     ClassLoaderData* loader_data =
>>>>>> ClassLoaderData::the_null_class_loader_data();
>>>>>>
>>>>>> I can see that this works with the current CDS (sharing only
>>>>>> classes loader
>>>>>> with the NULL class loader).  Will it work if the shared classes
>>>>>> are loaded
>>>>>> with an application class loader?
>>>>>
>>>>> I hope Ioi can answer that.  I assume so because the classes don't
>>>>> change class loaders if restoring shareable info fails.
>>>>>
>>>>> Ioi pointed out to me (not on open list) that I missed making
>>>>> link_method() and creating the constant pool resolved references
>>>>> array restartable, so I'm testing a fix for that also and I'll post
>>>>> an update later.   But you've seen most of it.
>>>>>
>>>>> Thanks!!
>>>>> Coleen
>>>>>>
>>>>>> Rest looks good.
>>>>>>
>>>>>> Jon
>>>>>>
>>>>>> On 3/25/2014 1:42 PM, Coleen Phillimore wrote:
>>>>>>> Summary: Keep class in CLD::_klasses list and mirror created for
>>>>>>> CDS classes if OOM during restore_shareable_info(). This keeps
>>>>>>> pointers consistent for CMS.
>>>>>>>
>>>>>>> This change makes restoring the mirror for shared classes
>>>>>>> restartable if there is an OOM in the middle of
>>>>>>> restore_unshareable_info.   The mirror field in the classes and
>>>>>>> CLD link is not cleaned up anymore.  The class isn't marked loaded
>>>>>>> though so must be loaded again.
>>>>>>>
>>>>>>> Tested with Stefan's test case with hard coded OOM in jvm, so
>>>>>>> can't add the test.   Also ran all the tests against -client and
>>>>>>> server with -Xshare:auto and with -XX:+UseConcMarkSweepGC.
>>>>>>>
>>>>>>> open webrev at http://cr.openjdk.java.net/~coleenp/8028497/
>>>>>>> bug link https://bugs.openjdk.java.net/browse/JDK-8028497
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Coleen
>>>>>>
>>>>>
>>>>
>>>
>>