[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: Re: 8218401: WRONG_PHASE: vmTestbase/nsk/jvmti test crash
From: Daniil Titov <daniil.x.titov () oracle ! com>
Date: 2019-03-21 17:17:23
Message-ID: CE652D66-D825-4A0D-9620-269C289FAEC4 () oracle ! com
[Download RAW message or body]
Thank you, JC and Serguei, for reviewing this change.
Best regards,
Daniil
From: Jean Christophe Beyler <jcbeyler@google.com>
Date: Thursday, March 21, 2019 at 9:29 AM
To: Daniil Titov <daniil.x.titov@oracle.com>
Subject: Re: FW: 8218401: WRONG_PHASE: vmTestbase/nsk/jvmti test crash
Hi Daniil,
Sorry yes it looks great to me :-)
My only nit would be : "callbacksEnabled" is camel backed and should probably be \
callbacks_enabled for c++ code, but that's a nit ;-)
Jc
On Thu, Mar 21, 2019 at 9:26 AM Daniil Titov <daniil.x.titov@oracle.com> wrote:
Hi JC,
Just wanted to check with you that you are also OK with this version of the fix.
Webrev: http://cr.openjdk.java.net/~dtitov/8218401/webrev.03
Bug : https://bugs.openjdk.java.net/browse/JDK-8218401
Thanks!
--Daniil
On 3/20/19, 4:50 PM, "serguei.spitsyn@oracle.com" <serguei.spitsyn@oracle.com> \
wrote:
Hi Daniil,
You are right, there is a hole for this.
It can be easily fixed if the callbacks check phase at the beginning.
But I don't think this would be simpler than your current fix.
As I already mentioned, I'm Okay with the fix you have now.
I'm still thinking on how to fix this on the JVMTI level.
Thanks,
Serguei
On 3/20/19 16:35, Daniil Titov wrote:
> Hi Serguei,
>
> I could be missing something but as I understood the suggested alternative \
approach was to not introduce the callbacksEnabled flag and instead just unregister \
other callbacks inside VMDeath callback. My concern was the case when one thread is \
on line 1302 (it is going to call a class loaded callback), while another thread just \
entered VMDeath callback. Without callbacksEnabled flag the first thread will enter \
the callback and wait on the raw monitor acquired by VMDeath callback thread, after \
that it will continue and call JVMTI API that might throw WRONG_PHASE error if by \
this time the second thread already returned from the VMDeath callback and changed \
the JVMTI phase. >
> 1277 void JvmtiExport::post_class_load(JavaThread *thread, Klass* klass) {
> 1278 if (JvmtiEnv::get_phase() < JVMTI_PHASE_PRIMORDIAL) {
> 1279 return;
> 1280 }
> 1281 HandleMark hm(thread);
> 1282
> 1283 EVT_TRIG_TRACE(JVMTI_EVENT_CLASS_LOAD, ("[%s] Trg Class Load \
triggered", > 1284 \
JvmtiTrace::safe_get_thread_name(thread))); > 1285 JvmtiThreadState* state = \
thread->jvmti_thread_state(); > 1286 if (state == NULL) {
> 1287 return;
> 1288 }
> 1289 JvmtiEnvThreadStateIterator it(state);
> 1290 for (JvmtiEnvThreadState* ets = it.first(); ets != NULL; ets = \
it.next(ets)) { > 1291 if (ets->is_enabled(JVMTI_EVENT_CLASS_LOAD)) {
> 1292 JvmtiEnv *env = ets->get_env();
> 1293 if (env->phase() == JVMTI_PHASE_PRIMORDIAL) {
> 1294 continue;
> 1295 }
> 1296 EVT_TRACE(JVMTI_EVENT_CLASS_LOAD, ("[%s] Evt Class Load sent \
%s", > 1297 \
JvmtiTrace::safe_get_thread_name(thread), > 1298 \
klass==NULL? "NULL" : klass->external_name() )); > 1299 \
JvmtiClassEventMark jem(thread, klass); > 1300 \
JvmtiJavaThreadEventTransition jet(thread); > 1301 jvmtiEventClassLoad \
callback = env->callbacks()->ClassLoad; > 1302 if (callback != NULL) {
> 1303 (*callback)(env->jvmti_external(), jem.jni_env(), \
jem.jni_thread(), jem.jni_class()); > 1304 }
> 1305 }
> 1306 }
> 1307 }
>
> Thanks!
>
> --Daniil
>
> On 3/20/19, 3:40 PM, "serguei.spitsyn@oracle.com" \
<serguei.spitsyn@oracle.com> wrote: >
> Hi Daniil,
>
> No chance for it if you lock a raw monitor in the event callbacks.
>
> Thanks,
> Serguei
>
> On 3/20/19 14:58, Daniil Titov wrote:
> > Hi Serguei,
> >
> > I think that just disabling event notifications inside VMDeath callback \
still leaves a small window for VMDeath callback being called after the classload \
callback is called (or about being called) but before it enters a raw monitor. Thus \
I decided to follow your original suggestion and restore volatile modifier for \
callbacksEnabled. Please review a new version of the patch. > >
> > Webrev: http://cr.openjdk.java.net/~dtitov/8218401/webrev.03/
> > Bug : https://bugs.openjdk.java.net/browse/JDK-8218401
> >
> > Thanks!
> >
> > Best regards,
> > Daniil
> >
> >
> >
> > From: <serguei.spitsyn@oracle.com>
> > Organization: Oracle Corporation
> > Date: Tuesday, March 19, 2019 at 7:17 PM
> > To: Daniil Titov <daniil.x.titov@oracle.com>, OpenJDK Serviceability \
<serviceability-dev@openjdk.java.net>, Jean Christophe Beyler <jcbeyler@google.com> \
> > Subject: Re: 8218401: WRONG_PHASE: vmTestbase/nsk/jvmti test crash > >
> > Hi Daniil,
> >
> > I'd keep the volatile modifier for callbacksEnabled to disable compiler \
optimizations. > > Otherwise, looks good to me.
> >
> >
> > Another approach could be to disable event notifications in VMDeath \
callback with: > > SetEventNotificationMode(JVMTI_DISABLE, \
JVMTI_EVENT_CLASS_LOAD, NULL); > > SetEventNotificationMode(JVMTI_DISABLE, \
JVMTI_EVENT_BREAKPOINT, NULL); > > . . .
> >
> > Thanks,
> > Serguei
> >
> > On 3/18/19 6:58 PM, Daniil Titov wrote:
> > Hi Serguei and JC,
> >
> > Please review a new version of the fix that locks a monitor across the \
callbacks, as Serguei suggested. > >
> > Webrev: http://cr.openjdk.java.net/~dtitov/8218401/webrev.02/
> > Bug: https://bugs.openjdk.java.net/browse/JDK-8218401
> >
> > Thanks!
> > --Daniil
> >
> >
> > On 3/18/19, 9:47 AM, mailto:serguei.spitsyn@oracle.com \
mailto:serguei.spitsyn@oracle.com wrote: > >
> > Hi Daniil,
> >
> > The JVMTI phase can change in the middle of callback work after the
> > check you added.
> > I'd suggest to lock a raw monitor across the callbacks to make them \
atomic. > >
> > Thank you for taking care about this issue!
> >
> > Thanks,
> > Serguei
> >
> >
> >
> > On 3/15/19 16:08, Daniil Titov wrote:
> > > Please review the change that fixes 3 tests that intermittently \
fail with JVMTI_ERROR_WRONG_PHASE error. > > >
> > > The problem here is that the callbacks these tests enable keep \
processing events and perform JVMTI calls after VM is terminated. The fix makes these \
test listen for VMDeath event and quick return from the callbacks after VMDeath \
event is received. > > >
> > > Webrev: http://cr.openjdk.java.net/~dtitov/8218401/webrev.01/
> > > Bug: https://bugs.openjdk.java.net/browse/JDK-8218401
> > >
> > > Thanks!
> > > -Daniil
> > >
> > >
> >
> >
> >
> >
> >
> >
> >
> >
>
>
>
>
--
Thanks,
Jc
[Attachment #3 (text/html)]
<html xmlns:o="urn:schemas-microsoft-com:office:office" \
xmlns:w="urn:schemas-microsoft-com:office:word" \
xmlns:m="http://schemas.microsoft.com/office/2004/12/omml" \
xmlns="http://www.w3.org/TR/REC-html40"><head><meta http-equiv=Content-Type \
content="text/html; charset=utf-8"><meta name=Generator content="Microsoft Word 15 \
(filtered medium)"><style><!-- /* Font Definitions */
@font-face
{font-family:"Cambria Math";
panose-1:2 4 5 3 5 4 6 3 2 4;}
@font-face
{font-family:Calibri;
panose-1:2 15 5 2 2 2 4 3 2 4;}
/* Style Definitions */
p.MsoNormal, li.MsoNormal, div.MsoNormal
{margin:0in;
margin-bottom:.0001pt;
font-size:11.0pt;
font-family:"Calibri",sans-serif;}
a:link, span.MsoHyperlink
{mso-style-priority:99;
color:blue;
text-decoration:underline;}
a:visited, span.MsoHyperlinkFollowed
{mso-style-priority:99;
color:purple;
text-decoration:underline;}
p.msonormal0, li.msonormal0, div.msonormal0
{mso-style-name:msonormal;
mso-margin-top-alt:auto;
margin-right:0in;
mso-margin-bottom-alt:auto;
margin-left:0in;
font-size:11.0pt;
font-family:"Calibri",sans-serif;}
span.EmailStyle18
{mso-style-type:personal-reply;
font-family:"Calibri",sans-serif;
color:windowtext;}
.MsoChpDefault
{mso-style-type:export-only;
font-size:10.0pt;}
@page WordSection1
{size:8.5in 11.0in;
margin:1.0in 1.0in 1.0in 1.0in;}
div.WordSection1
{page:WordSection1;}
--></style></head><body lang=EN-US link=blue vlink=purple><div class=WordSection1><p \
class=MsoNormal>Thank you, JC and Serguei, for reviewing this \
change.<o:p></o:p></p><p class=MsoNormal><o:p> </o:p></p><p class=MsoNormal>Best \
regards,<o:p></o:p></p><p class=MsoNormal>Daniil<o:p></o:p></p><p \
class=MsoNormal><o:p> </o:p></p><div style='border:none;border-top:solid #B5C4DF \
1.0pt;padding:3.0pt 0in 0in 0in'><p class=MsoNormal><b><span \
style='font-size:12.0pt;color:black'>From: </span></b><span \
style='font-size:12.0pt;color:black'>Jean Christophe Beyler \
<jcbeyler@google.com><br><b>Date: </b>Thursday, March 21, 2019 at 9:29 \
AM<br><b>To: </b>Daniil Titov <daniil.x.titov@oracle.com><br><b>Subject: \
</b>Re: FW: 8218401: WRONG_PHASE: vmTestbase/nsk/jvmti test \
crash<o:p></o:p></span></p></div><div><p \
class=MsoNormal><o:p> </o:p></p></div><div><div><div><p class=MsoNormal>Hi \
Daniil,<o:p></o:p></p><div><p class=MsoNormal><o:p> </o:p></p></div><div><p \
class=MsoNormal>Sorry yes it looks great to me :-)<o:p></o:p></p></div><div><p \
class=MsoNormal><o:p> </o:p></p></div><div><p class=MsoNormal>My only nit would \
be : "callbacksEnabled" is camel backed and should probably \
be callbacks_enabled for c++ code, but that's a nit \
;-)<o:p></o:p></p></div><div><p \
class=MsoNormal>Jc<o:p></o:p></p></div></div></div></div><p \
class=MsoNormal><o:p> </o:p></p><div><div><p class=MsoNormal>On Thu, Mar 21, \
2019 at 9:26 AM Daniil Titov <<a \
href="mailto:daniil.x.titov@oracle.com">daniil.x.titov@oracle.com</a>> \
wrote:<o:p></o:p></p></div><blockquote style='border:none;border-left:solid #CCCCCC \
1.0pt;padding:0in 0in 0in 6.0pt;margin-left:4.8pt;margin-right:0in'><p \
class=MsoNormal style='margin-bottom:12.0pt'>Hi JC,<br><br>Just wanted to check with \
you that you are also OK with this version of the fix.<br><br>Webrev: <a \
href="http://cr.openjdk.java.net/~dtitov/8218401/webrev.03" \
target="_blank">http://cr.openjdk.java.net/~dtitov/8218401/webrev.03</a> <br>Bug : <a \
href="https://bugs.openjdk.java.net/browse/JDK-8218401" \
target="_blank">https://bugs.openjdk.java.net/browse/JDK-8218401</a> \
<br><br>Thanks!<br>--Daniil<br><br><br>On 3/20/19, 4:50 PM, "<a \
href="mailto:serguei.spitsyn@oracle.com" \
target="_blank">serguei.spitsyn@oracle.com</a>" <<a \
href="mailto:serguei.spitsyn@oracle.com" \
target="_blank">serguei.spitsyn@oracle.com</a>> wrote:<br><br> Hi \
Daniil,<br><br> You are right, there is a hole for this.<br><br> \
It can be easily fixed if the callbacks check phase at the \
beginning.<br> But I don't think this would be simpler than your current \
fix.<br> As I already mentioned, I'm Okay with the fix you have \
now.<br><br> I'm still thinking on how to fix this on the JVMTI \
level.<br><br> Thanks,<br> Serguei<br><br><br> \
On 3/20/19 16:35, Daniil Titov wrote:<br> > Hi Serguei,<br> \
><br> > I could be missing something but as I understood \
the suggested alternative approach was to not introduce the \
callbacksEnabled flag and instead just unregister other callbacks inside \
VMDeath callback. My concern was the case when one thread is on line 1302 (it \
is going to call a class loaded callback), while another thread just entered VMDeath \
callback. Without callbacksEnabled flag the first thread will enter the \
callback and wait on the raw monitor acquired by VMDeath callback thread, after that \
it will continue and call JVMTI API that might throw WRONG_PHASE error if by this \
time the second thread already returned from the VMDeath callback and changed \
the JVMTI phase.<br> ><br> > 1277 \
void JvmtiExport::post_class_load(JavaThread *thread, Klass* klass) {<br> \
> 1278 if (JvmtiEnv::get_phase() < \
JVMTI_PHASE_PRIMORDIAL) {<br> > 1279 \
return;<br> > 1280 }<br> \
> 1281 HandleMark hm(thread);<br> \
> 1282 <br> > \
1283 EVT_TRIG_TRACE(JVMTI_EVENT_CLASS_LOAD, ("[%s] Trg Class \
Load triggered",<br> > 1284 \
\
JvmtiTrace::safe_get_thread_name(thread)));<br> > \
1285 JvmtiThreadState* state = \
thread->jvmti_thread_state();<br> > 1286 \
if (state == NULL) {<br> > 1287 \
return;<br> > 1288 \
}<br> > 1289 \
JvmtiEnvThreadStateIterator it(state);<br> > \
1290 for (JvmtiEnvThreadState* ets = it.first(); ets != NULL; ets \
= it.next(ets)) {<br> > 1291 \
if (ets->is_enabled(JVMTI_EVENT_CLASS_LOAD)) {<br> > \
1292 JvmtiEnv *env = \
ets->get_env();<br> > 1293 \
if (env->phase() == JVMTI_PHASE_PRIMORDIAL) {<br> > \
1294 continue;<br> \
> 1295 }<br> > \
1296 EVT_TRACE(JVMTI_EVENT_CLASS_LOAD, \
("[%s] Evt Class Load sent %s",<br> > \
1297 \
\
JvmtiTrace::safe_get_thread_name(thread),<br> > \
1298 \
\
klass==NULL? "NULL" : klass->external_name() ));<br> \
> 1299 JvmtiClassEventMark \
jem(thread, klass);<br> > 1300 \
JvmtiJavaThreadEventTransition jet(thread);<br> > \
1301 jvmtiEventClassLoad callback = \
env->callbacks()->ClassLoad;<br> > 1302 \
if (callback != NULL) {<br> > \
1303 (*callback)(env->jvmti_external(), \
jem.jni_env(), jem.jni_thread(), jem.jni_class());<br> > \
1304 }<br> > 1305 \
}<br> > 1306 \
}<br> > 1307 }<br> \
><br> > Thanks!<br> ><br> > \
--Daniil<br> ><br> > On 3/20/19, 3:40 PM, "<a \
href="mailto:serguei.spitsyn@oracle.com" \
target="_blank">serguei.spitsyn@oracle.com</a>" <<a \
href="mailto:serguei.spitsyn@oracle.com" \
target="_blank">serguei.spitsyn@oracle.com</a>> wrote:<br> \
><br> > Hi Daniil,<br> > \
<br> > No chance for it if you lock \
a raw monitor in the event callbacks.<br> > \
<br> > Thanks,<br> > \
Serguei<br> > <br> > \
On 3/20/19 14:58, Daniil Titov wrote:<br> > \
> Hi Serguei,<br> > \
><br> > > I think that just disabling event \
notifications inside VMDeath callback still leaves a small window for VMDeath \
callback being called after the classload callback is called (or about being \
called) but before it enters a raw monitor. Thus I decided to follow your \
original suggestion and restore volatile modifier for callbacksEnabled. \
Please review a new version of the patch.<br> > \
><br> > > Webrev: <a \
href="http://cr.openjdk.java.net/~dtitov/8218401/webrev.03/" \
target="_blank">http://cr.openjdk.java.net/~dtitov/8218401/webrev.03/</a><br> \
> > Bug : <a \
href="https://bugs.openjdk.java.net/browse/JDK-8218401" \
target="_blank">https://bugs.openjdk.java.net/browse/JDK-8218401</a><br> \
> ><br> > > \
Thanks!<br> > ><br> > \
> Best regards,<br> > > \
Daniil<br> > ><br> > \
><br> > ><br> \
> > From: <<a href="mailto:serguei.spitsyn@oracle.com" \
target="_blank">serguei.spitsyn@oracle.com</a>><br> > \
> Organization: Oracle Corporation<br> > \
> Date: Tuesday, March 19, 2019 at 7:17 PM<br> > \
> To: Daniil Titov <<a href="mailto:daniil.x.titov@oracle.com" \
target="_blank">daniil.x.titov@oracle.com</a>>, OpenJDK Serviceability <<a \
href="mailto:serviceability-dev@openjdk.java.net" \
target="_blank">serviceability-dev@openjdk.java.net</a>>, Jean Christophe Beyler \
<<a href="mailto:jcbeyler@google.com" \
target="_blank">jcbeyler@google.com</a>><br> > \
> Subject: Re: 8218401: WRONG_PHASE: vmTestbase/nsk/jvmti test crash<br> \
> ><br> > > \
Hi Daniil,<br> > ><br> > \
> I'd keep the volatile modifier for callbacksEnabled to disable \
compiler optimizations.<br> > > Otherwise, \
looks good to me.<br> > ><br> \
> ><br> > > Another \
approach could be to disable event notifications in VMDeath callback with:<br> \
> > \
SetEventNotificationMode(JVMTI_DISABLE, JVMTI_EVENT_CLASS_LOAD, NULL);<br> \
> > \
SetEventNotificationMode(JVMTI_DISABLE, JVMTI_EVENT_BREAKPOINT, NULL);<br> \
> > . . .<br> > \
><br> > > Thanks,<br> \
> > Serguei<br> > \
><br> > > On 3/18/19 6:58 PM, Daniil \
Titov wrote:<br> > > Hi Serguei and \
JC,<br> > ><br> > \
> Please review a new version of the fix that locks a monitor across the \
callbacks, as Serguei suggested.<br> > \
><br> > > Webrev: <a \
href="http://cr.openjdk.java.net/~dtitov/8218401/webrev.02/" \
target="_blank">http://cr.openjdk.java.net/~dtitov/8218401/webrev.02/</a><br> \
> > Bug: <a \
href="https://bugs.openjdk.java.net/browse/JDK-8218401" \
target="_blank">https://bugs.openjdk.java.net/browse/JDK-8218401</a><br> \
> ><br> > > \
Thanks!<br> > > --Daniil<br> \
> ><br> > \
><br> > > On 3/18/19, 9:47 AM, mailto:<a \
href="mailto:serguei.spitsyn@oracle.com" \
target="_blank">serguei.spitsyn@oracle.com</a> mailto:<a \
href="mailto:serguei.spitsyn@oracle.com" \
target="_blank">serguei.spitsyn@oracle.com</a> wrote:<br> > \
><br> > > \
Hi Daniil,<br> > ><br> > \
> The JVMTI phase can change in the middle of \
callback work after the<br> > > \
check you added.<br> > > \
I'd suggest to lock a raw monitor across the callbacks to make them \
atomic.<br> > ><br> > \
> Thank you for taking care about this \
issue!<br> > ><br> > \
> Thanks,<br> > \
> Serguei<br> > \
><br> > ><br> > \
><br> > > \
On 3/15/19 16:08, Daniil Titov wrote:<br> > \
> > Please review the change that fixes 3 tests that \
intermittently fail with JVMTI_ERROR_WRONG_PHASE error.<br> > \
> ><br> > \
> > The problem here is that the callbacks these tests \
enable keep processing events and perform JVMTI calls after VM is terminated. The fix \
makes these test listen for VMDeath event and quick return from the callbacks \
after VMDeath event is received.<br> > > \
><br> > > \
> Webrev: <a href="http://cr.openjdk.java.net/~dtitov/8218401/webrev.01/" \
target="_blank">http://cr.openjdk.java.net/~dtitov/8218401/webrev.01/</a><br> \
> > > Bug: <a \
href="https://bugs.openjdk.java.net/browse/JDK-8218401" \
target="_blank">https://bugs.openjdk.java.net/browse/JDK-8218401</a><br> \
> > ><br> > \
> > Thanks!<br> > \
> > -Daniil<br> > \
> ><br> > \
> ><br> > \
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic