|
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
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, [email protected] 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
|
