8148772: VM crash in nsk/jvmti/RedefineClasses/StressRedefine: assert failed: Corrupted constant pool

8151546: nsk/jvmti/RedefineClasses/StressRedefine fails in hs nightly

ConstantPool::resolve_constant_at_impl() isn't thread safe for MethodHandleInError and MethodTypeInError and Constant pool merging is not thread safe for source_file_name.

Reviewed-by: sspitsyn, dcubed

hotspot/src/share/vm/classfile/javaClasses.inline.hpp

@@ -222,20 +222,17 @@ inline int Backtrace::get_line_number(const methodHandle& method, int bci) {
   return line_number;
 }
 
-/*
- * Returns the source file name of a given InstanceKlass and version
- */
 inline Symbol* Backtrace::get_source_file_name(InstanceKlass* holder, int version) {
-  // Find the specific ik version that contains this source_file_name_index
-  // via the previous versions list, but use the current version's
-  // constant pool to look it up.  The previous version's index has been
-  // merged for the current constant pool.
-  InstanceKlass* ik = holder->get_klass_version(version);
-  // This version has been cleaned up.
-  if (ik == NULL) return NULL;
-  int source_file_name_index = ik->source_file_name_index();
-  return (source_file_name_index == 0) ?
-      (Symbol*)NULL : holder->constants()->symbol_at(source_file_name_index);
+  // RedefineClasses() currently permits redefine operations to
+  // happen in parallel using a "last one wins" philosophy. That
+  // spec laxness allows the constant pool entry associated with
+  // the source_file_name_index for any older constant pool version
+  // to be unstable so we shouldn't try to use it.
+  if (holder->constants()->version() != version) {
+    return NULL;
+  } else {
+    return holder->source_file_name();
+  }
 }
 
 #endif // SHARE_VM_CLASSFILE_JAVACLASSES_INLINE_HPP

hotspot/src/share/vm/oops/constantPool.cpp

@@ -395,7 +395,7 @@ int ConstantPool::impl_name_and_type_ref_index_at(int which, bool uncached) {
   int i = which;
   if (!uncached && cache() != NULL) {
     if (ConstantPool::is_invokedynamic_index(which)) {
-      // Invokedynamic index is index into resolved_references
+      // Invokedynamic index is index into the constant pool cache
       int pool_index = invokedynamic_cp_cache_entry_at(which)->constant_pool_index();
       pool_index = invoke_dynamic_name_and_type_ref_index_at(pool_index);
       assert(tag_at(pool_index).is_name_and_type(), "");
@@ -965,8 +965,8 @@ bool ConstantPool::compare_entry_to(int index1, const constantPoolHandle& cp2,
 
   case JVM_CONSTANT_MethodType:
   {
-    int k1 = method_type_index_at_error_ok(index1);
-    int k2 = cp2->method_type_index_at_error_ok(index2);
+    int k1 = method_type_index_at(index1);
+    int k2 = cp2->method_type_index_at(index2);
     bool match = compare_entry_to(k1, cp2, k2, CHECK_false);
     if (match) {
       return true;
@@ -975,11 +975,11 @@ bool ConstantPool::compare_entry_to(int index1, const constantPoolHandle& cp2,
 
   case JVM_CONSTANT_MethodHandle:
   {
-    int k1 = method_handle_ref_kind_at_error_ok(index1);
-    int k2 = cp2->method_handle_ref_kind_at_error_ok(index2);
+    int k1 = method_handle_ref_kind_at(index1);
+    int k2 = cp2->method_handle_ref_kind_at(index2);
     if (k1 == k2) {
-      int i1 = method_handle_index_at_error_ok(index1);
-      int i2 = cp2->method_handle_index_at_error_ok(index2);
+      int i1 = method_handle_index_at(index1);
+      int i2 = cp2->method_handle_index_at(index2);
       bool match = compare_entry_to(i1, cp2, i2, CHECK_false);
       if (match) {
         return true;
@@ -1311,15 +1311,15 @@ void ConstantPool::copy_entry_to(const constantPoolHandle& from_cp, int from_i,
   case JVM_CONSTANT_MethodType:
   case JVM_CONSTANT_MethodTypeInError:
   {
-    jint k = from_cp->method_type_index_at_error_ok(from_i);
+    jint k = from_cp->method_type_index_at(from_i);
     to_cp->method_type_index_at_put(to_i, k);
   } break;
 
   case JVM_CONSTANT_MethodHandle:
   case JVM_CONSTANT_MethodHandleInError:
   {
-    int k1 = from_cp->method_handle_ref_kind_at_error_ok(from_i);
-    int k2 = from_cp->method_handle_index_at_error_ok(from_i);
+    int k1 = from_cp->method_handle_ref_kind_at(from_i);
+    int k2 = from_cp->method_handle_index_at(from_i);
     to_cp->method_handle_index_at_put(to_i, k1, k2);
   } break;
 
@@ -1755,8 +1755,8 @@ int ConstantPool::copy_cpool_bytes(int cpool_size,
       case JVM_CONSTANT_MethodHandle:
       case JVM_CONSTANT_MethodHandleInError: {
         *bytes = JVM_CONSTANT_MethodHandle;
-        int kind = method_handle_ref_kind_at_error_ok(idx);
-        idx1 = method_handle_index_at_error_ok(idx);
+        int kind = method_handle_ref_kind_at(idx);
+        idx1 = method_handle_index_at(idx);
         *(bytes+1) = (unsigned char) kind;
         Bytes::put_Java_u2((address) (bytes+2), idx1);
         DBG(printf("JVM_CONSTANT_MethodHandle: %d %hd", kind, idx1));
@@ -1765,7 +1765,7 @@ int ConstantPool::copy_cpool_bytes(int cpool_size,
       case JVM_CONSTANT_MethodType:
       case JVM_CONSTANT_MethodTypeInError: {
         *bytes = JVM_CONSTANT_MethodType;
-        idx1 = method_type_index_at_error_ok(idx);
+        idx1 = method_type_index_at(idx);
         Bytes::put_Java_u2((address) (bytes+1), idx1);
         DBG(printf("JVM_CONSTANT_MethodType: %hd", idx1));
         break;
@@ -1953,12 +1953,12 @@ void ConstantPool::print_entry_on(const int index, outputStream* st) {
       break;
     case JVM_CONSTANT_MethodHandle :
     case JVM_CONSTANT_MethodHandleInError :
-      st->print("ref_kind=%d", method_handle_ref_kind_at_error_ok(index));
-      st->print(" ref_index=%d", method_handle_index_at_error_ok(index));
+      st->print("ref_kind=%d", method_handle_ref_kind_at(index));
+      st->print(" ref_index=%d", method_handle_index_at(index));
       break;
     case JVM_CONSTANT_MethodType :
     case JVM_CONSTANT_MethodTypeInError :
-      st->print("signature_index=%d", method_type_index_at_error_ok(index));
+      st->print("signature_index=%d", method_type_index_at(index));
       break;
     case JVM_CONSTANT_InvokeDynamic :
       {

hotspot/src/share/vm/oops/constantPool.hpp

@@ -460,40 +460,20 @@ class ConstantPool : public Metadata {
     return *int_at_addr(which);
   }
 
- private:
-  int method_handle_ref_kind_at(int which, bool error_ok) {
+  int method_handle_ref_kind_at(int which) {
     assert(tag_at(which).is_method_handle() ||
-           (error_ok && tag_at(which).is_method_handle_in_error()), "Corrupted constant pool");
+           tag_at(which).is_method_handle_in_error(), "Corrupted constant pool");
     return extract_low_short_from_int(*int_at_addr(which));  // mask out unwanted ref_index bits
   }
-  int method_handle_index_at(int which, bool error_ok) {
+  int method_handle_index_at(int which) {
     assert(tag_at(which).is_method_handle() ||
-           (error_ok && tag_at(which).is_method_handle_in_error()), "Corrupted constant pool");
+           tag_at(which).is_method_handle_in_error(), "Corrupted constant pool");
     return extract_high_short_from_int(*int_at_addr(which));  // shift out unwanted ref_kind bits
   }
-  int method_type_index_at(int which, bool error_ok) {
-    assert(tag_at(which).is_method_type() ||
-           (error_ok && tag_at(which).is_method_type_in_error()), "Corrupted constant pool");
-    return *int_at_addr(which);
-  }
- public:
-  int method_handle_ref_kind_at(int which) {
-    return method_handle_ref_kind_at(which, false);
-  }
-  int method_handle_ref_kind_at_error_ok(int which) {
-    return method_handle_ref_kind_at(which, true);
-  }
-  int method_handle_index_at(int which) {
-    return method_handle_index_at(which, false);
-  }
-  int method_handle_index_at_error_ok(int which) {
-    return method_handle_index_at(which, true);
-  }
   int method_type_index_at(int which) {
-    return method_type_index_at(which, false);
-  }
-  int method_type_index_at_error_ok(int which) {
-    return method_type_index_at(which, true);
+    assert(tag_at(which).is_method_type() ||
+           tag_at(which).is_method_type_in_error(), "Corrupted constant pool");
+    return *int_at_addr(which);
   }
 
   // Derived queries:

hotspot/src/share/vm/prims/jvmtiRedefineClasses.cpp

@@ -1443,8 +1443,9 @@ jvmtiError VM_RedefineClasses::merge_cp_and_rewrite(
     return JVMTI_ERROR_INTERNAL;
   }
 
-  // Update the version number of the constant pool
+  // Update the version number of the constant pools (may keep scratch_cp)
   merge_cp->increment_and_save_version(old_cp->version());
+  scratch_cp->increment_and_save_version(old_cp->version());
 
   ResourceMark rm(THREAD);
   _index_map_count = 0;
@@ -3911,6 +3912,11 @@ void VM_RedefineClasses::redefine_single_class(jclass the_jclass,
     method->set_constants(scratch_class->constants());
   }
 
+  // NOTE: this doesn't work because you can redefine the same class in two
+  // threads, each getting their own constant pool data appended to the
+  // original constant pool.  In order for the new methods to work when they
+  // become old methods, they need to keep their updated copy of the constant pool.
+
   {
     // walk all previous versions of the klass
     InstanceKlass *ik = (InstanceKlass *)the_class();