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

[Coleen Phillimore]

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