8222005: ClassRedefinition crashes with: guarantee(false) failed: OLD…

… and/or OBSOLETE method(s) found

Remove optimizations from class redefinition that cause the guarantee hit

Reviewed-by: coleenp, dcubed

src/hotspot/share/oops/cpCache.cpp

@@ -561,15 +561,14 @@ oop ConstantPoolCacheEntry::appendix_if_resolved(const constantPoolHandle& cpool
 #if INCLUDE_JVMTI
 
 void log_adjust(const char* entry_type, Method* old_method, Method* new_method, bool* trace_name_printed) {
-  if (log_is_enabled(Info, redefine, class, update)) {
-    ResourceMark rm;
-    if (!(*trace_name_printed)) {
-      log_info(redefine, class, update)("adjust: name=%s", old_method->method_holder()->external_name());
-      *trace_name_printed = true;
-    }
-    log_debug(redefine, class, update, constantpool)
-          ("cpc %s entry update: %s(%s)", entry_type, new_method->name()->as_C_string(), new_method->signature()->as_C_string());
+  ResourceMark rm;
+
+  if (!(*trace_name_printed)) {
+    log_info(redefine, class, update)("adjust: name=%s", old_method->method_holder()->external_name());
+    *trace_name_printed = true;
   }
+  log_trace(redefine, class, update, constantpool)
+    ("cpc %s entry update: %s", entry_type, new_method->external_name());
 }
 
 // RedefineClasses() API support:
@@ -809,9 +808,13 @@ void ConstantPoolCache::adjust_method_entries(bool * trace_name_printed) {
 
 // the constant pool cache should never contain old or obsolete methods
 bool ConstantPoolCache::check_no_old_or_obsolete_entries() {
+  ResourceMark rm;
   for (int i = 1; i < length(); i++) {
-    if (entry_at(i)->get_interesting_method_entry() != NULL &&
-        !entry_at(i)->check_no_old_or_obsolete_entries()) {
+    Method* m = entry_at(i)->get_interesting_method_entry();
+    if (m != NULL && !entry_at(i)->check_no_old_or_obsolete_entries()) {
+      log_trace(redefine, class, update, constantpool)
+        ("cpcache check found old method entry: class: %s, old: %d, obsolete: %d, method: %s",
+         constant_pool()->pool_holder()->external_name(), m->is_old(), m->is_obsolete(), m->external_name());
       return false;
     }
   }

src/hotspot/share/oops/klassVtable.cpp

@@ -961,6 +961,8 @@ bool klassVtable::adjust_default_method(int vtable_index, Method* old_method, Me
 // search the vtable for uses of either obsolete or EMCP methods
 void klassVtable::adjust_method_entries(bool * trace_name_printed) {
   int prn_enabled = 0;
+  ResourceMark rm;
+
   for (int index = 0; index < length(); index++) {
     Method* old_method = unchecked_method_at(index);
     if (old_method == NULL || !old_method->is_old()) {
@@ -978,27 +980,29 @@ void klassVtable::adjust_method_entries(bool * trace_name_printed) {
       updated_default = adjust_default_method(index, old_method, new_method);
     }
 
-    if (log_is_enabled(Info, redefine, class, update)) {
-      ResourceMark rm;
-      if (!(*trace_name_printed)) {
-        log_info(redefine, class, update)
-          ("adjust: klassname=%s for methods from name=%s",
-           _klass->external_name(), old_method->method_holder()->external_name());
-        *trace_name_printed = true;
-      }
-      log_debug(redefine, class, update, vtables)
-        ("vtable method update: %s(%s), updated default = %s",
-         new_method->name()->as_C_string(), new_method->signature()->as_C_string(), updated_default ? "true" : "false");
+    if (!(*trace_name_printed)) {
+      log_info(redefine, class, update)
+        ("adjust: klassname=%s for methods from name=%s",
+         _klass->external_name(), old_method->method_holder()->external_name());
+      *trace_name_printed = true;
     }
+    log_trace(redefine, class, update, vtables)
+      ("vtable method update: class: %s method: %s, updated default = %s",
+       _klass->external_name(), new_method->external_name(), updated_default ? "true" : "false");
   }
 }
 
 // a vtable should never contain old or obsolete methods
 bool klassVtable::check_no_old_or_obsolete_entries() {
+  ResourceMark rm;
+
   for (int i = 0; i < length(); i++) {
     Method* m = unchecked_method_at(i);
     if (m != NULL &&
         (NOT_PRODUCT(!m->is_valid() ||) m->is_old() || m->is_obsolete())) {
+      log_trace(redefine, class, update, vtables)
+        ("vtable check found old method entry: class: %s old: %d obsolete: %d, method: %s",
+         _klass->external_name(), m->is_old(), m->is_obsolete(), m->external_name());
       return false;
     }
   }
@@ -1281,8 +1285,9 @@ void klassItable::initialize_itable_for_interface(int method_table_offset, Insta
 #if INCLUDE_JVMTI
 // search the itable for uses of either obsolete or EMCP methods
 void klassItable::adjust_method_entries(bool * trace_name_printed) {
-
+  ResourceMark rm;
   itableMethodEntry* ime = method_entry(0);
+
   for (int i = 0; i < _size_method_table; i++, ime++) {
     Method* old_method = ime->method();
     if (old_method == NULL || !old_method->is_old()) {
@@ -1292,25 +1297,27 @@ void klassItable::adjust_method_entries(bool * trace_name_printed) {
     Method* new_method = old_method->get_new_method();
     ime->initialize(new_method);
 
-    if (log_is_enabled(Info, redefine, class, update)) {
-      ResourceMark rm;
-      if (!(*trace_name_printed)) {
-        log_info(redefine, class, update)("adjust: name=%s", old_method->method_holder()->external_name());
-        *trace_name_printed = true;
-      }
-      log_trace(redefine, class, update, itables)
-        ("itable method update: %s(%s)", new_method->name()->as_C_string(), new_method->signature()->as_C_string());
+    if (!(*trace_name_printed)) {
+      log_info(redefine, class, update)("adjust: name=%s", old_method->method_holder()->external_name());
+      *trace_name_printed = true;
     }
+    log_trace(redefine, class, update, itables)
+      ("itable method update: class: %s method: %s", _klass->external_name(), new_method->external_name());
   }
 }
 
 // an itable should never contain old or obsolete methods
 bool klassItable::check_no_old_or_obsolete_entries() {
+  ResourceMark rm;
   itableMethodEntry* ime = method_entry(0);
+
   for (int i = 0; i < _size_method_table; i++) {
     Method* m = ime->method();
     if (m != NULL &&
         (NOT_PRODUCT(!m->is_valid() ||) m->is_old() || m->is_obsolete())) {
+      log_trace(redefine, class, update, itables)
+        ("itable check found old method entry: class: %s old: %d obsolete: %d, method: %s",
+         _klass->external_name(), m->is_old(), m->is_obsolete(), m->external_name());
       return false;
     }
     ime++;

src/hotspot/share/prims/jvmtiRedefineClasses.cpp

@@ -70,8 +70,9 @@ Method**  VM_RedefineClasses::_added_methods        = NULL;
 int       VM_RedefineClasses::_matching_methods_length = 0;
 int       VM_RedefineClasses::_deleted_methods_length  = 0;
 int       VM_RedefineClasses::_added_methods_length    = 0;
+
+// This flag is global as the constructor does not reset it:
 bool      VM_RedefineClasses::_has_redefined_Object = false;
-bool      VM_RedefineClasses::_has_null_class_loader = false;
 u8        VM_RedefineClasses::_id_counter = 0;
 
 VM_RedefineClasses::VM_RedefineClasses(jint class_count,
@@ -83,8 +84,6 @@ VM_RedefineClasses::VM_RedefineClasses(jint class_count,
   _any_class_has_resolved_methods = false;
   _res = JVMTI_ERROR_NONE;
   _the_class = NULL;
-  _has_redefined_Object = false;
-  _has_null_class_loader = false;
   _id = next_id();
 }
 
@@ -3598,7 +3597,10 @@ void VM_RedefineClasses::AdjustAndCleanMetadata::do_klass(Klass* k) {
   bool trace_name_printed = false;
 
   // If the class being redefined is java.lang.Object, we need to fix all
-  // array class vtables also
+  // array class vtables also. The _has_redefined_Object flag is global.
+  // Once the java.lang.Object has been redefined (by the current or one
+  // of the previous VM_RedefineClasses operations) we have to always
+  // adjust method entries for array classes.
   if (k->is_array_klass() && _has_redefined_Object) {
     k->vtable().adjust_method_entries(&trace_name_printed);
 
@@ -3616,22 +3618,6 @@ void VM_RedefineClasses::AdjustAndCleanMetadata::do_klass(Klass* k) {
       }
     }
 
-    // HotSpot specific optimization! HotSpot does not currently
-    // support delegation from the bootstrap class loader to a
-    // user-defined class loader. This means that if the bootstrap
-    // class loader is the initiating class loader, then it will also
-    // be the defining class loader. This also means that classes
-    // loaded by the bootstrap class loader cannot refer to classes
-    // loaded by a user-defined class loader. Note: a user-defined
-    // class loader can delegate to the bootstrap class loader.
-    //
-    // If the current class being redefined has a user-defined class
-    // loader as its defining class loader, then we can skip all
-    // classes loaded by the bootstrap class loader.
-    if (!_has_null_class_loader && ik->class_loader() == NULL) {
-      return;
-    }
-
     // Adjust all vtables, default methods and itables, to clean out old methods.
     ResourceMark rm(_thread);
     if (ik->vtable_length() > 0) {
@@ -3650,21 +3636,24 @@ void VM_RedefineClasses::AdjustAndCleanMetadata::do_klass(Klass* k) {
     // constant pool cache holds the Method*s for non-virtual
     // methods and for virtual, final methods.
     //
-    // Special case: if the current class being redefined, then new_cp
-    // has already been attached to the_class and old_cp has already
-    // been added as a previous version. The new_cp doesn't have any
-    // cached references to old methods so it doesn't need to be
-    // updated. We can simply start with the previous version(s) in
-    // that case.
+    // Special case: if the current class is being redefined by the current
+    // VM_RedefineClasses operation, then new_cp has already been attached
+    // to the_class and old_cp has already been added as a previous version.
+    // The new_cp doesn't have any cached references to old methods so it
+    // doesn't need to be updated and we could optimize by skipping it.
+    // However, the current class can be marked as being redefined by another
+    // VM_RedefineClasses operation which has already executed its doit_prologue
+    // and needs cpcache method entries adjusted. For simplicity, the cpcache
+    // update is done unconditionally. It should result in doing nothing for
+    // classes being redefined by the current VM_RedefineClasses operation.
+    // Method entries in the previous version(s) are adjusted as well.
     ConstantPoolCache* cp_cache;
 
-    if (!ik->is_being_redefined()) {
-      // this klass' constant pool cache may need adjustment
-      ConstantPool* other_cp = ik->constants();
-      cp_cache = other_cp->cache();
-      if (cp_cache != NULL) {
-        cp_cache->adjust_method_entries(&trace_name_printed);
-      }
+    // this klass' constant pool cache may need adjustment
+    ConstantPool* other_cp = ik->constants();
+    cp_cache = other_cp->cache();
+    if (cp_cache != NULL) {
+      cp_cache->adjust_method_entries(&trace_name_printed);
     }
 
     // the previous versions' constant pool caches may need adjustment
@@ -4109,9 +4098,8 @@ void VM_RedefineClasses::redefine_single_class(jclass the_jclass,
 
   InstanceKlass* the_class = get_ik(the_jclass);
 
-  // Set some flags to control and optimize adjusting method entries
+  // Set a flag to control and optimize adjusting method entries
   _has_redefined_Object |= the_class == SystemDictionary::Object_klass();
-  _has_null_class_loader |= the_class->class_loader() == NULL;
 
   // Remove all breakpoints in methods of this class
   JvmtiBreakpoints& jvmti_breakpoints = JvmtiCurrentBreakpoints::get_jvmti_breakpoints();