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