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

[Coleen Phillimore]

On 3/27/14 3:01 PM, Ioi Lam wrote:
> 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.

It is to delay actually deallocating the InstanceKlass until a 
safepoint, so that the mirror can point to it until then.  But that 
might not be long enough.  We might need to scribble over the mirror too 
(see other emails).  It's also the only other case that unlinks the 
Klass from the CLD::_klasses list outside a safepoint.  That's why it 
was included in this fix, because it also helps CMS code.

Coleen

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