RFR: JDK-8147026- Convert an assert in ClassLoaderData to a guarantee

[Shafi Ahmad]

Thanks for review Coleen.

Regards,
Shafi

-----Original Message-----
From: Coleen Phillimore 
Sent: Tuesday, April 26, 2016 7:34 AM
To: hotspot-runtime-dev at openjdk.java.net
Subject: Re: RFR: JDK-8147026- Convert an assert in ClassLoaderData to a guarantee


I should add that this is Reviewed by me.

Thanks,

Coleen


On 4/25/16 3:09 PM, Coleen Phillimore wrote:
>
> I updated the bug and think there's no harm turning this assert into a 
> guarantee.  If a class_loader is incorrectly unloaded, this might be a 
> useful place to check this.
>
> Coleen
>
> On 2/18/16 9:20 AM, Coleen Phillimore wrote:
>>
>> Hi Shafi,
>>
>> I agree with Ioi, I don't see how this guarantee would have helped 
>> you.  From reading the bug, having some verification in jvm.cpp may 
>> have been more likely to have found your bug, because it's closer to 
>> the source of where you suspected the bug came from. For the record, 
>> I don't think this would hurt performance, which is another concern 
>> with turning some asserts into guarantees.
>>
>> I don't know if I like the idea of turning some asserts into 
>> guarantees with another switch like the -Xlog switches (which I find 
>> so complicated now I need wrapper scripts),
>> https://bugs.openjdk.java.net/browse/JDK-8149478 but we should 
>> discuss on another thread.
>>
>> Coleen
>>
>>
>> On 2/18/16 2:58 AM, Ioi Lam wrote:
>>> Hi Shafi,
>>>
>>> I read the bug report, and it says, " the JVM was crashing during GC 
>>> because the object it was looking at had a pointer to a bad klass.
>>> The core file analysis showed that the classloader object in the 
>>> klass was not a valid Oop". Are you sure that 
>>> ClassLoaderDataGraph::find_or_create was indeed called with an 
>>> invalid loader? There are many reason for this to happen. E.g., 
>>> there could have been strayed memory writes that corrupts the 
>>> pointer to the classloader object.
>>>
>>> In fact, if you have a invalid loader here, the call will return 
>>> junk
>>>
>>> inline ClassLoaderData *ClassLoaderDataGraph::find_or_create(Handle
>>> loader, TRAPS) {
>>>   guarantee(loader() != NULL && loader()->is_oop(), "Loader must be 
>>> oop");
>>>   // Gets the class loader data out of the java/lang/ClassLoader 
>>> object, if non-null
>>>   // it's already in the loader_data, so no need to add
>>>   ClassLoaderData* loader_data=
>>> java_lang_ClassLoader::loader_data(loader());
>>>   if (loader_data) {
>>>      return loader_data;  <<<<<<  junk
>>>   }
>>>
>>> The callers of this code (systemDicrionary.cpp) will deference the 
>>> returned value to do various operations related to class loading, 
>>> and will crash quickly with a bad pointer. It's highly unlikely that 
>>> your original test scenario would have been healthy enough to get to 
>>> the next GC.
>>>
>>> So, I don't think changing the assert to guarantee will 
>>> significantly affect how the product JVM would behave with bad input 
>>> to find_or_create.
>>>
>>> - Ioi
>>>
>>> On 2/16/16 1:51 AM, David Holmes wrote:
>>>> Hi Shafi,
>>>>
>>>> On 16/02/2016 7:41 PM, Shafi Ahmad wrote:
>>>>> Hi All,
>>>>>
>>>>>
>>>>>
>>>>> Please review the trivial change of replacing 'assert' by 'guarantee'
>>>>>
>>>>>
>>>>>
>>>>> Summary: Having a guarantee in method 
>>>>> "ClassLoaderDataGraph::find_or_create" ensuring that the 
>>>>> classloader is a valid oop will help catch the errors at an early 
>>>>> stage during the run rather than crashing the JVM later on in the GC.
>>>>
>>>> I'm not sure I agree with this change on principle. The assert says 
>>>> "we never expect this to fail and we expect to weed out any 
>>>> problems during testing". I could see the need to expand the assert 
>>>> to also check for it being an oop though.
>>>>
>>>> I'll defer approval of this to Coleen and Ioi as they have the 
>>>> expertise in this code area.
>>>>
>>>> Thanks,
>>>> David
>>>>
>>>>>
>>>>>
>>>>> Open webrev: http://cr.openjdk.java.net/~csahu/8147026/webrev.00
>>>>>
>>>>> Bug link: https://bugs.openjdk.java.net/browse/JDK-8147026
>>>>>
>>>>> Testing:  JPRT
>>>>>
>>>>>
>>>>>
>>>>> Since we are changing 'assert' by 'guarantee' which will add few 
>>>>> instruction into code in product build and I am not sure about the
>>>>> *hotness* of this method.
>>>>>
>>>>> So I run the performance test [refworkload] to make sure the 
>>>>> impact of this change in the product build.
>>>>>
>>>>> I am attaching the  performance run report for your reference.
>>>>>
>>>>>
>>>>>
>>>>> Regards,
>>>>>
>>>>> Shafi
>>>>>
>>>>>
>>>>>
>>>
>>
>