RFR (M) JDK-8033150 (#2): invokestatic: IncompatibleClassChangeError trying to invoke static method from a parent in presence of conflicting defaults

[Coleen Phillimore]

On 4/11/14, 10:09 AM, Lois Foltan wrote:
>
> On 4/11/2014 10:02 AM, Coleen Phillimore wrote:
>>
>> This code looks good.  The MethodLookupMode enum is a nice 
>> improvement.  It's a good warning of the complexity of this code.
>> Did something part of the build complain about MethodLookupMode not 
>> being in vmStructs?
>> I'd prefer the serviceability agent not be tempted to duplicate the 
>> method lookup code in the JVM, so not have this change.
>
> Hi Coleen,
>
> Thanks for the review.  The vmStructs.cpp change was a cautionary move 
> on my part to include the new enum MethodLookupMode.  The build did 
> not complain without it.  From your comments sounds like it would be 
> better to leave this change to vmStructs.cpp out.  I can do that and 
> run through some minor testing.  Are you okay with me going forward 
> with this change and not sending it out for another round of review?

Yes, I'm fine with the change if you revert the change to vmStructs.   I 
don't need another round of review.  If vmStructs actually needed this 
new type, then there'd be changes in the serviceability agent required.  
I'm glad that isn't the case.

Thanks!
Coleen

>
> Lois
>
>
>>
>> Thanks,
>> Coleen
>>
>> On 4/11/14, 9:04 AM, Lois Foltan wrote:
>>>
>>> Please review the updated fix, webrev at:
>>> http://cr.openjdk.java.net/~lfoltan/bug_jdk8033150.1/
>>>
>>> This includes Keith's suggestion below.  All testing has been rerun 
>>> successfully.
>>>
>>> Thank you,
>>> Lois
>>>
>>>
>>> On 3/31/2014 3:51 PM, Lois Foltan wrote:
>>>>
>>>> On 3/31/2014 2:08 PM, Keith McGuigan wrote:
>>>>> Hi Lois,
>>>>>
>>>>> I think that looks good.  I like it much better than doing the 
>>>>> static method check in the default method processing.
>>>>> My only suggestion is that it would be nice to encode new 
>>>>> parameter to uncached_lookup_method to be some sort of enum 
>>>>> instead of a boolean, so that it is obvious from the call-site 
>>>>> what the behavior should be (having just "false" in the parameter 
>>>>> list requires you to reference back to the declaration to figure 
>>>>> out what that "false" means).
>>>>>
>>>>> So instead of:
>>>>>    uncached_lookup_method(field_name, field_sig, false);
>>>>> It'd be:
>>>>>   uncached_lookup_method(field_name, field_sig, NORMAL); or
>>>>>   uncached_lookup_method(field_name, field_sig, IGNORE_OVERPASS);
>>>>>
>>>>> (or something like that -- I'm no good at names).
>>>>
>>>> Thank you Keith.  Good suggestion.  I will implement and repost an 
>>>> updated webrev for review.
>>>> Lois
>>>>
>>>>>
>>>>> --
>>>>> - Keith
>>>>>
>>>>>
>>>>> On Mon, Mar 31, 2014 at 1:25 PM, Lois Foltan 
>>>>> <lois.foltan at oracle.com <mailto:lois.foltan at oracle.com>> wrote:
>>>>>
>>>>>     Hi,
>>>>>
>>>>>     Please review the following fix:
>>>>>
>>>>>     Webrev:
>>>>>     http://cr.openjdk.java.net/~lfoltan/bug_jdk8033150/
>>>>>     <http://cr.openjdk.java.net/%7Elfoltan/bug_jdk8033150/>
>>>>>
>>>>>     Bug: invokestatic: IncompatibleClassChangeError trying to
>>>>>     invoke static method from a parent in presence of conflicting
>>>>>     defaults
>>>>>     https://bugs.openjdk.java.net/browse/JDK-8033150
>>>>>
>>>>>     Summary of fix:
>>>>>     During default method processing, determine_target(), is
>>>>>     responsible for making decisions on whether
>>>>>     to create and add an overpass method to the current class for
>>>>>     issues that have been encountered during the walk
>>>>>     of the default methods.  The routine
>>>>>     defaultMethods.cpp/has_matching_static() helped determine this
>>>>>     decision by looking within the current class for a static
>>>>>     method that should be preferred during method
>>>>>     resolution over an overpass method being created.  However,
>>>>>     has_matching_static() did not continue to
>>>>>     look for a static method in current class' superclasses which
>>>>>     JDK-8033150 exposed.
>>>>>
>>>>>     After looking at the code more closely, the has
>>>>>     _matching_static() concept is being moved out out of default
>>>>>     method processing and into method resolution processing.  The
>>>>>     primary reason for this is to allow
>>>>>     default method processing to match method selection where
>>>>>     statics are and should be ignored.   According
>>>>>     to JVMS, static methods should only be preferred over an
>>>>>     overpass method at method and interface
>>>>>     method resolution time.  To enable method resolution to
>>>>>     potentially find a static method over an overpass
>>>>>     method, a new parameter "ignore_overpass" was added to
>>>>>     InstanceKlass::uncached_lookup_method().
>>>>>     It has the affect of indicating to find_method_index() to
>>>>>     ignore overpass methods and continue the search
>>>>>     in case a static method of the same name and signature is
>>>>>     present in the current class or its superclasses.
>>>>>
>>>>>     Tests:
>>>>>         - jtreg hotspot/test/*, JDK java.lang & java.util,
>>>>>     vm.quick.testlist, JCK lang & vm, defmeth tests
>>>>>     - Test will be added to the defmeth test system
>>>>>
>>>>>     Thank you,
>>>>>     Lois
>>>>>
>>>>>
>>>>
>>>
>>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/attachments/20140411/478a4404/attachment.html>