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 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 >>>>> >>>> >>> >> >