[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");

[Tobias Hartmann]

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