RFR 8223229 [lworld] C1 crashes when calling final virtual methods with value arguments

[Ioi Lam]

On 5/2/19 1:00 AM, Tobias Hartmann wrote:
> Hi Ioi,
>
> On 02.05.19 08:06, Ioi Lam wrote:
>> https://bugs.openjdk.java.net/browse/JDK-8223229
>> http://cr.openjdk.java.net/~iklam/valhalla/8223229-c1-opt-virtual-call-scalarized-args.v01/

Hi Tobias, thanks for the review
> In sharedRuntime.cpp:1147, could you use caller->code()->is_compiled_by_c1() instead of doing a code
> cache lookup?

Will caller->code() may be different than callerFrame.cb() if the caller 
gets recompiled? Anyway, the lookup is not necessary, as callerFrame._cb 
was already looked up when the frame was constructed. How about this:?

     if (callerFrame.is_compiled_frame() && 
!callerFrame.is_deoptimized_frame()) {
       caller_is_c1 = callerFrame.cb()->is_compiled_by_c1();
     }

BTW, I copied the original code from here:


methodHandle SharedRuntime::reresolve_call_site(JavaThread *thread, 
bool& is_optimized, bool& caller_is_c1, TRAPS) {
   ...
   if (caller.is_compiled_frame() && !caller.is_deoptimized_frame()) {

     address pc = caller.pc();
     // Check for static or virtual call
     bool is_static_call = false;
     CompiledMethod* caller_nm = CodeCache::find_compiled(pc);
     caller_is_c1 = caller_nm->is_compiled_by_c1();

Do you think this lookup can also be replaced with 
caller.cb()->as_compiled_method()? If so, I'll file an REF and fix in 
the main repo.

This code seems to be there since mercurial version 1

http://hg.openjdk.java.net/valhalla/valhalla/annotate/489c9b5090e2/hotspot/src/share/vm/runtime/sharedRuntime.cpp#l1280

>
> Otherwise it looks good to me!
>
>> BTW, I am not quite sure what to do with the unverified entry point. It seems to me that we would
>> need something like this:
>>
>> void CompiledIC::compute_monomorphic_entry(...) {
>>   ...
>>   if (is_optimized) {
>>      entry = caller_is_c1 ?
>>                method_code->verified_value_entry_point() :
>>                method_code->verified_entry_point();
>>    } else {
>>      entry = caller_is_c1 ?
>>                method_code->unverified_value_entry_point() :
>>                method_code->entry_point();
>>      }
>>    }
>>
>> Tobias, what do you think?
> Yes, it seems that we need an unverified value entry point that falls through to the VVEP:
>
> UEP
> VVEP (RO)
> UVEP     <- new
> VVEP
> VEP
>
> I can implement this for C2, just file a bug when you have the C1 part ready.
OK I will do this for C1 first and get some test cases working with C2 
calling C1.

Thanks
- Ioi

> If everything is implemented, we should think about a better naming scheme for these entry points
> (John had some suggestions).
>
> Thanks,
> Tobias