[15] RFR(XS): 8239083: C1 assert(known_holder == NULL || (known_holder->is_instance_klass() && (!known_holder->is_interface() || ((ciInstanceKlass*)known_holder)->has_nonstatic_concrete_methods())), "should be non-static concrete method");

[Christian Hagedorn]

Thank you Vladimir and Tobias for your reviews!

Best regards,
Christian

On 02.06.20 08:31, Tobias Hartmann wrote:
> Oops, pressed reply on the wrong email :D
> 
> Best regards,
> Tobias
> 
> 
> On 02.06.20 08:30, Tobias Hartmann wrote:
>> Thanks Vladimir!
>>
>> Best regards,
>> Tobias
>>
>> On 02.06.20 04:09, Vladimir Kozlov wrote:
>>> +1
>>>
>>> Thanks,
>>> Vladimir
>>>
>>> On 5/26/20 6:56 AM, Tobias Hartmann wrote:
>>>> Hi Christian,
>>>>
>>>> looks reasonable to me.
>>>>
>>>> Best regards,
>>>> Tobias
>>>>
>>>> On 15.05.20 11:11, Christian Hagedorn wrote:
>>>>> Hi
>>>>>
>>>>> Please review the following patch:
>>>>> https://bugs.openjdk.java.net/browse/JDK-8239083
>>>>> http://cr.openjdk.java.net/~chagedorn/8239083/webrev.00/
>>>>>
>>>>> The assert fails in the test case when invoking the only static interface method with a method
>>>>> handle. In this case, known_holder is non-NULL. However, known_holder would be set to NULL at [1]
>>>>> since the call returns NULL when known_holder is an interface.
>>>>>
>>>>> In the failing test case, known_holder is non-NULL since GraphBuilder::try_method_handle_inline()
>>>>> calls GraphBuilder::try_inline() with holder_known set to true which eventually lets profile_call()
>>>>> to be called with a non-NULL known_holder argument.
>>>>>
>>>>> On the other hand, when calling a static method without a method handle, known_holder seems to be
>>>>> always NULL:
>>>>> profile_call() is called directly at [2] with NULL or indirectly via try_inline() [3]. In the latter
>>>>> case, cha_monomorphic_target and exact_target are always NULL for static methods and therefore
>>>>> known_holder will also be always NULL in profile_call().
>>>>>
>>>>> We could therefore just remove the assert which seems to be too strong (not handling this edge
>>>>> case). Another option would be to change the call to try_inline() in try_method_handle_inline() to
>>>>> only set holder_known to true if the target is not static. The known_holder is eventually only used
>>>>> in LIR_Assembler::emit_profile_call() [4] but only if op->should_profile_receiver_type() holds [5].
>>>>> This is only true if the callee is not static [6]. The webrev uses the second approach.
>>>>>
>>>>> What do you think?
>>>>>
>>>>> Best regards,
>>>>> Christian
>>>>>
>>>>>
>>>>> [1]
>>>>> http://hg.openjdk.java.net/jdk/jdk/file/dd0caf00b05c/src/hotspot/share/c1/c1_GraphBuilder.cpp#l4386
>>>>> [2]
>>>>> http://hg.openjdk.java.net/jdk/jdk/file/dd0caf00b05c/src/hotspot/share/c1/c1_GraphBuilder.cpp#l3571
>>>>> [3]
>>>>> http://hg.openjdk.java.net/jdk/jdk/file/dd0caf00b05c/src/hotspot/share/c1/c1_GraphBuilder.cpp#l2039
>>>>> [4]
>>>>> http://hg.openjdk.java.net/jdk/jdk/file/dd0caf00b05c/src/hotspot/cpu/x86/c1_LIRAssembler_x86.cpp#l3589
>>>>>
>>>>> [5]
>>>>> http://hg.openjdk.java.net/jdk/jdk/file/dd0caf00b05c/src/hotspot/cpu/x86/c1_LIRAssembler_x86.cpp#l3584
>>>>>
>>>>> [6] http://hg.openjdk.java.net/jdk/jdk/file/dd0caf00b05c/src/hotspot/share/c1/c1_LIR.hpp#l1916