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 <[email protected]>
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 <[email protected]>
> wrote:
>
>>
>> On 13 maj 2014, at 18:53, Daniel D. Daugherty <[email protected]>
>> 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, [email protected] 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
>>>>>> <[email protected]> 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
>>>>>
>>>>
>>>
>>
>
