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

[Coleen Phillimore]

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