[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 Tobias for your review!

Best regards,
Christian

On 26.05.20 15:56, 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