RFR (S): 8187289 NotifyFramePop request is not cleared if JVMTI_EVENT_FRAME_POP is disabled

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR (S): 8187289 NotifyFramePop request is not cleared if JVMTI_EVENT_FRAME_POP is disabled

serguei.spitsyn@oracle.com
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.
  N
otification 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 n
ew 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
Reply | Threaded
Open this post in threaded view
|

Re: RFR (S): 8187289 NotifyFramePop request is not cleared if JVMTI_EVENT_FRAME_POP is disabled

serguei.spitsyn@oracle.com
PING..

Thanks,
Serguei


On 10/12/17 21:58, [hidden email] 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

Reply | Threaded
Open this post in threaded view
|

Re: RFR (S): 8187289 NotifyFramePop request is not cleared if JVMTI_EVENT_FRAME_POP is disabled

Daniel D. Daugherty
In reply to this post by serguei.spitsyn@oracle.com
On 10/13/17 12:58 AM, [hidden email] 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.
  N
otification 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 n
ew 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.


Reply | Threaded
Open this post in threaded view
|

Re: RFR (S): 8187289 NotifyFramePop request is not cleared if JVMTI_EVENT_FRAME_POP is disabled

serguei.spitsyn@oracle.com
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, [hidden email] 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


Reply | Threaded
Open this post in threaded view
|

Re: RFR (S): 8187289 NotifyFramePop request is not cleared if JVMTI_EVENT_FRAME_POP is disabled

Daniel D. Daugherty
> 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, [hidden email] 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, [hidden email] 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



Reply | Threaded
Open this post in threaded view
|

Re: RFR (S): 8187289 NotifyFramePop request is not cleared if JVMTI_EVENT_FRAME_POP is disabled

serguei.spitsyn@oracle.com
Dan,

Thank you a lot for review!
Serguei


On 11/3/17 04:56, Daniel D. Daugherty wrote:
> 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, [hidden email] 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, [hidden email] 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