8055008: Clean up code that saves the previous versions of redefined classes

8156137: SIGSEGV in ReceiverTypeData::clean_weak_klass_links
8057570: RedefineClasses() tests fail assert(((Metadata*)obj)->is_valid()) failed: obj is valid
Reviewed-by: coleenp

src/share/vm/classfile/metadataOnStackMark.cpp

@@ -41,13 +41,13 @@ NOT_PRODUCT(bool MetadataOnStackMark::_is_active = false;)
 // Walk metadata on the stack and mark it so that redefinition doesn't delete
 // it.  Class unloading also walks the previous versions and might try to
 // delete it, so this class is used by class unloading also.
-MetadataOnStackMark::MetadataOnStackMark(bool visit_code_cache) {
+MetadataOnStackMark::MetadataOnStackMark(bool has_redefined_a_class) {
   assert(SafepointSynchronize::is_at_safepoint(), "sanity check");
   assert(_used_buffers == NULL, "sanity check");
   NOT_PRODUCT(_is_active = true;)
 
   Threads::metadata_do(Metadata::mark_on_stack);
-  if (visit_code_cache) {
+  if (has_redefined_a_class) {
     CodeCache::alive_nmethods_do(nmethod::mark_on_stack);
   }
   CompileBroker::mark_on_stack();

src/share/vm/classfile/metadataOnStackMark.hpp

@@ -47,7 +47,7 @@ class MetadataOnStackMark : public StackObj {
   static void retire_buffer(MetadataOnStackBuffer* buffer);
 
  public:
-  MetadataOnStackMark(bool visit_code_cache);
+  MetadataOnStackMark(bool has_redefined_a_class);
    ~MetadataOnStackMark();
 
   static void record(Metadata* m, Thread* thread);

src/share/vm/code/nmethod.cpp

@@ -2172,7 +2172,7 @@ void nmethod::metadata_do(void f(Metadata*)) {
                "metadata must be found in exactly one place");
         if (r->metadata_is_immediate() && r->metadata_value() != NULL) {
           Metadata* md = r->metadata_value();
-          f(md);
+          if (md != _method) f(md);
         }
       } else if (iter.type() == relocInfo::virtual_call_type) {
         // Check compiledIC holders associated with this nmethod
@@ -2198,7 +2198,7 @@ void nmethod::metadata_do(void f(Metadata*)) {
     f(md);
   }
 
-  // Visit metadata not embedded in the other places.
+  // Call function Method*, not embedded in these other places.
   if (_method != NULL) f(_method);
 }
 

src/share/vm/oops/instanceKlass.hpp

@@ -88,7 +88,6 @@ class BreakpointInfo;
 class fieldDescriptor;
 class DepChange;
 class nmethodBucket;
-class PreviousVersionNode;
 class JvmtiCachedClassFieldMap;
 class MemberNameTable;
 
@@ -235,7 +234,8 @@ class InstanceKlass: public Klass {
     _misc_is_anonymous             = 1 << 3, // has embedded _host_klass field
     _misc_is_contended             = 1 << 4, // marked with contended annotation
     _misc_has_default_methods      = 1 << 5, // class/superclass/implemented interfaces has default methods
-    _misc_declares_default_methods = 1 << 6  // directly declares default methods (any access)
+    _misc_declares_default_methods = 1 << 6, // directly declares default methods (any access)
+    _misc_has_been_redefined       = 1 << 7  // class has been redefined
   };
   u2              _misc_flags;
   u2              _minor_version;        // minor version number of class file
@@ -250,9 +250,8 @@ class InstanceKlass: public Klass {
   nmethodBucket*  _dependencies;         // list of dependent nmethods
   nmethod*        _osr_nmethods_head;    // Head of list of on-stack replacement nmethods for this class
   BreakpointInfo* _breakpoints;          // bpt lists, managed by Method*
-  // Array of interesting part(s) of the previous version(s) of this
-  // InstanceKlass. See PreviousVersionWalker below.
-  GrowableArray<PreviousVersionNode *>* _previous_versions;
+  // Linked instanceKlasses of previous versions
+  InstanceKlass* _previous_versions;
   // JVMTI fields can be moved to their own structure - see 6315920
   // JVMTI: cached class file, before retransformable agent modified it in CFLH
   JvmtiCachedClassFileData* _cached_class_file;
@@ -669,21 +668,31 @@ class InstanceKlass: public Klass {
   }
 
   // RedefineClasses() support for previous versions:
-  void add_previous_version(instanceKlassHandle ikh, BitMap *emcp_methods,
-         int emcp_method_count);
-  // If the _previous_versions array is non-NULL, then this klass
-  // has been redefined at least once even if we aren't currently
-  // tracking a previous version.
-  bool has_been_redefined() const { return _previous_versions != NULL; }
-  bool has_previous_version() const;
+  void add_previous_version(instanceKlassHandle ikh, int emcp_method_count);
+
+  InstanceKlass* previous_versions() const { return _previous_versions; }
+
+  bool has_been_redefined() const {
+    return (_misc_flags & _misc_has_been_redefined) != 0;
+  }
+  void set_has_been_redefined() {
+    _misc_flags |= _misc_has_been_redefined;
+  }
+
   void init_previous_versions() {
     _previous_versions = NULL;
   }
-  GrowableArray<PreviousVersionNode *>* previous_versions() const {
-    return _previous_versions;
+
+
+  InstanceKlass* get_klass_version(int version) {
+    for (InstanceKlass* ik = this; ik != NULL; ik = ik->previous_versions()) {
+      if (ik->constants()->version() == version) {
+        return ik;
+      }
+    }
+    return NULL;
   }
 
-  InstanceKlass* get_klass_version(int version);
   static void purge_previous_versions(InstanceKlass* ik);
 
   // JVMTI: Support for caching a class file before it is modified by an agent that can do retransformation
@@ -1124,6 +1133,10 @@ private:
 
   // Free CHeap allocated fields.
   void release_C_heap_structures();
+
+  // RedefineClasses support
+  void link_previous_versions(InstanceKlass* pv) { _previous_versions = pv; }
+  void mark_newly_obsolete_methods(Array<Method*>* old_methods, int emcp_method_count);
 public:
   // CDS support - remove and restore oops from metadata. Oops are not shared.
   virtual void remove_unshareable_info();
@@ -1222,62 +1235,6 @@ class JNIid: public CHeapObj<mtClass> {
 };
 
 
-// If breakpoints are more numerous than just JVMTI breakpoints,
-// consider compressing this data structure.
-// It is currently a simple linked list defined in method.hpp.
-
-class BreakpointInfo;
-
-
-// A collection point for interesting information about the previous
-// version(s) of an InstanceKlass.  A GrowableArray of PreviousVersionNodes
-// is attached to the InstanceKlass as needed. See PreviousVersionWalker below.
-class PreviousVersionNode : public CHeapObj<mtClass> {
- private:
-  ConstantPool*    _prev_constant_pool;
-
-  // If the previous version of the InstanceKlass doesn't have any
-  // EMCP methods, then _prev_EMCP_methods will be NULL. If all the
-  // EMCP methods have been collected, then _prev_EMCP_methods can
-  // have a length of zero.
-  GrowableArray<Method*>* _prev_EMCP_methods;
-
-public:
-  PreviousVersionNode(ConstantPool* prev_constant_pool,
-                      GrowableArray<Method*>* prev_EMCP_methods);
-  ~PreviousVersionNode();
-  ConstantPool* prev_constant_pool() const {
-    return _prev_constant_pool;
-  }
-  GrowableArray<Method*>* prev_EMCP_methods() const {
-    return _prev_EMCP_methods;
-  }
-};
-
-
-// Helper object for walking previous versions.
-class PreviousVersionWalker : public StackObj {
- private:
-  Thread*                               _thread;
-  GrowableArray<PreviousVersionNode *>* _previous_versions;
-  int                                   _current_index;
-
-  // A pointer to the current node object so we can handle the deletes.
-  PreviousVersionNode*                  _current_p;
-
-  // The constant pool handle keeps all the methods in this class from being
-  // deallocated from the metaspace during class unloading.
-  constantPoolHandle                    _current_constant_pool_handle;
-
- public:
-  PreviousVersionWalker(Thread* thread, InstanceKlass *ik);
-
-  // Return the interesting information for the next previous version
-  // of the klass. Returns NULL if there are no more previous versions.
-  PreviousVersionNode* next_previous_version();
-};
-
-
 //
 // nmethodBucket is used to record dependent nmethods for
 // deoptimization.  nmethod dependencies are actually <klass, method>

src/share/vm/oops/klass.cpp

@@ -455,6 +455,12 @@ void Klass::clean_weak_klass_links(BoolObjectClosure* is_alive, bool clean_alive
     if (clean_alive_klasses && current->oop_is_instance()) {
       InstanceKlass* ik = InstanceKlass::cast(current);
       ik->clean_weak_instanceklass_links(is_alive);
+
+      // JVMTI RedefineClasses creates previous versions that are not in
+      // the class hierarchy, so process them here.
+      while ((ik = ik->previous_versions()) != NULL) {
+        ik->clean_weak_instanceklass_links(is_alive);
+      }
     }
   }
 }

src/share/vm/oops/method.cpp

@@ -91,6 +91,7 @@ Method::Method(ConstMethod* xconst, AccessFlags access_flags, int size) {
   set_hidden(false);
   set_dont_inline(false);
   set_has_injected_profile(false);
+  set_running_emcp(false);
   set_method_data(NULL);
   clear_method_counters();
   set_vtable_index(Method::garbage_vtable_index);

src/share/vm/oops/method.hpp

@@ -111,6 +111,7 @@ class Method : public Metadata {
                     _caller_sensitive     : 1,
                     _force_inline         : 1,
                     _hidden               : 1,
+                    _running_emcp         : 1,
                     _dont_inline          : 1,
                     _has_injected_profile : 1,
                                           : 2;
@@ -712,6 +713,21 @@ class Method : public Metadata {
   void set_is_obsolete()                            { _access_flags.set_is_obsolete(); }
   bool is_deleted() const                           { return access_flags().is_deleted(); }
   void set_is_deleted()                             { _access_flags.set_is_deleted(); }
+
+  bool is_running_emcp() const {
+    // EMCP methods are old but not obsolete or deleted. Equivalent
+    // Modulo Constant Pool means the method is equivalent except
+    // the constant pool and instructions that access the constant
+    // pool might be different.
+    // If a breakpoint is set in a redefined method, its EMCP methods that are
+    // still running must have a breakpoint also.
+    return _running_emcp;
+  }
+
+  void set_running_emcp(bool x) {
+    _running_emcp = x;
+  }
+
   bool on_stack() const                             { return access_flags().on_stack(); }
   void set_on_stack(const bool value);
 

src/share/vm/prims/jvmtiImpl.cpp

@@ -282,39 +282,22 @@ address JvmtiBreakpoint::getBcp() {
 void JvmtiBreakpoint::each_method_version_do(method_action meth_act) {
   ((Method*)_method->*meth_act)(_bci);
 
-  // add/remove breakpoint to/from versions of the method that
-  // are EMCP. Directly or transitively obsolete methods are
-  // not saved in the PreviousVersionNodes.
+  // add/remove breakpoint to/from versions of the method that are EMCP.
   Thread *thread = Thread::current();
   instanceKlassHandle ikh = instanceKlassHandle(thread, _method->method_holder());
   Symbol* m_name = _method->name();
   Symbol* m_signature = _method->signature();
 
   // search previous versions if they exist
-  PreviousVersionWalker pvw(thread, (InstanceKlass *)ikh());
-  for (PreviousVersionNode * pv_node = pvw.next_previous_version();
-       pv_node != NULL; pv_node = pvw.next_previous_version()) {
-    GrowableArray<Method*>* methods = pv_node->prev_EMCP_methods();
-
-    if (methods == NULL) {
-      // We have run into a PreviousVersion generation where
-      // all methods were made obsolete during that generation's
-      // RedefineClasses() operation. At the time of that
-      // operation, all EMCP methods were flushed so we don't
-      // have to go back any further.
-      //
-      // A NULL methods array is different than an empty methods
-      // array. We cannot infer any optimizations about older
-      // generations from an empty methods array for the current
-      // generation.
-      break;
-    }
+  for (InstanceKlass* pv_node = ikh->previous_versions();
+       pv_node != NULL;
+       pv_node = pv_node->previous_versions()) {
+    Array<Method*>* methods = pv_node->methods();
 
     for (int i = methods->length() - 1; i >= 0; i--) {
       Method* method = methods->at(i);
-      // obsolete methods that are running are not deleted from
-      // previous version array, but they are skipped here.
-      if (!method->is_obsolete() &&
+      // Only set breakpoints in running EMCP methods.
+      if (method->is_running_emcp() &&
           method->name() == m_name &&
           method->signature() == m_signature) {
         RC_TRACE(0x00000800, ("%sing breakpoint in %s(%s)",

src/share/vm/prims/jvmtiRedefineClasses.cpp

@@ -3435,13 +3435,12 @@ void VM_RedefineClasses::AdjustCpoolCacheAndVtable::do_klass(Klass* k) {
     }
 
     // the previous versions' constant pool caches may need adjustment
-    PreviousVersionWalker pvw(_thread, ik);
-    for (PreviousVersionNode * pv_node = pvw.next_previous_version();
-         pv_node != NULL; pv_node = pvw.next_previous_version()) {
-      other_cp = pv_node->prev_constant_pool();
-      cp_cache = other_cp->cache();
+    for (InstanceKlass* pv_node = ik->previous_versions();
+         pv_node != NULL;
+         pv_node = pv_node->previous_versions()) {
+      cp_cache = pv_node->constants()->cache();
       if (cp_cache != NULL) {
-        cp_cache->adjust_method_entries(other_cp->pool_holder(), &trace_name_printed);
+        cp_cache->adjust_method_entries(pv_node, &trace_name_printed);
       }
     }
   }
@@ -3461,9 +3460,8 @@ void VM_RedefineClasses::update_jmethod_ids() {
   }
 }
 
-void VM_RedefineClasses::check_methods_and_mark_as_obsolete(
-       BitMap *emcp_methods, int * emcp_method_count_p) {
-  *emcp_method_count_p = 0;
+int VM_RedefineClasses::check_methods_and_mark_as_obsolete() {
+  int emcp_method_count = 0;
   int obsolete_count = 0;
   int old_index = 0;
   for (int j = 0; j < _matching_methods_length; ++j, ++old_index) {
@@ -3537,9 +3535,9 @@ void VM_RedefineClasses::check_methods_and_mark_as_obsolete(
       // that we get from effectively overwriting the old methods
       // when the new methods are attached to the_class.
 
-      // track which methods are EMCP for add_previous_version() call
-      emcp_methods->set_bit(old_index);
-      (*emcp_method_count_p)++;
+      // Count number of methods that are EMCP.  The method will be marked
+      // old but not obsolete if it is EMCP.
+      emcp_method_count++;
 
       // An EMCP method is _not_ obsolete. An obsolete method has a
       // different jmethodID than the current method. An EMCP method
@@ -3589,10 +3587,11 @@ void VM_RedefineClasses::check_methods_and_mark_as_obsolete(
                           old_method->name()->as_C_string(),
                           old_method->signature()->as_C_string()));
   }
-  assert((*emcp_method_count_p + obsolete_count) == _old_methods->length(),
+  assert((emcp_method_count + obsolete_count) == _old_methods->length(),
     "sanity check");
-  RC_TRACE(0x00000100, ("EMCP_cnt=%d, obsolete_cnt=%d", *emcp_method_count_p,
+  RC_TRACE(0x00000100, ("EMCP_cnt=%d, obsolete_cnt=%d", emcp_method_count,
     obsolete_count));
+  return emcp_method_count;
 }
 
 // This internal class transfers the native function registration from old methods
@@ -3973,11 +3972,8 @@ void VM_RedefineClasses::redefine_single_class(jclass the_jclass,
   old_constants->set_pool_holder(scratch_class());
 #endif
 
-  // track which methods are EMCP for add_previous_version() call below
-  BitMap emcp_methods(_old_methods->length());
-  int emcp_method_count = 0;
-  emcp_methods.clear();  // clears 0..(length() - 1)
-  check_methods_and_mark_as_obsolete(&emcp_methods, &emcp_method_count);
+  // track number of methods that are EMCP for add_previous_version() call below
+  int emcp_method_count = check_methods_and_mark_as_obsolete();
   transfer_old_native_function_registrations(the_class);
 
   // The class file bytes from before any retransformable agents mucked
@@ -4064,9 +4060,10 @@ void VM_RedefineClasses::redefine_single_class(jclass the_jclass,
     scratch_class->enclosing_method_method_index());
   scratch_class->set_enclosing_method_indices(old_class_idx, old_method_idx);
 
+  the_class->set_has_been_redefined();
+
   // keep track of previous versions of this class
-  the_class->add_previous_version(scratch_class, &emcp_methods,
-    emcp_method_count);
+  the_class->add_previous_version(scratch_class, emcp_method_count);
 
   RC_TIMER_STOP(_timer_rsc_phase1);
   RC_TIMER_START(_timer_rsc_phase2);

src/share/vm/prims/jvmtiRedefineClasses.hpp

@@ -403,14 +403,9 @@ class VM_RedefineClasses: public VM_Operation {
   // Change jmethodIDs to point to the new methods
   void update_jmethod_ids();
 
-  // In addition to marking methods as obsolete, this routine
-  // records which methods are EMCP (Equivalent Module Constant
-  // Pool) in the emcp_methods BitMap and returns the number of
-  // EMCP methods via emcp_method_count_p. This information is
-  // used when information about the previous version of the_class
-  // is squirreled away.
-  void check_methods_and_mark_as_obsolete(BitMap *emcp_methods,
-         int * emcp_method_count_p);
+  // In addition to marking methods as old and/or obsolete, this routine
+  // counts the number of methods that are EMCP (Equivalent Module Constant Pool).
+  int check_methods_and_mark_as_obsolete();
   void transfer_old_native_function_registrations(instanceKlassHandle the_class);
 
   // Install the redefinition of a class

test/runtime/RedefineFinalizer/RedefineFinalizer.java → test/runtime/RedefineTests/RedefineFinalizer.java

 /*
- * Copyright (c) 2014, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2014, 2018, 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

test/runtime/RedefineTests/RedefineRunningMethods.java

+/*
+ * Copyright (c) 2014, 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.
+ */
+
+/*
+ * @test
+ * @bug 8055008
+ * @summary Redefine EMCP and non-EMCP methods that are running in an infinite loop
+ * @library /testlibrary
+ * @build RedefineClassHelper
+ * @run main RedefineClassHelper
+ * @run main/othervm -javaagent:redefineagent.jar RedefineRunningMethods
+ */
+public class RedefineRunningMethods {
+
+    public static String newB =
+                "class RedefineRunningMethods$B {" +
+                "   static int count1 = 0;" +
+                "   static int count2 = 0;" +
+                "   public static volatile boolean stop = false;" +
+                "  static void localSleep() { " +
+                "    try{ " +
+                "      Thread.currentThread().sleep(10);" +
+                "    } catch(InterruptedException ie) { " +
+                "    } " +
+                " } " +
+                "   public static void infinite() { " +
+                "       System.out.println(\"infinite called\");" +
+                "   }" +
+                "   public static void infinite_emcp() { " +
+                "       while (!stop) { count2++; localSleep(); }" +
+                "   }" +
+                "}";
+
+    public static String evenNewerB =
+                "class RedefineRunningMethods$B {" +
+                "   static int count1 = 0;" +
+                "   static int count2 = 0;" +
+                "   public static volatile boolean stop = false;" +
+                "  static void localSleep() { " +
+                "    try{ " +
+                "      Thread.currentThread().sleep(1);" +
+                "    } catch(InterruptedException ie) { " +
+                "    } " +
+                " } " +
+                "   public static void infinite() { }" +
+                "   public static void infinite_emcp() { " +
+                "       System.out.println(\"infinite_emcp now obsolete called\");" +
+                "   }" +
+                "}";
+
+    static class B {
+        static int count1 = 0;
+        static int count2 = 0;
+        public static volatile boolean stop = false;
+        static void localSleep() {
+          try{
+            Thread.currentThread().sleep(10);//sleep for 10 ms
+          } catch(InterruptedException ie) {
+          }
+        }
+
+        public static void infinite() {
+            while (!stop) { count1++; localSleep(); }
+        }
+        public static void infinite_emcp() {
+            while (!stop) { count2++; localSleep(); }
+        }
+    }
+
+
+    public static void main(String[] args) throws Exception {
+
+        new Thread() {
+            public void run() {
+                B.infinite();
+            }
+        }.start();
+
+        new Thread() {
+            public void run() {
+                B.infinite_emcp();
+            }
+        }.start();
+
+        RedefineClassHelper.redefineClass(B.class, newB);
+
+        System.gc();
+
+        B.infinite();
+
+        // Start a thread with the second version of infinite_emcp running
+        new Thread() {
+            public void run() {
+                B.infinite_emcp();
+            }
+        }.start();
+
+        for (int i = 0; i < 20 ; i++) {
+            String s = new String("some garbage");
+            System.gc();
+        }
+
+        RedefineClassHelper.redefineClass(B.class, evenNewerB);
+        System.gc();
+
+        for (int i = 0; i < 20 ; i++) {
+            B.infinite();
+            String s = new String("some garbage");
+            System.gc();
+        }
+
+        B.infinite_emcp();
+
+        // purge should clean everything up.
+        B.stop = true;
+
+        for (int i = 0; i < 20 ; i++) {
+            B.infinite();
+            String s = new String("some garbage");
+            System.gc();
+        }
+    }
+}