8005056: NPG: Crash after redefining java.lang.Object

Summary: Need to walk array class vtables replacing old methods too if j.l.o redefined
Reviewed-by: sspitsyn, dcubed, ctornqvi

src/share/vm/classfile/dictionary.cpp

@@ -253,22 +253,6 @@ void Dictionary::classes_do(void f(Klass*, TRAPS), TRAPS) {
   }
 }
 
-
-//   All classes, and their class loaders
-//   (added for helpers that use HandleMarks and ResourceMarks)
-// Don't iterate over placeholders
-void Dictionary::classes_do(void f(Klass*, ClassLoaderData*, TRAPS), TRAPS) {
-  for (int index = 0; index < table_size(); index++) {
-    for (DictionaryEntry* probe = bucket(index);
-                          probe != NULL;
-                          probe = probe->next()) {
-      Klass* k = probe->klass();
-      f(k, probe->loader_data(), CHECK);
-    }
-  }
-}
-
-
 //   All classes, and their class loaders
 // Don't iterate over placeholders
 void Dictionary::classes_do(void f(Klass*, ClassLoaderData*)) {

src/share/vm/classfile/dictionary.hpp

@@ -90,7 +90,6 @@ public:
   void classes_do(void f(Klass*));
   void classes_do(void f(Klass*, TRAPS), TRAPS);
   void classes_do(void f(Klass*, ClassLoaderData*));
-  void classes_do(void f(Klass*, ClassLoaderData*, TRAPS), TRAPS);
 
   void methods_do(void f(Method*));
 

src/share/vm/classfile/systemDictionary.cpp

@@ -1747,13 +1747,6 @@ void SystemDictionary::classes_do(void f(Klass*, ClassLoaderData*)) {
   dictionary()->classes_do(f);
 }
 
-//   All classes, and their class loaders
-//   (added for helpers that use HandleMarks and ResourceMarks)
-// Don't iterate over placeholders
-void SystemDictionary::classes_do(void f(Klass*, ClassLoaderData*, TRAPS), TRAPS) {
-  dictionary()->classes_do(f, CHECK);
-}
-
 void SystemDictionary::placeholders_do(void f(Symbol*)) {
   placeholders()->entries_do(f);
 }

src/share/vm/classfile/systemDictionary.hpp

@@ -313,10 +313,7 @@ public:
   static void classes_do(void f(Klass*, TRAPS), TRAPS);
   //   All classes, and their class loaders
   static void classes_do(void f(Klass*, ClassLoaderData*));
-  //   All classes, and their class loaders
-  //   (added for helpers that use HandleMarks and ResourceMarks)
-  static void classes_do(void f(Klass*, ClassLoaderData*, TRAPS), TRAPS);
-  // All entries in the placeholder table and their class loaders
+
   static void placeholders_do(void f(Symbol*));
 
   // Iterate over all methods in all klasses in dictionary

src/share/vm/oops/klass.hpp

@@ -393,6 +393,7 @@ class Klass : public Metadata {
 
   // vtables
   virtual klassVtable* vtable() const        { return NULL; }
+  virtual int vtable_length() const          { return 0; }
 
   // subclass check
   bool is_subclass_of(const Klass* k) const;

src/share/vm/prims/jvmtiRedefineClasses.cpp

@@ -160,7 +160,8 @@ void VM_RedefineClasses::doit() {
   if (RC_TRACE_ENABLED(0x00004000)) {
 #endif
     RC_TRACE_WITH_THREAD(0x00004000, thread, ("calling check_class"));
-    SystemDictionary::classes_do(check_class, thread);
+    CheckClass check_class(thread);
+    ClassLoaderDataGraph::classes_do(&check_class);
 #ifdef PRODUCT
   }
 #endif
@@ -2653,29 +2654,35 @@ void VM_RedefineClasses::set_new_constant_pool(
 } // end set_new_constant_pool()
 
 
-void VM_RedefineClasses::adjust_array_vtable(Klass* k_oop) {
-  ArrayKlass* ak = ArrayKlass::cast(k_oop);
-  bool trace_name_printed = false;
-  ak->vtable()->adjust_method_entries(_matching_old_methods,
-                                      _matching_new_methods,
-                                      _matching_methods_length,
-                                      &trace_name_printed);
-}
-
 // Unevolving classes may point to methods of the_class directly
 // from their constant pool caches, itables, and/or vtables. We
-// use the SystemDictionary::classes_do() facility and this helper
+// use the ClassLoaderDataGraph::classes_do() facility and this helper
 // to fix up these pointers.
-//
-// Note: We currently don't support updating the vtable in
-// arrayKlassOops. See Open Issues in jvmtiRedefineClasses.hpp.
-void VM_RedefineClasses::adjust_cpool_cache_and_vtable(Klass* k_oop,
-       ClassLoaderData* initiating_loader,
-       TRAPS) {
-  Klass *k = k_oop;
-  if (k->oop_is_instance()) {
-    HandleMark hm(THREAD);
-    InstanceKlass *ik = (InstanceKlass *) k;
+
+// Adjust cpools and vtables closure
+void VM_RedefineClasses::AdjustCpoolCacheAndVtable::do_klass(Klass* k) {
+
+  // This is a very busy routine. We don't want too much tracing
+  // printed out.
+  bool trace_name_printed = false;
+
+  // Very noisy: only enable this call if you are trying to determine
+  // that a specific class gets found by this routine.
+  // RC_TRACE macro has an embedded ResourceMark
+  // RC_TRACE_WITH_THREAD(0x00100000, THREAD,
+  //   ("adjust check: name=%s", k->external_name()));
+  // trace_name_printed = true;
+
+  // If the class being redefined is java.lang.Object, we need to fix all
+  // array class vtables also
+  if (k->oop_is_array() && _the_class_oop == SystemDictionary::Object_klass()) {
+    k->vtable()->adjust_method_entries(_matching_old_methods,
+                                       _matching_new_methods,
+                                       _matching_methods_length,
+                                       &trace_name_printed);
+  } else if (k->oop_is_instance()) {
+    HandleMark hm(_thread);
+    InstanceKlass *ik = InstanceKlass::cast(k);
 
     // HotSpot specific optimization! HotSpot does not currently
     // support delegation from the bootstrap class loader to a
@@ -2695,23 +2702,6 @@ void VM_RedefineClasses::adjust_cpool_cache_and_vtable(Klass* k_oop,
       return;
     }
 
-    // If the class being redefined is java.lang.Object, we need to fix all
-    // array class vtables also
-    if (_the_class_oop == SystemDictionary::Object_klass()) {
-      ik->array_klasses_do(adjust_array_vtable);
-    }
-
-    // This is a very busy routine. We don't want too much tracing
-    // printed out.
-    bool trace_name_printed = false;
-
-    // Very noisy: only enable this call if you are trying to determine
-    // that a specific class gets found by this routine.
-    // RC_TRACE macro has an embedded ResourceMark
-    // RC_TRACE_WITH_THREAD(0x00100000, THREAD,
-    //   ("adjust check: name=%s", ik->external_name()));
-    // trace_name_printed = true;
-
     // Fix the vtable embedded in the_class and subclasses of the_class,
     // if one exists. We discard scratch_class and we don't keep an
     // InstanceKlass around to hold obsolete methods so we don't have
@@ -2719,7 +2709,7 @@ void VM_RedefineClasses::adjust_cpool_cache_and_vtable(Klass* k_oop,
     // holds the Method*s for virtual (but not final) methods.
     if (ik->vtable_length() > 0 && ik->is_subtype_of(_the_class_oop)) {
       // ik->vtable() creates a wrapper object; rm cleans it up
-      ResourceMark rm(THREAD);
+      ResourceMark rm(_thread);
       ik->vtable()->adjust_method_entries(_matching_old_methods,
                                           _matching_new_methods,
                                           _matching_methods_length,
@@ -2735,7 +2725,7 @@ void VM_RedefineClasses::adjust_cpool_cache_and_vtable(Klass* k_oop,
     if (ik->itable_length() > 0 && (_the_class_oop->is_interface()
         || ik->is_subclass_of(_the_class_oop))) {
       // ik->itable() creates a wrapper object; rm cleans it up
-      ResourceMark rm(THREAD);
+      ResourceMark rm(_thread);
       ik->itable()->adjust_method_entries(_matching_old_methods,
                                           _matching_new_methods,
                                           _matching_methods_length,
@@ -2758,7 +2748,7 @@ void VM_RedefineClasses::adjust_cpool_cache_and_vtable(Klass* k_oop,
     constantPoolHandle other_cp;
     ConstantPoolCache* cp_cache;
 
-    if (k_oop != _the_class_oop) {
+    if (ik != _the_class_oop) {
       // this klass' constant pool cache may need adjustment
       other_cp = constantPoolHandle(ik->constants());
       cp_cache = other_cp->cache();
@@ -2770,7 +2760,7 @@ void VM_RedefineClasses::adjust_cpool_cache_and_vtable(Klass* k_oop,
       }
     }
     {
-      ResourceMark rm(THREAD);
+      ResourceMark rm(_thread);
       // PreviousVersionInfo objects returned via PreviousVersionWalker
       // contain a GrowableArray of handles. We have to clean up the
       // GrowableArray _after_ the PreviousVersionWalker destructor
@@ -3208,7 +3198,7 @@ void VM_RedefineClasses::swap_annotations(instanceKlassHandle the_class,
 //      parts of the_class
 //    - adjusting constant pool caches and vtables in other classes
 //      that refer to methods in the_class. These adjustments use the
-//      SystemDictionary::classes_do() facility which only allows
+//      ClassLoaderDataGraph::classes_do() facility which only allows
 //      a helper method to be specified. The interesting parameters
 //      that we would like to pass to the helper method are saved in
 //      static global fields in the VM operation.
@@ -3442,7 +3432,8 @@ void VM_RedefineClasses::redefine_single_class(jclass the_jclass,
 
   // Adjust constantpool caches and vtables for all classes
   // that reference methods of the evolved class.
-  SystemDictionary::classes_do(adjust_cpool_cache_and_vtable, THREAD);
+  AdjustCpoolCacheAndVtable adjust_cpool_cache_and_vtable(THREAD);
+  ClassLoaderDataGraph::classes_do(&adjust_cpool_cache_and_vtable);
 
   // JSR-292 support
   MemberNameTable* mnt = the_class->member_names();
@@ -3503,34 +3494,33 @@ void VM_RedefineClasses::increment_class_counter(InstanceKlass *ik, TRAPS) {
   }
 }
 
-void VM_RedefineClasses::check_class(Klass* k_oop,
-                                     ClassLoaderData* initiating_loader,
-                                     TRAPS) {
-  Klass *k = k_oop;
-  if (k->oop_is_instance()) {
-    HandleMark hm(THREAD);
-    InstanceKlass *ik = (InstanceKlass *) k;
-    bool no_old_methods = true;  // be optimistic
-    ResourceMark rm(THREAD);
+void VM_RedefineClasses::CheckClass::do_klass(Klass* k) {
+  bool no_old_methods = true;  // be optimistic
 
-    // a vtable should never contain old or obsolete methods
-    if (ik->vtable_length() > 0 &&
-        !ik->vtable()->check_no_old_or_obsolete_entries()) {
-      if (RC_TRACE_ENABLED(0x00004000)) {
-        RC_TRACE_WITH_THREAD(0x00004000, THREAD,
-          ("klassVtable::check_no_old_or_obsolete_entries failure"
-           " -- OLD or OBSOLETE method found -- class: %s",
-           ik->signature_name()));
-        ik->vtable()->dump_vtable();
-      }
-      no_old_methods = false;
+  // Both array and instance classes have vtables.
+  // a vtable should never contain old or obsolete methods
+  ResourceMark rm(_thread);
+  if (k->vtable_length() > 0 &&
+      !k->vtable()->check_no_old_or_obsolete_entries()) {
+    if (RC_TRACE_ENABLED(0x00004000)) {
+      RC_TRACE_WITH_THREAD(0x00004000, _thread,
+        ("klassVtable::check_no_old_or_obsolete_entries failure"
+         " -- OLD or OBSOLETE method found -- class: %s",
+         k->signature_name()));
+      k->vtable()->dump_vtable();
     }
+    no_old_methods = false;
+  }
+
+  if (k->oop_is_instance()) {
+    HandleMark hm(_thread);
+    InstanceKlass *ik = InstanceKlass::cast(k);
 
     // an itable should never contain old or obsolete methods
     if (ik->itable_length() > 0 &&
         !ik->itable()->check_no_old_or_obsolete_entries()) {
       if (RC_TRACE_ENABLED(0x00004000)) {
-        RC_TRACE_WITH_THREAD(0x00004000, THREAD,
+        RC_TRACE_WITH_THREAD(0x00004000, _thread,
           ("klassItable::check_no_old_or_obsolete_entries failure"
            " -- OLD or OBSOLETE method found -- class: %s",
            ik->signature_name()));
@@ -3544,7 +3534,7 @@ void VM_RedefineClasses::check_class(Klass* k_oop,
         ik->constants()->cache() != NULL &&
         !ik->constants()->cache()->check_no_old_or_obsolete_entries()) {
       if (RC_TRACE_ENABLED(0x00004000)) {
-        RC_TRACE_WITH_THREAD(0x00004000, THREAD,
+        RC_TRACE_WITH_THREAD(0x00004000, _thread,
           ("cp-cache::check_no_old_or_obsolete_entries failure"
            " -- OLD or OBSOLETE method found -- class: %s",
            ik->signature_name()));
@@ -3552,19 +3542,21 @@ void VM_RedefineClasses::check_class(Klass* k_oop,
       }
       no_old_methods = false;
     }
+  }
 
-    if (!no_old_methods) {
-      if (RC_TRACE_ENABLED(0x00004000)) {
-        dump_methods();
-      } else {
-        tty->print_cr("INFO: use the '-XX:TraceRedefineClasses=16384' option "
-          "to see more info about the following guarantee() failure.");
-      }
-      guarantee(false, "OLD and/or OBSOLETE method(s) found");
+  // print and fail guarantee if old methods are found.
+  if (!no_old_methods) {
+    if (RC_TRACE_ENABLED(0x00004000)) {
+      dump_methods();
+    } else {
+      tty->print_cr("INFO: use the '-XX:TraceRedefineClasses=16384' option "
+        "to see more info about the following guarantee() failure.");
     }
+    guarantee(false, "OLD and/or OBSOLETE method(s) found");
   }
 }
 
+
 void VM_RedefineClasses::dump_methods() {
   int j;
   RC_TRACE(0x00004000, ("_old_methods --"));

src/share/vm/prims/jvmtiRedefineClasses.hpp

@@ -87,7 +87,7 @@
 //      parts of the_class
 //    - adjusting constant pool caches and vtables in other classes
 //      that refer to methods in the_class. These adjustments use the
-//      SystemDictionary::classes_do() facility which only allows
+//      ClassLoaderDataGraph::classes_do() facility which only allows
 //      a helper method to be specified. The interesting parameters
 //      that we would like to pass to the helper method are saved in
 //      static global fields in the VM operation.
@@ -333,8 +333,8 @@
 
 class VM_RedefineClasses: public VM_Operation {
  private:
-  // These static fields are needed by SystemDictionary::classes_do()
-  // facility and the adjust_cpool_cache_and_vtable() helper:
+  // These static fields are needed by ClassLoaderDataGraph::classes_do()
+  // facility and the AdjustCpoolCacheAndVtable helper:
   static Array<Method*>* _old_methods;
   static Array<Method*>* _new_methods;
   static Method**      _matching_old_methods;
@@ -408,13 +408,6 @@ class VM_RedefineClasses: public VM_Operation {
          int * emcp_method_count_p);
   void transfer_old_native_function_registrations(instanceKlassHandle the_class);
 
-  // Unevolving classes may point to methods of the_class directly
-  // from their constant pool caches, itables, and/or vtables. We
-  // use the SystemDictionary::classes_do() facility and this helper
-  // to fix up these pointers.
-  static void adjust_cpool_cache_and_vtable(Klass* k_oop, ClassLoaderData* initiating_loader, TRAPS);
-  static void adjust_array_vtable(Klass* k_oop);
-
   // Install the redefinition of a class
   void redefine_single_class(jclass the_jclass,
     Klass* scratch_class_oop, TRAPS);
@@ -480,10 +473,27 @@ class VM_RedefineClasses: public VM_Operation {
 
   void flush_dependent_code(instanceKlassHandle k_h, TRAPS);
 
-  static void check_class(Klass* k_oop, ClassLoaderData* initiating_loader,
-                TRAPS);
   static void dump_methods();
 
+  // Check that there are no old or obsolete methods
+  class CheckClass : public KlassClosure {
+    Thread* _thread;
+   public:
+    CheckClass(Thread* t) : _thread(t) {}
+    void do_klass(Klass* k);
+  };
+
+  // Unevolving classes may point to methods of the_class directly
+  // from their constant pool caches, itables, and/or vtables. We
+  // use the ClassLoaderDataGraph::classes_do() facility and this helper
+  // to fix up these pointers.
+  class AdjustCpoolCacheAndVtable : public KlassClosure {
+    Thread* _thread;
+   public:
+    AdjustCpoolCacheAndVtable(Thread* t) : _thread(t) {}
+    void do_klass(Klass* k);
+  };
+
  public:
   VM_RedefineClasses(jint class_count,
                      const jvmtiClassDefinition *class_defs,

test/runtime/RedefineObject/Agent.java

+/*
+ * Copyright (c) 2013, Oracle and/or its affiliates. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+import java.security.*;
+import java.lang.instrument.*;
+
+public class Agent implements ClassFileTransformer {
+    public synchronized byte[] transform(final ClassLoader classLoader,
+                                         final String className,
+                                         Class<?> classBeingRedefined,
+                                         ProtectionDomain protectionDomain,
+                                         byte[] classfileBuffer) {
+        //System.out.println("Transforming class " + className);
+        return classfileBuffer;
+    }
+
+    public static void premain(String agentArgs, Instrumentation instrumentation) {
+
+        Agent transformer = new Agent();
+
+        instrumentation.addTransformer(transformer, true);
+
+        Class c = Object.class;
+        try {
+            instrumentation.retransformClasses(c);
+        } catch (Exception e) {
+            e.printStackTrace();
+        }
+
+        instrumentation.removeTransformer(transformer);
+    }
+
+    public static void main(String[] args) {
+        byte[] ba = new byte[0];
+
+        // If it survives 1000 GC's, it's good.
+        for (int i = 0; i < 1000 ; i++) {
+            System.gc();
+            ba.clone();
+        }
+    }
+}

test/runtime/RedefineObject/TestRedefineObject.java

+/*
+ * Copyright (c) 2013, Oracle and/or its affiliates. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+import java.io.PrintWriter;
+import com.oracle.java.testlibrary.*;
+
+/*
+ * Test to redefine java/lang/Object and verify that it doesn't crash on vtable
+ * call on basic array type.
+ *
+ * @test
+ * @bug 8005056
+ * @library /testlibrary
+ * @build Agent
+ * @run main ClassFileInstaller Agent
+ * @run main Test
+ * @run main/othervm -javaagent:agent.jar Agent
+ */
+public class Test {
+    public static void main(String[] args) throws Exception  {
+
+      PrintWriter pw = new PrintWriter("MANIFEST.MF");
+      pw.println("Premain-Class: Agent");
+      pw.println("Can-Retransform-Classes: true");
+      pw.close();
+
+      ProcessBuilder pb = new ProcessBuilder();
+      pb.command(new String[] { JDKToolFinder.getJDKTool("jar"), "cmf", "MANIFEST.MF", "agent.jar", "Agent.class"});
+      pb.start().waitFor();
+    }
+}

test/testlibrary/ClassFileInstaller.java

@@ -45,7 +45,9 @@ public class ClassFileInstaller {
 
             // Create the class file's package directory
             Path p = Paths.get(pathName);
-            Files.createDirectories(p.getParent());
+            if (pathName.contains("/")) {
+                Files.createDirectories(p.getParent());
+            }
             // Create the class file
             Files.copy(is, p, StandardCopyOption.REPLACE_EXISTING);
         }