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

[Coleen Phillimore]

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