RFR: 8087342: crash in klassItable_initialize_itable_for_interface

[Karen Kinnear]

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