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

[Stefan Karlsson]

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.

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.

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