JDK-7127032 : fix for 7122253 adds a JvmtiThreadState earlier than necessary
  • Type: Bug
  • Component: hotspot
  • Sub-Component: jvmti
  • Affected Version: hs23
  • Priority: P2
  • Status: Closed
  • Resolution: Fixed
  • OS: generic
  • CPU: generic
  • Submitted: 2012-01-04
  • Updated: 2013-06-22
  • Resolved: 2012-03-29
The Version table provides details related to the release that this issue/RFE will be addressed.

Unresolved : Release in which this issue/RFE will be addressed.
Resolved: Release in which this issue/RFE has been resolved.
Fixed : Release in which this issue/RFE has been fixed. The release containing this fix may be available for download as an Early Access Release or a General Availability Release.

To download the current JDK release, click here.
JDK 6 JDK 8 Other
6u60Fixed 8Fixed hs23Fixed
Related Reports
Relates :  
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
EVALUATION http://hg.openjdk.java.net/lambda/lambda/hotspot/rev/5b58979183f9
22-03-2012

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

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

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

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.
04-01-2012

EVALUATION Switch from: JvmtiThreadState *state = JvmtiThreadState::state_for(jt); to: JvmtiThreadState *state = jt->jvmti_thread_state(); if (state != NULL) { with appropriate indentation changes.
04-01-2012