[8] RFR (XS): 8031695: CHA ignores default methods during analysis leading to incorrect code generation

[harold seigel]

Hi Vladimir,

Thanks for the explanation.

Harold

On 1/22/2014 3:27 PM, Vladimir Ivanov wrote:
> Harold,
>
> I'm reluctant to change it with this fix.
>
> Dependencies::is_concrete_method(ciMethod* m) isn't called from 
> anywhere and it already diverged a bit from Method* version. Also, 
> method type (normal or overpass) isn't cached in ciMethod, so 
> is_overpass() isn't available unless corresponding Method is queries 
> instead.
>
> At least, it needs additional investigation. Probably, this function 
> should go away or delegate to Method* version.
>
> Thanks for looking into the fix!
>
> Best regards,
> Vladimir Ivanov
>
> On 1/22/14 11:40 PM, harold seigel wrote:
>> Hi Vladimir.
>>
>> Does this second version of is_concrete_method() also need a "&&
>> !m->is_overpass()"?
>>
>> 1191 bool Dependencies::is_concrete_method(ciMethod* m) {
>> 1192   // Statics are irrelevant to virtual call sites.
>> 1193   if (m->is_static())  return false;
>> 1194
>> 1195   // We could also return false if m does not yet appear to be
>> 1196   // executed, if the VM version supports this distinction also.
>> 1197   return !m->is_abstract();
>> 1198 }
>>
>> Thanks, Harold
>>
>>
>> On 1/22/2014 12:29 PM, Vladimir Ivanov wrote:
>>> http://cr.openjdk.java.net/~vlivanov/8031695/webrev.00/
>>> https://bugs.openjdk.java.net/browse/JDK-8031695
>>>
>>> CHA erroneously ignores default methods when looking for different
>>> versions of a method in a class hierarchy.
>>>
>>> Consider the following hierarchy:
>>>   interface I { default void m() { ... }}
>>>   class A implements I {}
>>>
>>>   class C extends A { }
>>>   class D extends A { public void m() { ... } }
>>>
>>> Call site: invokevirtual A.m()
>>>
>>> A.m resolves to I.m. Searching from A, CHA finds only D.m and
>>> concludes that D.m is the only implementation of I.m, since
>>> Dependencies::is_concrete_method returns false for I.m.
>>>
>>> But there is C with default implementation from I. So, if VM inlines
>>> D.m and then C's instance is passed as objectref, the wrong method is
>>> invoked (D.m instead of I.m).
>>>
>>> The fix is to take default methods into account doing CHA analysis and
>>> treat the call site as non-monomorphic when a concrete method is found
>>> for a default method.
>>>
>>> Conservative approach is chosen, since the fix is targeted for 8.
>>>
>>> Testing: failing application, regression test, vm.defmeth.testlist,
>>> JDK regression tests (java/util/, java/lang)
>>>
>>> Thanks!
>>>
>>> Best regards,
>>> Vladimir Ivanov
>>