8005494: SIGSEGV in Rewriter::relocate_and_link() when testing Weblogic with...

8005494: SIGSEGV in Rewriter::relocate_and_link() when testing Weblogic with CompressedOops and KlassPtrs
Summary: Relocate functions with jsr's when rewriting so not repeated after reading shared archive
Reviewed-by: twisti, jrose

hotspot/src/share/vm/interpreter/rewriter.cpp

@@ -27,13 +27,8 @@
 #include "interpreter/interpreter.hpp"
 #include "interpreter/rewriter.hpp"
 #include "memory/gcLocker.hpp"
-#include "memory/metadataFactory.hpp"
-#include "memory/oopFactory.hpp"
 #include "memory/resourceArea.hpp"
 #include "oops/generateOopMap.hpp"
-#include "oops/objArrayOop.hpp"
-#include "oops/oop.inline.hpp"
-#include "prims/methodComparator.hpp"
 #include "prims/methodHandles.hpp"
 
 // Computes a CPC map (new_index -> original_index) for constant pool entries
@@ -402,13 +397,6 @@ void Rewriter::rewrite(instanceKlassHandle klass, TRAPS) {
 }
 
 
-void Rewriter::rewrite(instanceKlassHandle klass, constantPoolHandle cpool, Array<Method*>* methods, TRAPS) {
-  ResourceMark rm(THREAD);
-  Rewriter     rw(klass, cpool, methods, CHECK);
-  // (That's all, folks.)
-}
-
-
 Rewriter::Rewriter(instanceKlassHandle klass, constantPoolHandle cpool, Array<Method*>* methods, TRAPS)
   : _klass(klass),
     _pool(cpool),
@@ -453,46 +441,25 @@ Rewriter::Rewriter(instanceKlassHandle klass, constantPoolHandle cpool, Array<Me
     restore_bytecodes();
     return;
   }
-}
 
-// Relocate jsr/rets in a method.  This can't be done with the rewriter
-// stage because it can throw other exceptions, leaving the bytecodes
-// pointing at constant pool cache entries.
-// Link and check jvmti dependencies while we're iterating over the methods.
-// JSR292 code calls with a different set of methods, so two entry points.
-void Rewriter::relocate_and_link(instanceKlassHandle this_oop, TRAPS) {
-  relocate_and_link(this_oop, this_oop->methods(), THREAD);
-}
-
-void Rewriter::relocate_and_link(instanceKlassHandle this_oop,
-                                 Array<Method*>* methods, TRAPS) {
-  int len = methods->length();
+  // Relocate after everything, but still do this under the is_rewritten flag,
+  // so methods with jsrs in custom class lists in aren't attempted to be
+  // rewritten in the RO section of the shared archive.
+  // Relocated bytecodes don't have to be restored, only the cp cache entries
   for (int i = len-1; i >= 0; i--) {
-    methodHandle m(THREAD, methods->at(i));
+    methodHandle m(THREAD, _methods->at(i));
 
     if (m->has_jsrs()) {
-      m = rewrite_jsrs(m, CHECK);
+      m = rewrite_jsrs(m, THREAD);
+      // Restore bytecodes to their unrewritten state if there are exceptions
+      // relocating bytecodes.  If some are relocated, that is ok because that
+      // doesn't affect constant pool to cpCache rewriting.
+      if (HAS_PENDING_EXCEPTION) {
+        restore_bytecodes();
+        return;
+      }
       // Method might have gotten rewritten.
       methods->at_put(i, m());
     }
-
-    // Set up method entry points for compiler and interpreter    .
-    m->link_method(m, CHECK);
-
-    // This is for JVMTI and unrelated to relocator but the last thing we do
-#ifdef ASSERT
-    if (StressMethodComparator) {
-      static int nmc = 0;
-      for (int j = i; j >= 0 && j >= i-4; j--) {
-        if ((++nmc % 1000) == 0)  tty->print_cr("Have run MethodComparator %d times...", nmc);
-        bool z = MethodComparator::methods_EMCP(m(),
-                   methods->at(j));
-        if (j == i && !z) {
-          tty->print("MethodComparator FAIL: "); m->print(); m->print_codes();
-          assert(z, "method must compare equal to itself");
-        }
-      }
-    }
-#endif //ASSERT
   }
 }

hotspot/src/share/vm/interpreter/rewriter.hpp

@@ -158,14 +158,6 @@ class Rewriter: public StackObj {
  public:
   // Driver routine:
   static void rewrite(instanceKlassHandle klass, TRAPS);
-  static void rewrite(instanceKlassHandle klass, constantPoolHandle cpool, Array<Method*>* methods, TRAPS);
-
-  // Second pass, not gated by is_rewritten flag
-  static void relocate_and_link(instanceKlassHandle klass, TRAPS);
-  // JSR292 version to call with it's own methods.
-  static void relocate_and_link(instanceKlassHandle klass,
-                                Array<Method*>* methods, TRAPS);
-
 };
 
 #endif // SHARE_VM_INTERPRETER_REWRITER_HPP

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

@@ -47,6 +47,7 @@
 #include "oops/symbol.hpp"
 #include "prims/jvmtiExport.hpp"
 #include "prims/jvmtiRedefineClassesTrace.hpp"
+#include "prims/methodComparator.hpp"
 #include "runtime/fieldDescriptor.hpp"
 #include "runtime/handles.inline.hpp"
 #include "runtime/javaCalls.hpp"
@@ -602,7 +603,7 @@ bool InstanceKlass::link_class_impl(
       }
 
       // relocate jsrs and link methods after they are all rewritten
-      this_oop->relocate_and_link_methods(CHECK_false);
+      this_oop->link_methods(CHECK_false);
 
       // Initialize the vtable and interface table after
       // methods have been rewritten since rewrite may
@@ -650,10 +651,31 @@ void InstanceKlass::rewrite_class(TRAPS) {
 // Now relocate and link method entry points after class is rewritten.
 // This is outside is_rewritten flag. In case of an exception, it can be
 // executed more than once.
-void InstanceKlass::relocate_and_link_methods(TRAPS) {
-  assert(is_loaded(), "must be loaded");
-  instanceKlassHandle this_oop(THREAD, this);
-  Rewriter::relocate_and_link(this_oop, CHECK);
+void InstanceKlass::link_methods(TRAPS) {
+  int len = methods()->length();
+  for (int i = len-1; i >= 0; i--) {
+    methodHandle m(THREAD, methods()->at(i));
+
+    // Set up method entry points for compiler and interpreter    .
+    m->link_method(m, CHECK);
+
+    // This is for JVMTI and unrelated to relocator but the last thing we do
+#ifdef ASSERT
+    if (StressMethodComparator) {
+      ResourceMark rm(THREAD);
+      static int nmc = 0;
+      for (int j = i; j >= 0 && j >= i-4; j--) {
+        if ((++nmc % 1000) == 0)  tty->print_cr("Have run MethodComparator %d times...", nmc);
+        bool z = MethodComparator::methods_EMCP(m(),
+                   methods()->at(j));
+        if (j == i && !z) {
+          tty->print("MethodComparator FAIL: "); m->print(); m->print_codes();
+          assert(z, "method must compare equal to itself");
+        }
+      }
+    }
+#endif //ASSERT
+  }
 }
 
 

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

@@ -454,7 +454,7 @@ class InstanceKlass: public Klass {
   bool link_class_or_fail(TRAPS); // returns false on failure
   void unlink_class();
   void rewrite_class(TRAPS);
-  void relocate_and_link_methods(TRAPS);
+  void link_methods(TRAPS);
   Method* class_initializer();
 
   // set the class to initialized if no static initializer is present

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

@@ -1043,7 +1043,7 @@ jvmtiError VM_RedefineClasses::load_new_class_versions(TRAPS) {
 
     Rewriter::rewrite(scratch_class, THREAD);
     if (!HAS_PENDING_EXCEPTION) {
-      Rewriter::relocate_and_link(scratch_class, THREAD);
+      scratch_class->link_methods(THREAD);
     }
     if (HAS_PENDING_EXCEPTION) {
       Symbol* ex_name = PENDING_EXCEPTION->klass()->name();

hotspot/src/share/vm/runtime/handles.inline.hpp

@@ -80,6 +80,8 @@ inline name##Handle::name##Handle(const name##Handle &h) {             \
       _thread = Thread::current();                                     \
     }                                                                  \
     _thread->metadata_handles()->push((Metadata*)_value);              \
+  } else {                                                             \
+    _thread = NULL;                                                    \
   }                                                                    \
 }                                                                      \
 inline name##Handle& name##Handle::operator=(const name##Handle &s) {  \
@@ -94,6 +96,8 @@ inline name##Handle& name##Handle::operator=(const name##Handle &s) {  \
       _thread = Thread::current();                                     \
     }                                                                  \
     _thread->metadata_handles()->push((Metadata*)_value);              \
+  } else {                                                             \
+    _thread = NULL;                                                    \
   }                                                                    \
   return *this;                                                        \
 }                                                                      \