RFR: 8087342: crash in klassItable_initialize_itable_for_interface

[Karen Kinnear]

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