Re: 8218401: WRONG_PHASE: vmTestbase/nsk/jvmti test crash
Thank you, JC and Serguei, for reviewing this change.
Best regards,
Daniil
From: Jean Christophe Beyler
Date: Thursday, March 21, 2019 at 9:29 AM
To: Daniil Titov
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 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.spit...@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),
>1298klass==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.spit...@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,
> > Dani
Re: 8218401: WRONG_PHASE: vmTestbase/nsk/jvmti test crash
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.
1277void 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.spit...@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:
> Organization: Oracle Corporation
> Date: Tuesday, March 19, 2019 at 7:17 PM
> To: Daniil Titov , OpenJDK Serviceability
, Jean Christophe Beyler
> 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.spit...@oracle.com
mailto:serguei.spit...@oracle.com wrote:
>
> Hi Daniil,
>
> The JVMTI phase can change
Re: 8218401: WRONG_PHASE: vmTestbase/nsk/jvmti test crash
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.
1277void JvmtiExport::post_class_load(JavaThread *thread, Klass* klass) {
1278if (JvmtiEnv::get_phase() < JVMTI_PHASE_PRIMORDIAL) {
1279 return;
1280}
1281HandleMark hm(thread);
1282
1283EVT_TRIG_TRACE(JVMTI_EVENT_CLASS_LOAD, ("[%s] Trg Class Load
triggered",
1284JvmtiTrace::safe_get_thread_name(thread)));
1285JvmtiThreadState* state = thread->jvmti_thread_state();
1286if (state == NULL) {
1287 return;
1288}
1289JvmtiEnvThreadStateIterator it(state);
1290for (JvmtiEnvThreadState* ets = it.first(); ets != NULL; ets =
it.next(ets)) {
1291 if (ets->is_enabled(JVMTI_EVENT_CLASS_LOAD)) {
1292JvmtiEnv *env = ets->get_env();
1293if (env->phase() == JVMTI_PHASE_PRIMORDIAL) {
1294 continue;
1295}
1296EVT_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() ));
1299JvmtiClassEventMark jem(thread, klass);
1300JvmtiJavaThreadEventTransition jet(thread);
1301jvmtiEventClassLoad callback = env->callbacks()->ClassLoad;
1302if (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.spit...@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:
> Organization: Oracle Corporation
> Date: Tuesday, March 19, 2019 at 7:17 PM
> To: Daniil Titov , OpenJDK Serviceability
, Jean Christophe Beyler
> 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.spit...@oracle.com
mailto:serguei.spit...@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 rev
Re: 8218401: WRONG_PHASE: vmTestbase/nsk/jvmti test crash
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: Organization: Oracle Corporation Date: Tuesday, March 19, 2019 at 7:17 PM To: Daniil Titov , OpenJDK Serviceability , Jean Christophe Beyler 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.spit...@oracle.com mailto:serguei.spit...@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 > >
Re: 8218401: WRONG_PHASE: vmTestbase/nsk/jvmti test crash
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: Organization: Oracle Corporation Date: Tuesday, March 19, 2019 at 7:17 PM To: Daniil Titov , OpenJDK Serviceability , Jean Christophe Beyler 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.spit...@oracle.com mailto:serguei.spit...@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 > >
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 approachcould 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, "serguei.spit...@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 > >
Re: 8218401: WRONG_PHASE: vmTestbase/nsk/jvmti test crash
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, "serguei.spit...@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 > >
Re: 8218401: WRONG_PHASE: vmTestbase/nsk/jvmti test crash
Hi JC,
You are right, the synchronization is required across callbacks, otherwise
JVMTI phase could change in the middle of a callback. I will send a new version
of the patch soon.
Thanks!
--Daniil
From: Jean Christophe Beyler
Date: Friday, March 15, 2019 at 4:45 PM
To: Daniil Titov
Cc: OpenJDK Serviceability
Subject: Re: RFR: 8218401: WRONG_PHASE: vmTestbase/nsk/jvmti test crash
Hi Daniil,
Is it not possible that the VMDeath would happen later in the case of:
http://cr.openjdk.java.net/~dtitov/8218401/webrev.01/test/hotspot/jtreg/vmTestbase/nsk/jvmti/FramePop/framepop002/framepop002.cpp.html
For example, that callbacks are not yet disabled when we pass the new test:
+if (!callbacksEnabled) {
+return;
+}
But gets disabled right after? (this is for my own understanding of what is
possible or not :-))
Thanks!
Jc
On Fri, Mar 15, 2019 at 4:08 PM 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
