|
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.spit...@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
|
