RFR (S): 8187289 NotifyFramePop request is not cleared if JVMTI_EVENT_FRAME_POP is disabled
Daniel D. Daugherty
daniel.daugherty at oracle.com
Thu Nov 2 17:13:36 UTC 2017
On 10/13/17 12:58 AM, serguei.spitsyn at oracle.com wrote:
> Please, review a fix for:
> https://bugs.openjdk.java.net/browse/JDK-8187289
>
> Webrev:
> http://cr.openjdk.java.net/~sspitsyn/webrevs/2017/hotspot/8187289-jvmti-framepop.1/
>
>
> Summary:
> The issue is that the FRAME_POP event request is not cleaned if the
> requested
> method frame is popped but the notification mode is temporarily
> disabled.
> If later the notification mode is enabled again then it will post a
> FRAME_POP
> event on the first return from an arbitrary method with the same frame
> depth.
> Notification mode for JVMTI_EVENT_FRAME_POP events is checked in the
> JvmtiExport::post_method_exit() under the condition:
> if (state->is_enabled(JVMTI_EVENT_FRAME_POP)) {
> Just removing this condition would likely to introduce a performance
> regression
> in interpreted mode. To mitigate it the fix introduces new
> JVMTI_SUPPORT_FLAG
> can_generate_frame_pop_events that is is checked instead of the
> notification mode.
> Also, it is necessary to to keep the
> state->is_interp_only_mode()turned on
> while the JvmtiEnvThreadState has any FRAME_POP requests registered.
>
> One alternate approach to this fix is to leave the current behavior
> as it is
> but update the JVMTI spec with some clarification about this behavior.
>
> Testing:
> Verified with new unit test serviceability/jvmti/NotifyFramePop.
> Also ran the nsk.jvmti, nsk.jdi and jtreg jdk_jdi tests to make sure
> there is no regression.
>
> Thanks,
> Serguei
First, I'm going to take a step back and look at the JVM/TI programming
model for NotifyFramePop() and JVMTI_EVENT_FRAME_POP events. Here's the
relevant JVM/TI spec links:
https://docs.oracle.com/javase/9/docs/specs/jvmti.html#NotifyFramePop
https://docs.oracle.com/javase/9/docs/specs/jvmti.html#FramePop
NotifyFramePop() is used by agent code to register interest in
when a target thread leaves a Java frame at a specific depth.
The target thread can be NULL which means that the current thread
is registering interest in itself. The frame is specified by a
'depth' parameter so the agent is not expressing an interest in a
specific named function. Also note that NotifyFramePop() can only
register interest in Java frames.
The agent code must also enable the JVMTI_EVENT_FRAME_POP event in
order for the FRAME_POP event to be delivered to the agent's event
handler. There is no requirement to enable the FRAME_POP event before
calling NotifyFramePop(). Nor is there any requirement for clearing
existing frame interest entries when the FRAME_POP event is disabled.
If we have target thread call stack that looks like this:
call_depth_0()
call_depth_1()
call_depth_2()
agent_code()
The agent_code(), operating on the current thread, calls:
callbacks.FramePop = frame_pop_callback;
SetEventCallbacks(callbacks, size_of_callbacks);
SetEventNotificationMode(JVMTI_ENABLE, JVMTI_EVENT_FRAME_POP,
NULL);
NotifyFramePop(NULL, 1);
or it calls:
callbacks.FramePop = frame_pop_callback;
SetEventCallbacks(callbacks, size_of_callbacks);
NotifyFramePop(NULL, 1);
SetEventNotificationMode(JVMTI_ENABLE, JVMTI_EVENT_FRAME_POP,
NULL);
and in either case, we expect a FRAME_POP event to be posted when the
target thread returns from call_depth_1() to call_depth_0(). The
FRAME_POP event is typically disabled in the FRAME_POP event handler
function (frame_pop_callback) that is called when the FRAME_POP event
is posted.
Because disabling FRAME_POP is not specified to clear interest in a
frame, if the agent_code(), operating on the current thread, calls:
callbacks.FramePop = frame_pop_callback;
SetEventCallbacks(callbacks, size_of_callbacks);
SetEventNotificationMode(JVMTI_ENABLE, JVMTI_EVENT_FRAME_POP,
NULL);
NotifyFramePop(NULL, 1);
SetEventNotificationMode(JVMTI_DISABLE, JVMTI_EVENT_FRAME_POP,
NULL);
SetEventNotificationMode(JVMTI_ENABLE, JVMTI_EVENT_FRAME_POP,
NULL);
then we expect a FRAME_POP event to be posted when the target thread
returns from call_depth_1 to call_depth_0 (and the frame_pop_callback()
event handler is called).
Okay so I think that covers the general programming model and includes
a scenario that shows why we can't clear interest in a frame when the
FRAME_POP event is disabled.
So we have this NotifyFramePop() function that registers interest in when a
target thread leaves a Java frame at a specific depth. We don't have a
ClearFramePop() or UnNotifyFramePop() function so the expected way to clear
an existing frame interest entry is to do so when we leave that frame. So
the current behavior where we can return from call_depth_1() to
call_depth_0()
without clearing the existing frame interest entry is a bug.
Okay so I'm now caught up with this thread and I agree that we have a bug.
Definitely a corner case, but a bug none the less. So far I don't see a
reason to change any spec wording so let's look at the webrev:
>
http://cr.openjdk.java.net/~sspitsyn/webrevs/2017/hotspot/8187289-jvmti-framepop.1/
src/hotspot/share/prims/jvmtiExport.hpp
I'm wondering why the existing can_post_interpreter_events() can't
cover the case for us since we need to stay in interpreted mode.
Update: The existing can_post_interpreter_events() could be used
but the new can_generate_frame_pop_events() is more specific. I'm
okay with it.
src/hotspot/share/prims/jvmtiExport.cpp
I agree that JvmtiExport::post_method_exit() needs to check a
condition other than "is_enabled(JVMTI_EVENT_FRAME_POP)", but
should it be can_generate_frame_pop_events() or should it be
can_post_interpreter_events()?
Update: The existing can_post_interpreter_events() could be used
but the new can_generate_frame_pop_events() is more specific. I'm
okay with it.
src/hotspot/share/prims/jvmtiEventController.cpp
L513: // This iteration will include JvmtiEnvThreadStates
whoses environments
Not yours, but could you please fix it?
Typo: 'whoses' -> 'whose'
L523-542 - Perhaps we should refactor the code as:
523 if (any_env_enabled != was_any_env_enabled) {
524 // mark if event is truly enabled on this thread in any
environment
525
state->thread_event_enable()->_event_enabled.set_bits(any_env_enabled);
526
527 // update the JavaThread cached value for thread-specific
should_post_on_exceptions value
528 bool should_post_on_exceptions = (any_env_enabled &
SHOULD_POST_ON_EXCEPTIONS_BITS) != 0;
529 state->set_should_post_on_exceptions(should_post_on_exceptions);
530 }
531
532 // compute interp_only mode
533 bool should_be_interp = (any_env_enabled & INTERP_EVENT_BITS)
!= 0 || has_frame_pops;
534 bool is_now_interp = state->is_interp_only_mode();
535 if (should_be_interp != is_now_interp) {
536 if (should_be_interp) {
537 enter_interp_only_mode(state);
538 } else {
539 leave_interp_only_mode(state);
540 }
541 }
My reasoning is that new L525 and L529 should only be called when
this is true: (any_env_enabled != was_any_env_enabled). We always
have to check for a change in interp_only mode because the new
has_frame_pops condition is independent of the
(any_env_enabled != was_any_env_enabled) check so we might as well
make that really clear.
src/hotspot/share/prims/jvmtiManageCapabilities.cpp
Needs copyright year update.
make/test/JtregNativeHotspot.gmk
No comments.
test/hotspot/jtreg/serviceability/jvmti/NotifyFramePop/NotifyFramePopTest.java
L69: notifyFramePop(null);
Please add comment above L69:
// Ask for notification when we leave frame 1 (meth01).
L70: setFramePopNotificationMode(false);
Please add comment above L70:
// Disabling FRAME_POP event notification should not
// prevent the notification for frame 1 from being cleared
// when we return from meth01.
L75: setFramePopNotificationMode(true);
Please add comment above L75:
// Enabling FRAME_POP event notification here should
// not result in a FRAME_POP event when we return from
// frame 1 (meth02) because we did not ask for a
// notification when we leave frame 1 in the context of
// this method (meth02).
to replace this comment:
L76: // We expect no FramePop event on the meth02()
exit as the
L77: // request from meth01() had to be clear at its
exit.
If you like my new comment. :-)
test/hotspot/jtreg/serviceability/jvmti/NotifyFramePop/libNotifyFramePopTest.c
L73: jmethodID method, jboolean wasPopedByException) {
Typo: 'wasPopedByException' -> 'wasPoppedByException'
L80: result = FAILED; // This event is not expect
Typo: 'expect' -> 'expected'
L101: jint Agent_Initialize(JavaVM *jvm, char *options, void
*reserved) {
Extra space between 'jint' and function name.
L150: if (err != JVMTI_ERROR_NONE) {
L151: printf("Failed to set notification mode for
FRAME_POP events: %s (%d)\n",
L152: TranslateError(err), err);
This error message should also report the 'mode' we're
trying to set for complete context.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20171102/a2f15e14/attachment-0001.html>
More information about the serviceability-dev
mailing list