[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]

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