[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: Re: RFR: 8041934 com/sun/jdi/RepStep.java fails on all platforms with assert(_cur_stack_depth == cou
From: Christian Thalinger <christian.thalinger () oracle ! com>
Date: 2014-05-14 18:30:54
Message-ID: ECB36303-8D90-4812-ADED-09ABDF3E0DA3 () oracle ! com
[Download RAW message or body]
Looks good.
On May 13, 2014, at 11:58 PM, Staffan Larsen <staffan.larsen@oracle.com> wrote:
> Thanks Christian,
>
> I will make the change below before I push.
>
> /Staffan
>
>
> diff --git a/src/cpu/x86/vm/sharedRuntime_x86_32.cpp \
> b/src/cpu/x86/vm/sharedRuntime_x86_32.cpp
> --- a/src/cpu/x86/vm/sharedRuntime_x86_32.cpp
> +++ b/src/cpu/x86/vm/sharedRuntime_x86_32.cpp
> @@ -2248,7 +2248,7 @@
> // if we are now in interp_only_mode and in that case we do the jvmti
> // callback.
> Label skip_jvmti_method_exit;
> - __ cmpb(Address(thread, JavaThread::interp_only_mode_offset()), 0);
> + __ cmpl(Address(thread, JavaThread::interp_only_mode_offset()), 0);
> __ jcc(Assembler::zero, skip_jvmti_method_exit, true);
>
> save_native_result(masm, ret_type, stack_slots);
> diff --git a/src/cpu/x86/vm/sharedRuntime_x86_64.cpp \
> b/src/cpu/x86/vm/sharedRuntime_x86_64.cpp
> --- a/src/cpu/x86/vm/sharedRuntime_x86_64.cpp
> +++ b/src/cpu/x86/vm/sharedRuntime_x86_64.cpp
> @@ -2495,7 +2495,7 @@
> // if we are now in interp_only_mode and in that case we do the jvmti
> // callback.
> Label skip_jvmti_method_exit;
> - __ cmpb(Address(r15_thread, JavaThread::interp_only_mode_offset()), 0);
> + __ cmpl(Address(r15_thread, JavaThread::interp_only_mode_offset()), 0);
> __ jcc(Assembler::zero, skip_jvmti_method_exit, true);
>
> save_native_result(masm, ret_type, stack_slots);
>
>
> On 14 maj 2014, at 00:21, Christian Thalinger <christian.thalinger@oracle.com> \
> wrote:
> > Since:
> >
> > int _interp_only_mode;
> >
> > is an int field I would prefer to actually read the value as an int instead of \
> > just a byte on x86: + __ cmpb(Address(r15_thread, \
> > JavaThread::interp_only_mode_offset()), 0); Otherwise this looks good.
> >
> > On May 13, 2014, at 11:30 AM, Staffan Larsen <staffan.larsen@oracle.com> wrote:
> >
> > >
> > > On 13 maj 2014, at 18:53, Daniel D. Daugherty <daniel.daugherty@oracle.com> \
> > > wrote:
> > > > > new webrev is here: http://cr.openjdk.java.net/~sla/8041934/webrev.02/
> > > >
> > > > src/share/vm/runtime/sharedRuntime.hpp
> > > > No comments.
> > > >
> > > > src/share/vm/runtime/sharedRuntime.cpp
> > > > No comments.
> > > >
> > > > src/cpu/sparc/vm/sharedRuntime_sparc.cpp
> > > > No comments.
> > > >
> > > > src/cpu/x86/vm/sharedRuntime_x86_32.cpp
> > > > No comments.
> > > >
> > > > src/cpu/x86/vm/sharedRuntime_x86_64.cpp
> > > > No comments.
> > > >
> > > > On the switch from call_VM_leaf() -> call_VM(), I looked through all
> > > > the mentions of the string call_VM in the three src/cpu files. Your
> > > > change adds the first call_VM() use in all three files and the other
> > > > places that mention the string "call_VM" seem to have gone through
> > > > a great deal of trouble not to use call_VM() directly. I have no
> > > > specific thing I think is wrong, but I find this data worrisome…
> > >
> > > Yes, it would be great if someone from the compiler team could review this, \
> > > too.
> > > Thanks,
> > > /Staffan
> > >
> > > >
> > > > Thumbs up!
> > > >
> > > > Dan
> > > >
> > > >
> > > > On 5/13/14 3:20 AM, Staffan Larsen wrote:
> > > > >
> > > > > On 9 maj 2014, at 20:18, serguei.spitsyn@oracle.com wrote:
> > > > >
> > > > > > Staffan,
> > > > > >
> > > > > > This is important discovery, thanks!
> > > > > > The fix looks good to me.
> > > > > > One question below.
> > > > > >
> > > > > > Thanks,
> > > > > > Serguei
> > > > > >
> > > > > >
> > > > > > On 5/9/14 3:47 AM, Staffan Larsen wrote:
> > > > > > > On 8 maj 2014, at 19:05, Daniel D. Daugherty \
> > > > > > > <daniel.daugherty@oracle.com> wrote:
> > > > > > > > > webrev: http://cr.openjdk.java.net/~sla/8041934/webrev.00/
> > > > > > > > src/share/vm/runtime/sharedRuntime.hpp
> > > > > > > > No comments.
> > > > > > > >
> > > > > > > > src/share/vm/runtime/sharedRuntime.cpp
> > > > > > > > line 994: JRT_LEAF(int, SharedRuntime::jvmti_method_exit(
> > > > > > > > I'm not sure that JRT_LEAF is right. I would think that
> > > > > > > > JvmtiExport::post_method_exit() would have to grab one or
> > > > > > > > more locks with all the state checks it has to make...
> > > > > > > >
> > > > > > > > For reference, InterpreterRuntime::post_method_exit()
> > > > > > > > is a wrapper around JvmtiExport::post_method_exit()
> > > > > > > > and it is IRT_ENTRY instead of IRT_LEAF.
> > > > > > > Good catch!
> > > > > >
> > > > > > Q: Is correct to use call_VM_leaf (vs call_VM ) in the \
> > > > > > sharedRuntime_<arch>.cpp ?
> > > > > > + __ call_VM_leaf(
> > > > > > + CAST_FROM_FN_PTR(address, SharedRuntime::jvmti_method_exit),
> > > > > > + thread, rax);
> > > > >
> > > > > That is another good catch! It should probably be call_VM as you note. The \
> > > > > reason for all these leaf references is because we used the dtrace probe as \
> > > > > a template - obviously without fully understanding what we did :-/
> > > > > I have changed to code to use call_VM instead. This also required a change \
> > > > > from jccb to jcc for the jump (which is now longer than an 8-bit offset).
> > > > > new webrev is here: http://cr.openjdk.java.net/~sla/8041934/webrev.02/
> > > > >
> > > > > Thanks,
> > > > > /Staffan
> > > > >
> > > > >
> > > > > >
> > > > > > > > src/cpu/sparc/vm/sharedRuntime_sparc.cpp
> > > > > > > > No comments.
> > > > > > > >
> > > > > > > > src/cpu/x86/vm/sharedRuntime_x86_32.cpp
> > > > > > > > No comments.
> > > > > > > >
> > > > > > > > src/cpu/x86/vm/sharedRuntime_x86_64.cpp
> > > > > > > > No comments.
> > > > > > > >
> > > > > > > > I'm guessing that PPC has the same issue, but I'm presuming
> > > > > > > > that someone else (Vladimir?) will handle that…
> > > > > > > Yes, I was hoping that I could file a follow-up bug for the platforms I \
> > > > > > > didn’t know how to fix.
> > > > > > > Updated review: http://cr.openjdk.java.net/~sla/8041934/webrev.01/
> > > > > > >
> > > > > > > Thanks,
> > > > > > > /Staffan
> > > > > > >
> > > > > > > > Dan
> > > > > > > >
> > > > > > > >
> > > > > > > > On 5/8/14 12:06 AM, Staffan Larsen wrote:
> > > > > > > > > All,
> > > > > > > > >
> > > > > > > > > This is a fix for an assert in JVMTI that verifies that JVMTI’s \
> > > > > > > > > internal notion of the number of frames on the stack is correct.
> > > > > > > > > When running in -Xcomp mode and enable single-stepping (or \
> > > > > > > > > method_entry/exit) we will revert all frames on the stack to be run \
> > > > > > > > > by the interpreter. Only the interpreter can send single-step and \
> > > > > > > > > method_entry/exit. However, if one of the frames is a native \
> > > > > > > > > wrapper, that frame will not be reverted (presumably because we \
> > > > > > > > > don't know how to do that). This will cause us to miss a \
> > > > > > > > > method_exit event when that native frame is popped. This in turn \
> > > > > > > > > will mess up the logic in JVMTI that keeps track of the number of \
> > > > > > > > > frames currently on the stack (see \
> > > > > > > > > JvmtiThreadState::_cur_stack_depth) and will trigger the assert.
> > > > > > > > > The proposed solution is to include a method_exit event in the \
> > > > > > > > > native wrapper frame if interpreted mode has been enabled. This \
> > > > > > > > > needs updates to SharedRuntime::generate_native_wrapper() for all \
> > > > > > > > > platforms.
> > > > > > > > > Kudos to Rickard for helping me write the code.
> > > > > > > > >
> > > > > > > > > webrev: http://cr.openjdk.java.net/~sla/8041934/webrev.00/
> > > > > > > > > bug: https://bugs.openjdk.java.net/browse/JDK-8041934
> > > > > > > > >
> > > > > > > > > The fix has been verified by running the failing test in JPRT with \
> > > > > > > > > -Xcomp.
> > > > > > > > > Thanks,
> > > > > > > > > /Staffan
> > > > > >
> > > > >
> > > >
> > >
> >
>
[Attachment #3 (unknown)]
<html><head><meta http-equiv="Content-Type" content="text/html \
charset=windows-1252"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: \
space; -webkit-line-break: after-white-space;">Looks good.<div><br><div \
style=""><div>On May 13, 2014, at 11:58 PM, Staffan Larsen <<a \
href="mailto:staffan.larsen@oracle.com">staffan.larsen@oracle.com</a>> \
wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><meta \
http-equiv="Content-Type" content="text/html charset=windows-1252"><div \
style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: \
after-white-space;">Thanks Christian,<div><br></div><div>I will make the change below \
before I push.</div><div><br></div><div>/Staffan</div><div><br></div><div><br></div><div><div>diff \
--git a/src/cpu/x86/vm/sharedRuntime_x86_32.cpp \
b/src/cpu/x86/vm/sharedRuntime_x86_32.cpp</div><div>--- \
a/src/cpu/x86/vm/sharedRuntime_x86_32.cpp</div><div>+++ \
b/src/cpu/x86/vm/sharedRuntime_x86_32.cpp</div><div>@@ -2248,7 +2248,7 \
@@</div><div> // if we are now in interp_only_mode and in that \
case we do the jvmti</div><div> // callback.</div><div> \
Label skip_jvmti_method_exit;</div><div>- __ \
cmpb(Address(thread, JavaThread::interp_only_mode_offset()), 0);</div><div>+ \
__ cmpl(Address(thread, JavaThread::interp_only_mode_offset()), \
0);</div><div> __ jcc(Assembler::zero, skip_jvmti_method_exit, \
true);</div><div><br></div><div> save_native_result(masm, \
ret_type, stack_slots);</div><div>diff --git \
a/src/cpu/x86/vm/sharedRuntime_x86_64.cpp \
b/src/cpu/x86/vm/sharedRuntime_x86_64.cpp</div><div>--- \
a/src/cpu/x86/vm/sharedRuntime_x86_64.cpp</div><div>+++ \
b/src/cpu/x86/vm/sharedRuntime_x86_64.cpp</div><div>@@ -2495,7 +2495,7 \
@@</div><div> // if we are now in interp_only_mode and in that \
case we do the jvmti</div><div> // callback.</div><div> \
Label skip_jvmti_method_exit;</div><div>- __ \
cmpb(Address(r15_thread, JavaThread::interp_only_mode_offset()), 0);</div><div>+ \
__ cmpl(Address(r15_thread, JavaThread::interp_only_mode_offset()), \
0);</div><div> __ jcc(Assembler::zero, skip_jvmti_method_exit, \
true);</div><div><br></div><div> save_native_result(masm, \
ret_type, stack_slots);</div><div><br></div><div><br></div><div><div>On 14 maj 2014, \
at 00:21, Christian Thalinger <<a \
href="mailto:christian.thalinger@oracle.com">christian.thalinger@oracle.com</a>> \
wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><meta \
http-equiv="Content-Type" content="text/html charset=windows-1252"><div \
style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: \
after-white-space;">Since:<div><br></div><div><div style="margin: 0px; font-size: \
11px; font-family: Monaco;"> <span style="color: #931a68">int</span> \
<span style="color: \
#0326cc">_interp_only_mode</span>;</div><div><br></div><div>is an int field I would \
prefer to actually read the value as an int instead of just a byte on \
x86:</div><div><pre><span class="new" style="color: blue;">+ __ \
cmpb(Address(r15_thread, JavaThread::interp_only_mode_offset()), 0);</span> \
</pre></div><div>Otherwise this looks good.</div><div><div><br></div><div>On May 13, \
2014, at 11:30 AM, Staffan Larsen <<a \
href="mailto:staffan.larsen@oracle.com">staffan.larsen@oracle.com</a>> \
wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><meta \
http-equiv="Content-Type" content="text/html charset=windows-1252"><div \
style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: \
after-white-space;"><br><div><div>On 13 maj 2014, at 18:53, Daniel D. Daugherty \
<<a href="mailto:daniel.daugherty@oracle.com">daniel.daugherty@oracle.com</a>> \
wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite">
<meta content="text/html; charset=windows-1252" http-equiv="Content-Type">
<div bgcolor="#FFFFFF" text="#000000">
<tt>> new webrev is here:
<a class="moz-txt-link-freetext" \
href="http://cr.openjdk.java.net/~sla/8041934/webrev.02/">http://cr.openjdk.java.net/~sla/8041934/webrev.02/</a><br>
<br>
</tt><tt>src/share/vm/runtime/sharedRuntime.hpp<br>
</tt><tt> No comments.<br>
<br>
</tt><tt><tt><tt>src/share/vm/runtime/sharedRuntime.cpp<br>
No comments.<br>
<br>
</tt></tt>src/cpu/sparc/vm/sharedRuntime_sparc.cpp<br>
No comments.<br>
<br>
src/cpu/x86/vm/sharedRuntime_x86_32.cpp<br>
No comments.<br>
<br>
src/cpu/x86/vm/sharedRuntime_x86_64.cpp<br>
No comments.<br>
<br>
On the switch from call_VM_leaf() -> call_VM(), I looked
through all<br>
the mentions of the string call_VM in the three src/cpu files.
Your<br>
change adds the first call_VM() use in all three files and the
other<br>
places that mention the string "call_VM" seem to have gone through<br>
a great deal of trouble not to use call_VM() directly. I have no<br>
specific thing I think is wrong, but I find this data \
worrisome…</tt></div></blockquote><div><br></div><div>Yes, it would be great if \
someone from the compiler team could review this, \
too.</div><div><br></div><div>Thanks,</div><div>/Staffan</div><br><blockquote \
type="cite"><div bgcolor="#FFFFFF" text="#000000"><tt> <br>
Thumbs up!<br>
<br>
Dan<br>
<br>
<br>
</tt>
<div class="moz-cite-prefix">On 5/13/14 3:20 AM, Staffan Larsen
wrote:<br>
</div>
<blockquote cite="mid:C6B702AE-DCB5-4E11-8879-D8540E3E969A@oracle.com" \
type="cite"> <meta http-equiv="Content-Type" content="text/html;
charset=windows-1252">
<br>
<div>
<div>On 9 maj 2014, at 20:18, <a moz-do-not-send="true" \
href="mailto:serguei.spitsyn@oracle.com">serguei.spitsyn@oracle.com</a> wrote:</div>
<br class="Apple-interchange-newline">
<blockquote type="cite">
<meta content="text/html; charset=windows-1252" http-equiv="Content-Type">
<div text="#000000" bgcolor="#FFFFFF">
<div class="moz-cite-prefix">Staffan,<br>
<br>
This is important discovery, thanks!<br>
The fix looks good to me.<br>
One question below.<br>
<br>
Thanks,<br>
Serguei<br>
<br>
<br>
On 5/9/14 3:47 AM, Staffan Larsen wrote:<br>
</div>
<blockquote cite="mid:2E48F010-CE96-4676-8763-E801CBCF5D00@oracle.com" \
type="cite"> <pre wrap="">On 8 maj 2014, at 19:05, Daniel D. Daugherty <a \
moz-do-not-send="true" class="moz-txt-link-rfc2396E" \
href="mailto:daniel.daugherty@oracle.com"><daniel.daugherty@oracle.com></a> \
wrote:
</pre>
<blockquote type="cite">
<blockquote type="cite">
<pre wrap="">webrev: <a moz-do-not-send="true" \
class="moz-txt-link-freetext" \
href="http://cr.openjdk.java.net/%7Esla/8041934/webrev.00/">http://cr.openjdk.java.net/~sla/8041934/webrev.00/</a>
</pre>
</blockquote>
<pre wrap="">src/share/vm/runtime/sharedRuntime.hpp
No comments.
src/share/vm/runtime/sharedRuntime.cpp
line 994: JRT_LEAF(int, SharedRuntime::jvmti_method_exit(
I'm not sure that JRT_LEAF is right. I would think that
JvmtiExport::post_method_exit() would have to grab one or
more locks with all the state checks it has to make...
For reference, InterpreterRuntime::post_method_exit()
is a wrapper around JvmtiExport::post_method_exit()
and it is IRT_ENTRY instead of IRT_LEAF.
</pre>
</blockquote>
<pre wrap="">Good catch!</pre>
</blockquote>
<br>
Q: Is correct to use <span class="new">call_VM_leaf</span>
<meta http-equiv="content-type" content="text/html;
charset=windows-1252">
(vs <span class="new">call_VM</span>
<meta http-equiv="content-type" content="text/html;
charset=windows-1252">
) in the sharedRuntime_<arch>.cpp ?<br>
<br>
<meta http-equiv="content-type" content="text/html;
charset=windows-1252">
<pre><span class="new">+ __ call_VM_leaf(</span>
<span class="new">+ CAST_FROM_FN_PTR(address, \
SharedRuntime::jvmti_method_exit),</span> <span class="new">+ thread, \
rax);</span></pre> </div>
</blockquote>
<div><br>
</div>
<div>That is another good catch! It should probably be call_VM
as you note. The reason for all these leaf references is
because we used the dtrace probe as a template - obviously
without fully understanding what we did :-/</div>
<div><br>
</div>
<div>I have changed to code to use call_VM instead. This also
required a change from jccb to jcc for the jump (which is now
longer than an 8-bit offset). </div>
<div><br>
</div>
<div>new webrev is here: <a moz-do-not-send="true" \
href="http://cr.openjdk.java.net/%7Esla/8041934/webrev.02/">http://cr.openjdk.java.net/~sla/8041934/webrev.02/</a></div>
<div><br>
</div>
<div>
<div>Thanks,</div>
<div>/Staffan</div>
<div><br>
</div>
</div>
<div><br>
</div>
<blockquote type="cite">
<div text="#000000" bgcolor="#FFFFFF"><br>
<blockquote cite="mid:2E48F010-CE96-4676-8763-E801CBCF5D00@oracle.com" \
type="cite"> <blockquote type="cite">
<pre wrap="">src/cpu/sparc/vm/sharedRuntime_sparc.cpp
No comments.
src/cpu/x86/vm/sharedRuntime_x86_32.cpp
No comments.
src/cpu/x86/vm/sharedRuntime_x86_64.cpp
No comments.
I'm guessing that PPC has the same issue, but I'm presuming
that someone else (Vladimir?) will handle that…
</pre>
</blockquote>
<pre wrap="">Yes, I was hoping that I could file a follow-up bug for \
the platforms I didn’t know how to fix.
Updated review: <a moz-do-not-send="true" class="moz-txt-link-freetext" \
href="http://cr.openjdk.java.net/%7Esla/8041934/webrev.01/">http://cr.openjdk.java.net/~sla/8041934/webrev.01/</a>
Thanks,
/Staffan
</pre>
<blockquote type="cite">
<pre wrap="">Dan
On 5/8/14 12:06 AM, Staffan Larsen wrote:
</pre>
<blockquote type="cite">
<pre wrap="">All,
This is a fix for an assert in JVMTI that verifies that JVMTI’s internal notion of \
the number of frames on the stack is correct.
When running in -Xcomp mode and enable single-stepping (or method_entry/exit) we will \
revert all frames on the stack to be run by the interpreter. Only the interpreter can \
send single-step and method_entry/exit. However, if one of the frames is a native \
wrapper, that frame will not be reverted (presumably because we don't know how to do \
that). This will cause us to miss a method_exit event when that native frame is \
popped. This in turn will mess up the logic in JVMTI that keeps track of the number \
of frames currently on the stack (see JvmtiThreadState::_cur_stack_depth) and will \
trigger the assert.
The proposed solution is to include a method_exit event in the native wrapper frame \
if interpreted mode has been enabled. This needs updates to \
SharedRuntime::generate_native_wrapper() for all platforms.
Kudos to Rickard for helping me write the code.
webrev: <a moz-do-not-send="true" class="moz-txt-link-freetext" \
href="http://cr.openjdk.java.net/%7Esla/8041934/webrev.00/">http://cr.openjdk.java.net/~sla/8041934/webrev.00/</a>
bug: <a moz-do-not-send="true" class="moz-txt-link-freetext" \
href="https://bugs.openjdk.java.net/browse/JDK-8041934">https://bugs.openjdk.java.net/browse/JDK-8041934</a>
The fix has been verified by running the failing test in JPRT with -Xcomp.
Thanks,
/Staffan
</pre>
</blockquote>
</blockquote>
</blockquote>
<br>
</div>
</blockquote>
</div>
<br>
</blockquote>
<br>
</div>
</blockquote></div><br></div></blockquote></div><br></div></div></blockquote></div><br></div></div></blockquote></div><br></div></body></html>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic