RFR 8016325: JVM hangs verifying system dictionary

[Jon Masamitsu]

On 6/24/13 2:20 PM, Coleen Phillimore wrote:
>
> Thanks Jon for looking at this.
>
> On 06/24/2013 02:21 PM, Jon Masamitsu wrote:
>> Coleen,
>>
>> http://cr.openjdk.java.net/~coleenp/8016325/src/share/vm/oops/klass.cpp.frames.html 
>>
>>
>> 650 void Klass::verify_on(outputStream* st, bool in_dictionary) {
>>
>>
>> "in_dictionary" is not used, right?
>
> It isn't used but I forgot to change the name of the parameter to 
> check_dictionary which has the opposite sense.
I'm confused by that statement.
>>
>> http://cr.openjdk.java.net/~coleenp/8016325/src/share/vm/oops/klass.hpp.frames.html 
>>
>>
>> So do you need this change to add the parameter "check_dictionary"?
>>>   706   virtual void verify_on(outputStream* st, bool 
>>> check_dictionary = true);
>>>   707   void verify(bool check_dictionary = true) { verify_on(tty, 
>>> check_dictionary); }
>>>   708
>> If you do, is the "virtual" on 706 needed?  And are the number of 
>> lines you would
>> have to change if "check_dictionary" does not have a default (not a 
>> fan of defaults),
>> so large?
>
> Yes, it's needed because then InstanceKlass::verify_on will override 
> this versions.  It won't if the parameters don't match. We definitely 
> want verify_on() to be virtual.

Ok.
>
> I'm afraid the number of lines would be pretty large if there wasn't a 
> default parameter.
>
The clean coding book would not be happy.  Your call, but are you
sure you don't want to tidy it up now?

>>
>> What's the rule about when to use is_metaspace_object() to
>> do an assertion check?
>
> The rule I applied was to use this test if the metadata object was 
> coming from some location where non-metadata could also be stored by 
> mistake (like the frame_x86.hpp example).   In the verification code 
> the compiler's static type checking code can do the job for us so 
> that's why I removed them.

Thanks.

Jon

>
> Thanks,
> Coleen
>
>>
>> Rest looks good.
>>
>> Jon
>>
>>
>> On 6/21/13 9:35 AM, Coleen Phillimore wrote:
>>> Summary: Minimize redundant verifications of Klasses.
>>>
>>> Also removed is_metadata() because checking that metadata is not in 
>>> the Java heap doesn't make sense anymore and could slow down 
>>> verification also.   Some cases that aren't statically checked by 
>>> the compiler now call is_metaspace_object() under assert for 
>>> verification.
>>>
>>> Tested by nsk.quick.testlist, and failing SQE test.
>>>
>>> open webrev at http://cr.openjdk.java.net/~coleenp/8016325/
>>> bug link at http://bugs.sun.com/view_bug.do?bug_id=8016325
>>> local bug link https://jbs.oracle.com/bugs/browse/JDK-8016325
>>>
>>> thanks,
>>> Coleen
>>
>