RFR: 8087342: crash in klassItable_initialize_itable_for_interface

Tue Aug 4 17:39:08 UTC 2015

On 8/4/2015 11:16 AM, Karen Kinnear wrote:
> Actually, I didn't include this in the webrev, but I added some text 
> from the email below to the is_miranda
> description, so the full text before the method is now ... Suggested 
> edits are welcome.
>

Thanks Karen, looks better.  I have no further comments.
Lois

> thanks,
> Karen
>
> // Check if a method is a miranda method, given a class's methods array,
> // its default_method table and its super class.
> // "Miranda" means an abstract non-private method that would not be
> // overridden for the local class.
> // A "miranda" method should only include non-private interface
> // instance methods, i.e. not private methods, not static methods,
> // not default methods (concrete interface methods), not overpass methods.
> // If a given class already has a local (including overpass) method, a
> // default method, or any of its superclasses has the same which would 
> have
> // overridden an abstract method, then this is not a miranda method.
> //
> // Miranda methods are checked multiple times.
> // Pass 1: during class load/class file parsing: before vtable size 
> calculation:
> // include superinterface abstract and default methods (non-private 
> instance).
> // We include potential default methods to give them space in the vtable.
> // During the first run, the current instanceKlass has not yet been
> // created, the superclasses and superinterfaces do have instanceKlasses
> // but may not have vtables, the default_methods list is empty, no 
> overpasses.
> // This is seen by default method creation.
> //
> // Pass 2: recalculated during vtable initialization: only include 
> abstract methods.
> // The goal of pass 2 is to walk through the superinterfaces to see if 
> any of
> // the superinterface methods (which were all abstract pre-default 
> methods)
> // need to be added to the vtable.
> // With the addition of default methods, we have three new challenges:
> // overpasses, static interface methods and private interface methods.
> // Static and private interface methods do not get added to the vtable and
> // are not seen by the method resolution process, so we skip those.
> // Overpass methods are already in the vtable, so vtable lookup will
> // find them and we don't need to add a miranda method to the end of
> // the vtable. So we look for overpass methods and if they are found we
> // return false. Note that we inherit our superclasses vtable, so
> // the superclass' search also needs to use find_overpass so that if
> // one is found we return false.
> // False means - we don't need a miranda method added to the vtable.
> //
> // During the second run, default_methods is set up, so concrete 
> methods from
> // superinterfaces with matching names/signatures to default_methods 
> are already
> // in the default_methods list and do not need to be appended to the 
> vtable
> // as mirandas. Abstract methods may already have been handled via
> // overpasses - either local or superclass overpasses, which may be
> // in the vtable already.
> //
> // Pass 3: They are also checked by link resolution and selection,
> // for invocation on a method (not interface method) reference that
> // resolves to a method with an interface as its method_holder.
> // Used as part of walking from the bottom of the vtable to find
> // the vtable index for the miranda method.
> //
> // Part of the Miranda Rights in the US mean that if you do not have
> // an attorney one will be appointed for you.
>
>
>
> On Jul 31, 2015, at 2:01 PM, Karen Kinnear wrote:
>
>> Lois,
>>
>> Here is an updated webrev. The hotspot code has not changed (except 
>> for the fixed comments). I added a test to investigate if
>> I could have static and instance fields in the same class (obviously 
>> with -Xverify:none). I did manage
>> to have 1 public static, 1 private instance and 1 overpass for an AME 
>> from an abstract interface.
>> Obviously none of this matches the jls, or jvms or passes the 
>> verifier - but I did want to make sure
>> the code did the right thing.
>>
>> The test passes in product and fastdebug. I would appreciate a review 
>> for the test itself.
>> Thanks to Harold for jasm example :-)
>>
>> updated webrev: http://cr.openjdk.java.net/~acorn/8087342.3/webrev/ 
>> <http://cr.openjdk.java.net/%7Eacorn/8087342.3/webrev/>
>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8087342
>>
>> thanks,
>> Karen
>>
>> On Jul 16, 2015, at 6:03 PM, Karen Kinnear wrote:
>>
>>> Lois,
>>>
>>> Thank you for the detailed review. I really appreciate it.
>>> On Jul 15, 2015, at 8:12 PM, Lois Foltan wrote:
>>>
>>>>
>>>> On 7/15/2015 12:40 PM, Karen Kinnear wrote:
>>>>> Please review for JDK9:
>>>>>
>>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8087342
>>>>> webrev: http://cr.openjdk.java.net/~acorn/8087342.2/webrev/ 
>>>>> <http://cr.openjdk.java.net/%7Eacorn/8087342.2/webrev/>
>>>>>
>>>>> Crash occurs in product when we should through 
>>>>> IncompatibleClassChangeError.
>>>>
>>>> Hi Karen,
>>>> I think this looks good and I really like how straight forward 
>>>> klassVtable::is_miranda now is.  Some minor clarification comments:
>>>>
>>>> - src/share/vm/instanceKlass.cpp
>>>> comments for the find_local_method* were changed to state:
>>>> +// note that the local methods array can have up to one overpass, 
>>>> one static
>>>> +// and one instance (private or not) with the same name/signature
>>>> I think there are two combinations that the code depends on not 
>>>> occurring, they are:
>>>>      1. all 3 are in existence in the local methods array (one 
>>>> overpass, one static and one instance)
>>>>      2. the combination of one static and one instance (private or not)
>>>> In other words there has to be an overpass to cause more than one 
>>>> method with the same name/signature within the local methods array. 
>>>>  And it is either an overpass and a static or an overpass and an 
>>>> instance, but not all 3.  Correct me if I am wrong.
>>> I need to write an additional test to check that. I agree that both 
>>> the spec and the ClassFileParser if _need_verify is set will
>>> prevent instance and static overlap. I need to see what happens if 
>>> you skip verification. I will get back to you with that and update the
>>> comments to clarify if needed.
>>>>
>>>> - src/share/vm/oops/klassVtable.cpp
>>> Let me see if I can make this clearer - let me know if I can make 
>>> the comments clearer. I truly appreciate your trying to see
>>> if this all makes sense and is consistent. It is still too complex.
>>>> Thank you for adding the improved comments ahead of is_miranda.  My 
>>>> read is that overpass methods are not considered miranda methods 
>>>> and I agree with that statement.
>>> Yes, they are not considered miranda methods because you don't need 
>>> to add them to the vtable as abstract methods because
>>> they already are in the vtable from being in the class' LOCAL 
>>> methods array.
>>> So pass 1: overpasses do not exist
>>> pass 2: overpasses are already in the vtable when we calculate mirandas
>>> pass 3: overpasses in a class have the class as their method_holder, 
>>> not an interface, so we aren't looking them up here
>>>
>>> So - pass 2 is the one that cares about the 
>>> find_local_method(Klass:find_overpass vs. Klass::skip_overpass).
>>>
>>>
>>>> Yet, Klass::find_overpass is specified in the code.  I think the 
>>>> code is correct, but based on the comment I would have thought 
>>>> Klass::skip_overpass should have been specified?
>>> I also think the code is correct.
>>> So what pass 2 is doing is walking through the superinterfaces to 
>>> see if any of the superinterface methods (which all used to be abstract)
>>> need to be added to the vtable.
>>>
>>> So the question is - what superinterface methods belong in the vtable?
>>> So the searches in is_miranda are designed to find out if there is a 
>>> method in the vtable already such that we don't
>>> need to add the superinterface method - e.g. this was abstract and 
>>> we have an implementation for it.
>>>
>>> With the addition of default methods, we have three new challenges - 
>>> overpasses, static interface methods and private
>>> interface methods.
>>>
>>> Static and private interface methods do not get added to the vtable 
>>> and are not seen by the method resolution process.
>>> So we skip those.
>>>
>>> Overpass methods are already in the vtable, so vtable lookup will 
>>> find them there and we don't need to add a miranda method
>>> to the end of the vtable. So we look for those explicitly. Note that 
>>> we inherit our superclasses vtable, so the superclass' search
>>> also needs to use find_overpass.
>>>
>>> Does this make sense?
>>>
>>> Is there a way I could make this clearer via comments?
>>>
>>>> Much like skip_static and skip_private.  So based on your later 
>>>> statement that "Abstract methods may already have been handled via 
>>>> overpasses" it implies that overpass methods, although not miranda 
>>>> methods, can satisfy or stand in for an miranda during pass 2.  So 
>>>> they must be found, did I understand that correctly?
>>>
>>>>
>>>> Again, looks good.  I don't need to see another review.  My 
>>>> comments were merely clarification based.
>>> many thanks,
>>> Karen
>>>
>>>>
>>>> Thanks,
>>>> Lois
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>>
>>>>> testing:
>>>>> internal tests: Defmeth (updated), SelectionResolution - product 
>>>>> and fastdebug
>>>>> jprt
>>>>> jck
>>>>> jtreg/hotspot_all
>>>>> jtreg/jdk_streams
>>>>> test,noncolo.testlist, -atk quick
>>>>>
>>>>> (jck and macosx testing in progress)
>>>>>
>>>>> thanks,
>>>>> Karen
>>>>>
>>>>
>>>
>>
>