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

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

Post by Alex Menkov
Hi all,
Please review the fix for
https://bugs.openjdk.java.net/browse/JDK-8195639
http://cr.openjdk.java.net/~amenkov/popframe009/webrev.01/
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

s***@oracle.com

2018-11-27 09:39:03 UTC

<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<div class="moz-cite-prefix">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 <a
href="https://bugs.openjdk.java.net/browse/JDK-8195635"
title="[Graal] nsk/jvmti/unit/ForceEarlyReturn/earlyretbase
crashes with assertion "compilation level out of
bounds"" class="issue-link" data-issue-key="JDK-8195635">JDK-8195635</a>
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, <a class="moz-txt-link-abbreviated" href="mailto:***@oracle.com">***@oracle.com</a> wrote: 
</div>
<blockquote type="cite"
cite="mid:5897ce86-dda6-dd44-ee53-***@oracle.com">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:
 
<blockquote type="cite">Hi all,
 
 
Please review the fix for
 
<a class="moz-txt-link-freetext" href="https://bugs.openjdk.java.net/browse/JDK-8195639">https://bugs.openjdk.java.net/browse/JDK-8195639</a>
 
webrev:
 
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~amenkov/popframe009/webrev.01/">http://cr.openjdk.java.net/~amenkov/popframe009/webrev.01/</a>
 
 
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
 
</blockquote>
 
</blockquote>
 
</body>
</html>

Alex Menkov

2018-11-27 21:01:14 UTC

Hi Dean,

Thank you for the feedback and for the comments in Jira.

Post by d***@oracle.com
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

Post by s***@oracle.com
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
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;
 }

Post by d***@oracle.com
So I think the fix needs to be in code like

SharedRuntime::resolve_static_call_C(),

Post by d***@oracle.com
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

Post by d***@oracle.com
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

Post by Alex Menkov
Hi all,
Please review the fix for
https://bugs.openjdk.java.net/browse/JDK-8195639
http://cr.openjdk.java.net/~amenkov/popframe009/webrev.01/
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

s***@oracle.com

2018-11-27 22:19:04 UTC

Hi Dean,

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

Hi Dean,
Thank you for the feedback and for the comments in Jira.

Post by d***@oracle.com
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

Post by s***@oracle.com
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
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

Post by d***@oracle.com
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

Post by Alex Menkov
Hi all,
Please review the fix for
https://bugs.openjdk.java.net/browse/JDK-8195639
http://cr.openjdk.java.net/~amenkov/popframe009/webrev.01/
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

d***@oracle.com

2018-11-27 22:50:05 UTC

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

Post by s***@oracle.com
Hi Dean,

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.

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

Hi Dean,
Thank you for the feedback and for the comments in Jira.

Post by d***@oracle.com
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

Post by s***@oracle.com
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
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

Post by d***@oracle.com
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

Post by Alex Menkov
Hi all,
Please review the fix for
https://bugs.openjdk.java.net/browse/JDK-8195639
http://cr.openjdk.java.net/~amenkov/popframe009/webrev.01/
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

s***@oracle.com

2018-11-27 23:38:20 UTC

Good summary with a list of solutions below!

Post by d***@oracle.com
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.

I doubt, the Solution 1 penalizes other threads.
First, it is an important pretty frequent/common case that allows to
save on compilations.
Second, there is no point to compile methods on a thread executed in
interp_only_mode.
Also, there is no big advantage to compile methods even for other threads
when an active debugging (e.g. single stepping) is in progress.
Third, this would make it consistent with what the the interpreter is doing.

So, I think, the Solution 1 is needed independently of other solutions
we take.
My suggestion is to apply it in the JDK-8195639 bug fix.

Post by d***@oracle.com
Solutions 1 and 2 allow the thread to resume in a different method
(the callee method), causing other problems.

Agreed.

Post by d***@oracle.com
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.

I agree in general.
This solution has some risks involved as it impacts a lot of code
including platform-independent.
It is also going to fix the other bugs: JDK-8195635, JDK-8214093,
JDK-8207013 (not sure about all of them yet).

My suggestion is to pursue it as the JDK-8195635 fix.
Could you, please, confirm if works for you?

Also, we have no consensus yet on the Solution 1.

Thanks,
Serguei

Post by s***@oracle.com
Hi Dean,

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.

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

Hi Dean,
Thank you for the feedback and for the comments in Jira.

Post by d***@oracle.com
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

Post by s***@oracle.com
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
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

Post by d***@oracle.com
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

Post by Alex Menkov
Hi all,
Please review the fix for
https://bugs.openjdk.java.net/browse/JDK-8195639
http://cr.openjdk.java.net/~amenkov/popframe009/webrev.01/
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

s***@oracle.com

2018-11-27 23:47:22 UTC

Post by s***@oracle.com
Good summary with a list of solutions below!

I doubt, the Solution 1 penalizes other threads.
First, it is an important pretty frequent/common case that allows to
save on compilations.
Second, there is no point to compile methods on a thread executed in
interp_only_mode.
Also, there is no big advantage to compile methods even for other threads
when an active debugging (e.g. single stepping) is in progress.
Third, this would make it consistent with what the the interpreter is doing.
So, I think, the Solution 1 is needed independently of other solutions
we take.
My suggestion is to apply it in the JDK-8195639 bug fix.

Post by d***@oracle.com
Solutions 1 and 2 allow the thread to resume in a different method
(the callee method), causing other problems.

Agreed.

I agree in general.
This solution has some risks involved as it impacts a lot of code
including platform-independent.

A correction: I wanted to say "platform-dependent".

Thanks,
Serguei

Post by s***@oracle.com
It is also going to fix the other bugs: JDK-8195635, JDK-8214093,
JDK-8207013 (not sure about all of them yet).
My suggestion is to pursue it as the JDK-8195635 fix.
Could you, please, confirm if works for you?
Also, we have no consensus yet on the Solution 1.
Thanks,
Serguei

Post by s***@oracle.com
Hi Dean,

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.

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

Hi Dean,
Thank you for the feedback and for the comments in Jira.

Post by d***@oracle.com
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

Post by s***@oracle.com
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
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

Post by d***@oracle.com
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

Post by Alex Menkov
Hi all,
Please review the fix for
https://bugs.openjdk.java.net/browse/JDK-8195639
http://cr.openjdk.java.net/~amenkov/popframe009/webrev.01/
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

d***@oracle.com

2018-11-28 09:07:10 UTC

OK, reconsidering solution 1, I have a couple more questions:

The bug report says the problem is reproducible with -Xcomp, but I don't
see the policy()->event() path taken with -Xcomp. Instead, -Xcomp uses
CompilationPolicy::compile_if_required, so I don't see how Solution 1
fixes the problem with -Xcomp.

Post by Alex Menkov
CompilerRuntime always calls policy()->event with CompLevel ==

CompLevel_aot.

As far as I can tell, CompLevel_aot is only used when there is active
AOT code. Is this an AOT-specific problem? I would expect there to be
a problem with c1 as well.

dl

Post by Alex Menkov
Good summary with a list of solutions below!

I doubt, the Solution 1 penalizes other threads.
First, it is an important pretty frequent/common case that allows to
save on compilations.
Second, there is no point to compile methods on a thread executed in
interp_only_mode.
Also, there is no big advantage to compile methods even for other threads
when an active debugging (e.g. single stepping) is in progress.
Third, this would make it consistent with what the the interpreter is doing.
So, I think, the Solution 1 is needed independently of other solutions
we take.
My suggestion is to apply it in the JDK-8195639 bug fix.

Post by d***@oracle.com
Solutions 1 and 2 allow the thread to resume in a different method
(the callee method), causing other problems.

Agreed.

I agree in general.
This solution has some risks involved as it impacts a lot of code
including platform-independent.
It is also going to fix the other bugs: JDK-8195635, JDK-8214093,
JDK-8207013 (not sure about all of them yet).
My suggestion is to pursue it as the JDK-8195635 fix.
Could you, please, confirm if works for you?
Also, we have no consensus yet on the Solution 1.
Thanks,
Serguei

Post by s***@oracle.com
Hi Dean,

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.

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

Hi Dean,
Thank you for the feedback and for the comments in Jira.

Post by d***@oracle.com
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

Post by s***@oracle.com
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
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

Post by d***@oracle.com
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

Post by Alex Menkov
Hi all,
Please review the fix for
https://bugs.openjdk.java.net/browse/JDK-8195639
http://cr.openjdk.java.net/~amenkov/popframe009/webrev.01/
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

s***@oracle.com

2018-11-28 09:27:03 UTC

Post by d***@oracle.com
The bug report says the problem is reproducible with -Xcomp, but I
don't see the policy()->event() path taken with -Xcomp. Instead,
-Xcomp uses CompilationPolicy::compile_if_required, so I don't see how
Solution 1 fixes the problem with -Xcomp.

Post by Alex Menkov
CompilerRuntime always calls policy()->event with CompLevel ==

CompLevel_aot.
As far as I can tell, CompLevel_aot is only used when there is active
AOT code. Is this an AOT-specific problem?

Tracing showed that the CompLevel_aot is always used for JVMCI which is
pretty confusing.
Most likely, it is because now the AOT is enabled by default (is it
correct?).
I'm not sure it actually has anything to do with the AOT mode.

Post by d***@oracle.com
I would expect there to be a problem with c1 as well.

Not sure about the c1.
I hope, Alex could comment on this tomorrow.

Thanks,
Serguei

Post by Alex Menkov
Good summary with a list of solutions below!

I doubt, the Solution 1 penalizes other threads.
First, it is an important pretty frequent/common case that allows to
save on compilations.
Second, there is no point to compile methods on a thread executed in
interp_only_mode.
Also, there is no big advantage to compile methods even for other threads
when an active debugging (e.g. single stepping) is in progress.
Third, this would make it consistent with what the the interpreter is doing.
So, I think, the Solution 1 is needed independently of other
solutions we take.
My suggestion is to apply it in the JDK-8195639 bug fix.

Post by d***@oracle.com
Solutions 1 and 2 allow the thread to resume in a different method
(the callee method), causing other problems.

Agreed.

Post by d***@oracle.com
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.

I agree in general.
This solution has some risks involved as it impacts a lot of code
including platform-independent.
It is also going to fix the other bugs: JDK-8195635, JDK-8214093,
JDK-8207013 (not sure about all of them yet).
My suggestion is to pursue it as the JDK-8195635 fix.
Could you, please, confirm if works for you?
Also, we have no consensus yet on the Solution 1.
Thanks,
Serguei

Post by s***@oracle.com
Hi Dean,

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

Hi Dean,
Thank you for the feedback and for the comments in Jira.

Post by d***@oracle.com
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

Post by s***@oracle.com
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
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

Post by d***@oracle.com
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

Post by Alex Menkov
Hi all,
Please review the fix for
https://bugs.openjdk.java.net/browse/JDK-8195639
http://cr.openjdk.java.net/~amenkov/popframe009/webrev.01/
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

Alex Menkov

2018-11-28 20:36:48 UTC

Hi guys,

I was able to reproduce the issue only with Graal + -Xcomp and only with
debug build (I tried Graal + -Xint, Graal without Xcomp/Xint, just
-Xcomp (i.e. C1 is used)).

--alex

Post by Alex Menkov
CompilerRuntime always calls policy()->event with CompLevel ==

CompLevel_aot.
As far as I can tell, CompLevel_aot is only used when there is active
AOT code. Is this an AOT-specific problem?

Post by d***@oracle.com
I would expect there to be a problem with c1 as well.

Not sure about the c1.
I hope, Alex could comment on this tomorrow.
Thanks,
Serguei

Post by Alex Menkov
Good summary with a list of solutions below!

I doubt, the Solution 1 penalizes other threads.
First, it is an important pretty frequent/common case that allows to
save on compilations.
Second, there is no point to compile methods on a thread executed in
interp_only_mode.
Also, there is no big advantage to compile methods even for other threads
when an active debugging (e.g. single stepping) is in progress.
Third, this would make it consistent with what the the interpreter is doing.
So, I think, the Solution 1 is needed independently of other
solutions we take.
My suggestion is to apply it in the JDK-8195639 bug fix.

Post by d***@oracle.com
Solutions 1 and 2 allow the thread to resume in a different method
(the callee method), causing other problems.

Agreed.

Post by d***@oracle.com
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.

I agree in general.
This solution has some risks involved as it impacts a lot of code
including platform-independent.
It is also going to fix the other bugs: JDK-8195635, JDK-8214093,
JDK-8207013 (not sure about all of them yet).
My suggestion is to pursue it as the JDK-8195635 fix.
Could you, please, confirm if works for you?
Also, we have no consensus yet on the Solution 1.
Thanks,
Serguei

Post by s***@oracle.com
Hi Dean,

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

Hi Dean,
Thank you for the feedback and for the comments in Jira.

Post by d***@oracle.com
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

Post by s***@oracle.com
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
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

Post by d***@oracle.com
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

Post by Alex Menkov
Hi all,
Please review the fix for
https://bugs.openjdk.java.net/browse/JDK-8195639
http://cr.openjdk.java.net/~amenkov/popframe009/webrev.01/
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

d***@oracle.com

2018-11-28 21:43:31 UTC

I guess even with -Xcomp, there are still compiles triggered by profiled
code, otherwise I don't see why changing policy()->event() helps.

Another problem I see with this change is that it will make the real
problem harder to reproduce.

dl

Post by Alex Menkov
Hi guys,
I was able to reproduce the issue only with Graal + -Xcomp and only
with debug build (I tried Graal + -Xint, Graal without Xcomp/Xint,
just -Xcomp (i.e. C1 is used)).
--alex

Post by Alex Menkov
CompilerRuntime always calls policy()->event with CompLevel ==

CompLevel_aot.
As far as I can tell, CompLevel_aot is only used when there is
active AOT code. Is this an AOT-specific problem?

Tracing showed that the CompLevel_aot is always used for JVMCI which
is pretty confusing.
Most likely, it is because now the AOT is enabled by default (is it
correct?).
I'm not sure it actually has anything to do with the AOT mode.

Post by d***@oracle.com
I would expect there to be a problem with c1 as well.

Not sure about the c1.
I hope, Alex could comment on this tomorrow.
Thanks,
Serguei

Post by Alex Menkov
Good summary with a list of solutions below!

Post by d***@oracle.com
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.

I doubt, the Solution 1 penalizes other threads.
First, it is an important pretty frequent/common case that allows
to save on compilations.
Second, there is no point to compile methods on a thread executed
in interp_only_mode.
Also, there is no big advantage to compile methods even for other threads
when an active debugging (e.g. single stepping) is in progress.
Third, this would make it consistent with what the the interpreter is doing.
So, I think, the Solution 1 is needed independently of other
solutions we take.
My suggestion is to apply it in the JDK-8195639 bug fix.

Post by d***@oracle.com
Solutions 1 and 2 allow the thread to resume in a different method
(the callee method), causing other problems.

Agreed.

Post by d***@oracle.com
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.

I agree in general.
This solution has some risks involved as it impacts a lot of code
including platform-independent.
It is also going to fix the other bugs: JDK-8195635, JDK-8214093,
JDK-8207013 (not sure about all of them yet).
My suggestion is to pursue it as the JDK-8195635 fix.
Could you, please, confirm if works for you?
Also, we have no consensus yet on the Solution 1.
Thanks,
Serguei

Post by s***@oracle.com
Hi Dean,

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

Hi Dean,
Thank you for the feedback and for the comments in Jira.

Post by d***@oracle.com
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

Post by s***@oracle.com
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
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

Post by d***@oracle.com
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

Post by Alex Menkov
Hi all,
Please review the fix for
https://bugs.openjdk.java.net/browse/JDK-8195639
http://cr.openjdk.java.net/~amenkov/popframe009/webrev.01/
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

s***@oracle.com

2018-11-28 21:52:06 UTC

Post by d***@oracle.com
I guess even with -Xcomp, there are still compiles triggered by
profiled code, otherwise I don't see why changing policy()->event()
helps.
Another problem I see with this change is that it will make the real
problem harder to reproduce.

True.
But on the other hand the tests become more stable. :)

Thanks,
Serguei

Post by Alex Menkov
CompilerRuntime always calls policy()->event with CompLevel ==

CompLevel_aot.
As far as I can tell, CompLevel_aot is only used when there is
active AOT code. Is this an AOT-specific problem?

Tracing showed that the CompLevel_aot is always used for JVMCI which
is pretty confusing.
Most likely, it is because now the AOT is enabled by default (is it
correct?).
I'm not sure it actually has anything to do with the AOT mode.

Post by d***@oracle.com
I would expect there to be a problem with c1 as well.

Not sure about the c1.
I hope, Alex could comment on this tomorrow.
Thanks,
Serguei

Post by Alex Menkov
Good summary with a list of solutions below!

Post by d***@oracle.com
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.

I doubt, the Solution 1 penalizes other threads.
First, it is an important pretty frequent/common case that allows
to save on compilations.
Second, there is no point to compile methods on a thread executed
in interp_only_mode.
Also, there is no big advantage to compile methods even for other threads
when an active debugging (e.g. single stepping) is in progress.
Third, this would make it consistent with what the the interpreter is doing.
So, I think, the Solution 1 is needed independently of other
solutions we take.
My suggestion is to apply it in the JDK-8195639 bug fix.

Post by d***@oracle.com
Solutions 1 and 2 allow the thread to resume in a different
method (the callee method), causing other problems.

Agreed.

Post by d***@oracle.com
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.

I agree in general.
This solution has some risks involved as it impacts a lot of code
including platform-independent.
It is also going to fix the other bugs: JDK-8195635, JDK-8214093,
JDK-8207013 (not sure about all of them yet).
My suggestion is to pursue it as the JDK-8195635 fix.
Could you, please, confirm if works for you?
Also, we have no consensus yet on the Solution 1.
Thanks,
Serguei

Post by s***@oracle.com
Hi Dean,

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

Hi Dean,
Thank you for the feedback and for the comments in Jira.