C1 GraphBuilder::profile_call assert failing when inlining static interface method through MethodHandle

[Jorn Vernee]

Hi Vlad,

I've filed here: https://bugs.openjdk.java.net/browse/JDK-8239083

FWIW, looking at the caller of this function: 
GraphBuilder::try_inline_full, it seems that arguments to the call are 
also profiled, which makes sense for a static call as well.

Jorn

On 13/02/2020 17:55, Vladimir Ivanov wrote:
> Hi Jorn,
>
> The assert does looks too strong and should be adjusted.
>
> But it looks surprising that profiling is peformend for static methods:
>
>   assert(..., "should be non-static concrete method");
>
> Please, file a bug.
>
> Best regards,
> Vladimir Ivanov
>
> On 13.02.2020 14:46, Jorn Vernee wrote:
>> Looks like the reproducer code got crumpled up for some reason. Let 
>> me try again:
>>
>> public class Test {
>>
>>      static final MethodHandle MH_m;
>>
>>      static {
>>          try {
>>              MH_m = lookup().findStatic(MyInterface.class, "m", 
>> MethodType.methodType(void.class));
>>          } catch (ReflectiveOperationException e) {
>>              throw new BootstrapMethodError(e);
>>          }
>>      }
>>
>>      public static void main(String[] args) throws Throwable {
>>          for (int i = 0; i < 20_000; i++) {
>>              payload();
>>          }
>>      }
>>
>>      static void payload() throws Throwable {
>>          MH_m.invokeExact();
>>      }
>>
>> }
>>
>> interface MyInterface {
>>      static void m() {}
>> }
>>
>> Jorn
>>
>> On 13/02/2020 12:39, Jorn Vernee wrote:
>>> Hi,
>>>
>>> We were seeing an assert being hit in C1 when running the project 
>>> Panama test suite. After some debugging I have narrowed the failure 
>>> down to the following reproducer:
>>>
>>> public class Test { static final MethodHandle MH_m; static { try { 
>>> MH_m = lookup().findStatic(MyInterface.class, "m", 
>>> MethodType.methodType(void.class)); } catch 
>>> (ReflectiveOperationException e) { throw new 
>>> BootstrapMethodError(e); } } public static void main(String[] args) 
>>> throws Throwable { for (int i = 0; i < 20_000; i++) { payload(); } } 
>>> static void payload() throws Throwable { MH_m.invokeExact(); } } 
>>> interface MyInterface { static void m() {} }
>>>
>>> Run with `-Xbatch -XX:TieredStopAtLevel=3`.
>>>
>>> The problem is the following assert in GraphBuild::profile_call:
>>>
>>>   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");
>>>
>>> In this particular case, we are inlining a static method in a known 
>>> holder which is also an interface, but doesn't have any other 
>>> non-static concrete methods, so the assert fails.
>>>
>>> The MethodHandle seems to be significant since known_holder is NULL 
>>> when simply calling MyInterface.m(). Looking at the a assert and 
>>> it's history, I'm not really sure what it's supposed to guard 
>>> against. It seems to want to check that the callee is non-abstract 
>>> when we have a known holder, but in that case I'm not sure why it 
>>> doesn't just use `!callee->is_abstract()`.
>>>
>>> Not sure what should be done here? I tried changing the assert to 
>>> `assert(known_holder == NULL || !callee->is_abstract())` but that 
>>> seems to fail in other cases. Maybe we can just remove it?
>>>
>>> Thanks,
>>> Jorn
>>>