RFR (S): 8187289 NotifyFramePop request is not cleared if JVMTI_EVENT_FRAME_POP is disabled
Daniel D. Daugherty
daniel.daugherty at oracle.com
Fri Nov 3 11:56:39 UTC 2017
>
http://cr.openjdk.java.net/%7Esspitsyn/webrevs/2017/hotspot/8187289-jvmti-framepop.2/
make/test/JtregNativeHotspot.gmk
No comments.
src/hotspot/share/prims/jvmtiEventController.cpp
No comments.
src/hotspot/share/prims/jvmtiExport.cpp
No comments.
src/hotspot/share/prims/jvmtiExport.hpp
No comments.
src/hotspot/share/prims/jvmtiManageCapabilities.cpp
No comments.
test/hotspot/jtreg/serviceability/jvmti/NotifyFramePop/NotifyFramePopTest.java
No comments.
test/hotspot/jtreg/serviceability/jvmti/NotifyFramePop/libNotifyFramePopTest.c
No comments.
Thumbs up!
Dan
On 11/2/17 4:59 PM, serguei.spitsyn at oracle.com wrote:
> Hi Dan,
>
> Thank you a lot for doing this review!
> I was waiting for you for a couple of weeks. :)
>
>
> On 11/2/17 10:13, Daniel D. Daugherty wrote:
>> 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.
>
> All the above is correct.
> Thank you for providing this background as it is useful for reviewers!
>
>
>> 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.
>
> The above is correct too.
>
>
>> 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.
>
> Right.
>
>
>> 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:
>
> Right but modulo what Alan said that bug fixing this bug we are going
> to change the behavior.
> So that at least we need a CSR and release note for this update.
>
>
>
>>
>> >
>> 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.
>
> Ok, thanks.
>
>
>> 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.
>
> Ok, thanks.
>
>
>> 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'
>
> Good eyes!
> Fixed.
>
>
>> 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.
>
> Refactored as you suggested.
> Will need to test it well.
>
>
>> src/hotspot/share/prims/jvmtiManageCapabilities.cpp
>> Needs copyright year update.
>
> Updated the copyright year, thanks.
>
>
>> 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. :-)
>
> Good suggestions, thanks.
> Added the comments.
>
>
>> 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.
>
> Good catches, thanks.
> Fixed.
>
>
> The updated webrev is:
> http://cr.openjdk.java.net/%7Esspitsyn/webrevs/2017/hotspot/8187289-jvmti-framepop.2/
>
>
> I will also run the jvmti, jdi, jdwp and j.l.instrument tests.
>
>
> Thanks,
> Serguei
>
>
More information about the hotspot-dev
mailing list