RFR JDK-8195639: [Graal] nsk/jvmti/PopFrame/popframe009 fail with Graal in -Xcomp

Tue Nov 27 22:50:05 UTC 2018

Let me list the proposed solutions:

1. short-circuit compiles in CompilationPolicy::event in is_interp_only_mode
2. detect is_interp_only_mode in resolve stubs and force a c2i call
3. don't offer a JVMTI suspend point in resolve stubs (or JavaCallWrapper)

Solution 1 penalizes other threads, and don't prevent other threads from 
compiling the method, so while it may help a little, it's incomplete.
Solutions 1 and 2 allow the thread to resume in a different method (the 
callee method), causing other problems.
With Solution 3, the frame we suspend is the same as the frame we end up 
in after the resume, so I believe it solves all the problems.
IMHO this is the correct solution to pursue.

dl

On 11/27/18 2:19 PM, serguei.spitsyn at oracle.com wrote:
> Hi Dean,
>
> You also asked the questions:
>
> > Doesn't is_interp_only_mode() only apply to the current thread?
>
> Yes it does.
>
>
> > I don't think it's safe to assume no compiles will happen in other 
> threads,
> > or that a method call target is not already compiled, because as far
> > as I can tell, JVMTI only deoptimizes the active frames.
>
> I agree with it.
> However, compilations on the thread with the interp_only_mode enabled 
> is the most important case.
> Disabling compilations such thread improves testing stability.
>
> > The bug report describes a case where the caller has been deoptimized
> > while resolving a call.  I don't see how this fix prevents the target
> > from being a previously compiled method.
> > But we do need to make sure not to call into compiled code, so I think
> > the fix needs to be in code like 
> SharedRuntime::resolve_static_call_C(),
> > where it returns get_c2i_entry() if is_interp_only_mode() is true.
>
> Thank you for making this point.
>
> It would be nice to attack this as well.
> We can try to investigate this approach further.
> One problem is that there are more cases like resolve_static_call_C,
> for instance, resolve_virtual_call_C and resolve_opt_virtual_call_C.
> We may need to fix the same in other places, like 
> JavaCallWrapper::JavaCallWrapper.
>
> We can start from fixing it in the resolve_static_call_C.
> If it is successful then continue this effort for other cases as well.
>
> What do you think?
>
> Thanks,
> Serguei
>
>
> On 11/27/18 1:01 PM, Alex Menkov wrote:
>> Hi Dean,
>>
>> Thank you for the feedback and for the comments in Jira.
>>
>> >> How does this solve the problem of
>> >> HotSpotJVMCIRuntime.adjustCompilationLevel being called?
>>
>> It doesn't. It fixes another issue.
>> The test suspend test thread and ensures that top frame is a 
>> "fibonachi" method. Then it turns SingleStep on, does PopFrame and 
>> resumes the thread.
>> The problem is the thread continues execution of compiled code 
>> (ignoring SingleStep) and we get SindleStep event only when 
>> adjustCompilationLevel method is called (it's interpreted code).
>>
>> There are several other bugs which are (most likely) caused by 
>> suspend during call setup (JDK-8195635, JDK-8214093, JDK-8207013)
>>
>> --alex
>>
>> On 11/27/2018 01:39, serguei.spitsyn at oracle.com wrote:
>>> Hi Dean,
>>>
>>> Thank you a lot for looking at this!
>>> Just a couple of points from me (it is up to Alex to provide a full 
>>> answer).
>>>
>>>
>>> I think, Alex in this RFR missed to tell that we knew about this 
>>> issue that an incorrect frame will be popped.
>>> But after some discussion we decided to file a separate issue on this.
>>> Alex wanted to create a good stand-alone test case demonstrating 
>>> this problem before filing it.
>>>
>>> Now, as I can see, the JDK-8195635 
>>> <https://bugs.openjdk.java.net/browse/JDK-8195635> looks very close 
>>> to a bug that we wanted to file.
>>> The only difference is that our scenario includes the 
>>> SharedRuntime::resolve_static_call_C instead of the 
>>> JavaCallWrapper::JavaCallWrapper as a helper for Java method 
>>> invocation.
>>> If I understand corrctly (Alex will fix me if needed), the jtreg 
>>> test we used to chase this issue did not catch a problem with the 
>>> HotSpotJVMCIRuntime.adjustCompilationLevel.
>>>
>>>
>>> The suggested fix was to fix the mismatch in the 
>>> TieredThresholdPolicy::even with the comment in the interpreter code:
>>> nmethod* InterpreterRuntime::frequency_counter_overflow(JavaThread* 
>>> thread, address branch_bcp) {
>>>      . . .
>>>    if (nm != NULL && thread->is_interp_only_mode()) {
>>>      // Normally we never get an nm if is_interp_only_mode() is 
>>> true, because
>>>      // policy()->event has a check for this and won't compile the 
>>> method when
>>>      // true. However, it's possible for is_interp_only_mode() to 
>>> become true
>>>      // during the compilation. We don't want to return the nm in 
>>> that case
>>>      // because we want to continue to execute interpreted.
>>>      nm = NULL;
>>>    }
>>>
>>>  > So I think the fix needs to be in code like 
>>> SharedRuntime::resolve_static_call_C(),
>>>  > where it returns get_c2i_entry() if is_interp_only_mode() is true.
>>>
>>> I'm not sure, the adding this condition and returning the 
>>> get_c2i_entry() is always correct.
>>> We need some help from the Compiler team to make it right.
>>>
>>> BTW, the interp_only_mode has been enabled only when some 
>>> interp_only events are enabled.
>>> It is not set by either PopFrame or ForceEarlyReturn.
>>> But the popframe009 test enables single stepping, so we wanted to 
>>> make this part right.
>>>
>>> Thanks,
>>> Serguei
>>>
>>>
>>> On 11/26/18 16:05, dean.long at oracle.com wrote:
>>>> How does this solve the problem of 
>>>> HotSpotJVMCIRuntime.adjustCompilationLevel being called?
>>>>
>>>> I don't think this fix is the right approach.  Doesn't 
>>>> is_interp_only_mode() only apply to the current thread?  I don't 
>>>> think it's safe to assume no compiles will happen in other threads, 
>>>> or that a method call target is not already compiled, because as 
>>>> far as I can tell, JVMTI only deoptimizes the active frames.  The 
>>>> bug report describes a case where the caller has been deoptimized 
>>>> while resolving a call.  I don't see how this fix prevents the 
>>>> target from being a previously compiled method.  But we do need to 
>>>> make sure not to call into compiled code, so I think the fix needs 
>>>> to be in code like SharedRuntime::resolve_static_call_C(), where it 
>>>> returns get_c2i_entry() if is_interp_only_mode() is true.  However, 
>>>> there is still another problem.  By allowing JVMTI to suspend the 
>>>> thread during call setup, but reporting the frame as still in the 
>>>> caller instead of the callee, we confuse JVMTI into thinking that 
>>>> execution will resume in the caller instead of the callee.  We may 
>>>> want to restrict where we offer JVMTI suspend points, and not offer 
>>>> a JVMTI suspend point in SharedRuntime::resolve_static_call_C and 
>>>> friends at all.
>>>>
>>>> dl
>>>>
>>>>
>>>> On 11/26/18 11:14 AM, Alex Menkov wrote:
>>>>> Hi all,
>>>>>
>>>>> Please review the fix for
>>>>> https://bugs.openjdk.java.net/browse/JDK-8195639
>>>>> webrev:
>>>>> http://cr.openjdk.java.net/~amenkov/popframe009/webrev.01/
>>>>>
>>>>> Description:
>>>>> The test suspends a thread, turns on single stepping and then 
>>>>> calls PopFrame. SingleStep event is expected as soon as the thread 
>>>>> is resumed and PopFrame is processed (so we have call stack with 
>>>>> the depth 1 less than it was before PopFrame). Instead SingleStep 
>>>>> event is received with much deeper call stack (and PopFrame 
>>>>> processes wrong frame).
>>>>> Research shown that this is caused by missed deoptimization of the 
>>>>> current frame.
>>>>> As far as I understand CompilationPolicy::event should handle the 
>>>>> case when the thread has is_interp_only_mode() set, but 
>>>>> TieredThresholdPolicy::event checks this only then CompLevel is 
>>>>> CompLevel_none.
>>>>> CompilerRuntime always calls policy()->event with CompLevel == 
>>>>> CompLevel_aot.
>>>>>
>>>>> The fix looks quite simple, but I'd appreciate feedback from 
>>>>> runtime and compiler teams as I'm not sure I completely understand 
>>>>> all the details of the "PopFrame dance".
>>>>>
>>>>> --alex
>>>>
>>>
>