United StatesChange Country, Oracle Worldwide Web Sites Communities I am a... I want to...
Bug ID: JDK-7127032 fix for 7122253 adds a JvmtiThreadState earlier than necessary
JDK-7127032 : fix for 7122253 adds a JvmtiThreadState earlier than necessary

Details
Type:
Bug
Submit Date:
2012-01-04
Status:
Closed
Updated Date:
2013-06-22
Project Name:
JDK
Resolved Date:
2012-03-29
Component:
hotspot
OS:
generic
Sub-Component:
jvmti
CPU:
generic
Priority:
P2
Resolution:
Fixed
Affected Versions:
hs23
Fixed Versions:
hs23 (b10)

Related Reports
Backport:
Backport:
Backport:
Backport:
Backport:
Relates:
Relates:

Sub Tasks

Description
The fix for the following bug:

    7122253 2/3 Instrumentation.retransformClasses() leaks
                class bytes

made a subtle change to when a JvmtiThreadState is added
to a JavaThread. This subtle change exposed a bug in the
way events are enabled in the different JVM/TI phases.
That bug is being tracked by the following:

    7126851 3/4 cm02t001 fails due to JVMTI_ERROR_WRONG_PHASE
                in GetThreadInfo() call

and the failing VM/NSK test is:

    nsk/jvmti/scenarios/capability/CM02/cm02t001

Update: here is a snippet from the test's .log file:

[2012-01-05T03:53:13.45] ${JAVA} ${JAVA_OPTS} ${EXECUTE_CLASS} ${TEST_ARGS}
[2012-01-05T03:53:13.47] # Actual: /export/local/common/jdk/baseline/solaris-i586/bin/java -client -Xcomp -XX:-PrintVMOptions -Dsun.jvm.hotspot.runtime.VM.disableVersionCheck=1 -XX:DefaultMaxRAMFraction=8 -XX:+IgnoreUnrecognizedVMOptions -XX:-UseCompressedOops -agentlib:cm02t001=-waittime=5 nsk.jvmti.scenarios.capability.CM02.cm02t001
[2012-01-05T03:53:13.47] # ERROR: cm02t001.c, 162: NSK_CPP_STUB3(GetThreadInfo, jvmti_env, thread, &info)
[2012-01-05T03:53:19.89] #   jvmti error: code=112, name=JVMTI_ERROR_WRONG_PHASE
[2012-01-05T03:53:19.89] # ERROR: cm02t001.c, 162: NSK_CPP_STUB3(GetThreadInfo, jvmti_env, thread, &info)
[2012-01-05T03:53:19.89] #   jvmti error: code=112, name=JVMTI_ERROR_WRONG_PHASE
[2012-01-05T03:53:19.89] # Test level exit status: 97


The purpose of this bug is to restore when a JvmtiThreadState
is added to a thread to the same semantics that existed before
the fix for 7122253.

Consider this code in src/share/vm/prims/jvmtiExport.cpp:

   507  class JvmtiClassFileLoadHookPoster : public StackObj {

<snip>

   542      _state = _thread->jvmti_thread_state();
   543      if (_state != NULL) {
   544        _h_class_being_redefined = _state->get_class_being_redefined();
   545        _load_kind = _state->get_class_load_kind();
   546        // Clear class_being_redefined flag here. The action
   547        // from agent handler could generate a new class file load
   548        // hook event and if it is not cleared the new event generated
   549        // from regular class file load could have this stale redefined
   550        // class handle info.
   551        _state->clear_class_being_redefined();
   552      } else {
   553        // redefine and retransform will always set the thread state
   554        _h_class_being_redefined = (KlassHandle *) NULL;
   555        _load_kind = jvmti_class_load_kind_load;
   556      }

Line 542 uses JavaThread::jvmti_thread_state() instead of calling
JvmtiThreadState::state_for() so the ClassFileLoadHook event posting
code is careful to use a JvmtiThreadState if one exists, but it does
not allocate (and attach) one if it does not exist. When I fixed
7122253, I missed this subtlety.

And now consider this code in src/share/vm/classfile/classFileParser.cpp
that was added by 7122253:

@@ -2870,6 +2874,20 @@ instanceKlassHandle ClassFileParser::par
   _max_bootstrap_specifier_index = -1;

   if (JvmtiExport::should_post_class_file_load_hook()) {
+    // Get the cached class file bytes (if any) from the
+    // class that is being redefined.
+    JvmtiThreadState *state = JvmtiThreadState::state_for(jt);
+    KlassHandle      *h_class_being_redefined =
+                        state->get_class_being_redefined();
+    if (h_class_being_redefined != NULL) {
+      instanceKlassHandle ikh_class_being_redefined =
+        instanceKlassHandle(THREAD, (*h_class_being_redefined)());
+      cached_class_file_bytes =
+        ikh_class_being_redefined->get_cached_class_file_bytes();
+      cached_class_file_length =
+        ikh_class_being_redefined->get_cached_class_file_len();
+    }
+
     unsigned char* ptr = cfs->buffer();
     unsigned char* end_ptr = cfs->buffer() + cfs->length();

The JvmtiThreadState is the right place to find "the class
being redefined", but the state_for() call causes a
JvmtiThreadState to be allocated (and attached) earlier
than it was before. If JavaThread::jvmti_thread_state() is
used instead, then the "the class being redefined" can be
found when it exists and no JvmtiThreadState is allocated
when one is not needed.
HS 23 b10 JDK 7u4/b07, 8/b22 PIT bugfix verification status: verified

Verified on the intelsdv13 machine running the test.

                                    

Comments
SUGGESTED FIX

Here is a context diff for a possible fix:

*** classFileParser.cpp Wed Jan  4 10:45:20 2012
--- classFileParser.cpp.new     Wed Jan  4 09:21:51 2012
***************
*** 2874,2891 ****
    _max_bootstrap_specifier_index = -1;
  
    if (JvmtiExport::should_post_class_file_load_hook()) {
!     // Get the cached class file bytes (if any) from the
!     // class that is being redefined.
!     JvmtiThreadState *state = JvmtiThreadState::state_for(jt);
!     KlassHandle      *h_class_being_redefined =
!                         state->get_class_being_redefined();
!     if (h_class_being_redefined != NULL) {
!       instanceKlassHandle ikh_class_being_redefined =
!         instanceKlassHandle(THREAD, (*h_class_being_redefined)());
!       cached_class_file_bytes =
!         ikh_class_being_redefined->get_cached_class_file_bytes();
!       cached_class_file_length =
!         ikh_class_being_redefined->get_cached_class_file_len();
      }
  
      unsigned char* ptr = cfs->buffer();
--- 2874,2896 ----
    _max_bootstrap_specifier_index = -1;
  
    if (JvmtiExport::should_post_class_file_load_hook()) {
!     // Get the cached class file bytes (if any) from the class that
!     // is being redefined or retransformed. We use jvmti_thread_state()
!     // instead of JvmtiThreadState::state_for(jt) so we don't allocate
!     // a JvmtiThreadState any earlier than necessary. This will help
!     // avoid the bug described by 7126851.
!     JvmtiThreadState *state = jt->jvmti_thread_state();
!     if (state != NULL) {
!       KlassHandle *h_class_being_redefined =
!                      state->get_class_being_redefined();
!       if (h_class_being_redefined != NULL) {
!         instanceKlassHandle ikh_class_being_redefined =
!           instanceKlassHandle(THREAD, (*h_class_being_redefined)());
!         cached_class_file_bytes =
!           ikh_class_being_redefined->get_cached_class_file_bytes();
!         cached_class_file_length =
!           ikh_class_being_redefined->get_cached_class_file_len();
!       }
      }
  
      unsigned char* ptr = cfs->buffer();


Here is a 'diff -w' of the same fix:

2877,2879c2877,2883
<     // Get the cached class file bytes (if any) from the
<     // class that is being redefined.
<     JvmtiThreadState *state = JvmtiThreadState::state_for(jt);
---
>     // Get the cached class file bytes (if any) from the class that
>     // is being redefined or retransformed. We use jvmti_thread_state()
>     // instead of JvmtiThreadState::state_for(jt) so we don't allocate
>     // a JvmtiThreadState any earlier than necessary. This will help
>     // avoid the bug described by 7126851.
>     JvmtiThreadState *state = jt->jvmti_thread_state();
>     if (state != NULL) {
2889a2894
>     }

so the indent changes are supressed.
                                     
2012-01-04
EVALUATION

Switch from:

    JvmtiThreadState *state = JvmtiThreadState::state_for(jt);

to:

    JvmtiThreadState *state = jt->jvmti_thread_state();
    if (state != NULL) {

with appropriate indentation changes.
                                     
2012-01-04
SUGGESTED FIX

See the attached 7127032-webrev-cr0.tgz for the proposed fix
for code review round 0.
                                     
2012-01-05
EVALUATION

http://hg.openjdk.java.net/hsx/hotspot-rt/hotspot/rev/5b58979183f9
                                     
2012-01-05
EVALUATION

http://hg.openjdk.java.net/hsx/hotspot-main/hotspot/rev/5b58979183f9
                                     
2012-01-07
EVALUATION

http://hg.openjdk.java.net/lambda/lambda/hotspot/rev/5b58979183f9
                                     
2012-03-22



Hardware and Software, Engineered to Work Together