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