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