Re: RFR: 8041934 com/sun/jdi/RepStep.java fails on all platforms with assert(_cur_stack_depth == count_frames()) failed: cur_stack_depth out of sync
Looks good. On May 13, 2014, at 11:58 PM, Staffan Larsen staffan.lar...@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.thalin...@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.lar...@oracle.com wrote: On 13 maj 2014, at 18:53, Daniel D. Daugherty daniel.daughe...@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.spit...@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.daughe...@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
Re: RFR: 8041934 com/sun/jdi/RepStep.java fails on all platforms with assert(_cur_stack_depth == count_frames()) failed: cur_stack_depth out of sync
On 9 maj 2014, at 20:18, serguei.spit...@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.daughe...@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
Re: RFR: 8041934 com/sun/jdi/RepStep.java fails on all platforms with assert(_cur_stack_depth == count_frames()) failed: cur_stack_depth out of sync
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... Thumbs up! Dan On 5/13/14 3:20 AM, Staffan Larsen wrote: On 9 maj 2014, at 20:18, serguei.spit...@oracle.com mailto:serguei.spit...@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. Daughertydaniel.daughe...@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/ http://cr.openjdk.java.net/%7Esla/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
Re: RFR: 8041934 com/sun/jdi/RepStep.java fails on all platforms with assert(_cur_stack_depth == count_frames()) failed: cur_stack_depth out of sync
On 5/13/14 2:20 AM, Staffan Larsen wrote: On 9 maj 2014, at 20:18, serguei.spit...@oracle.com mailto:serguei.spit...@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. Daughertydaniel.daughe...@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/ http://cr.openjdk.java.net/%7Esla/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/ Looks good to me. Thanks, Serguei 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
Re: RFR: 8041934 com/sun/jdi/RepStep.java fails on all platforms with assert(_cur_stack_depth == count_frames()) failed: cur_stack_depth out of sync
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.lar...@oracle.com wrote: On 13 maj 2014, at 18:53, Daniel D. Daugherty daniel.daughe...@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.spit...@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.daughe...@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
Re: RFR: 8041934 com/sun/jdi/RepStep.java fails on all platforms with assert(_cur_stack_depth == count_frames()) failed: cur_stack_depth out of sync
On 8 maj 2014, at 19:05, Daniel D. Daugherty daniel.daughe...@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! 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
Re: RFR: 8041934 com/sun/jdi/RepStep.java fails on all platforms with assert(_cur_stack_depth == count_frames()) failed: cur_stack_depth out of sync
Updated review: http://cr.openjdk.java.net/~sla/8041934/webrev.01/ Thumbs up! src/share/vm/runtime/sharedRuntime.hpp No comments. src/share/vm/runtime/sharedRuntime.cpp Thanks for fixing the entry type. 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. Dan On 5/9/14 4:47 AM, Staffan Larsen wrote: On 8 maj 2014, at 19:05, Daniel D. Daughertydaniel.daughe...@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! 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
Re: RFR: 8041934 com/sun/jdi/RepStep.java fails on all platforms with assert(_cur_stack_depth == count_frames()) failed: cur_stack_depth out of sync
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.daughe...@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); 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
Re: RFR: 8041934 com/sun/jdi/RepStep.java fails on all platforms with assert(_cur_stack_depth == count_frames()) failed: cur_stack_depth out of sync
Note that RepStep was just added to ProblemList due to this issue and will have to be removed from there when the fix is integrated. Mikael On 2014-05-08 08:06, 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
Re: RFR: 8041934 com/sun/jdi/RepStep.java fails on all platforms with assert(_cur_stack_depth == count_frames()) failed: cur_stack_depth out of sync
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. 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... 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
