RFR: 8087342: crash in klassItable_initialize_itable_for_interface

[Lois Foltan]

On 7/31/2015 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/>

Hi Karen,
Test looks good.  I applied your patch to a Windows build to see if I 
could trigger a failure due to a local method array that had a different 
layout, but it looks good, the test passed with both fastdebug and 
product builds.  Minor comments:

TestStaticandInstance.java:
   line #99 - I think "n" should be "m"
   line #100 - I think the "public" should be "private"
   line #105 - "C.n()" should be "C.m()"

At least that is the way it seems to be implemented in ASM.

Thanks,
Lois

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