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 >
