8014910: deadlock between JVM/TI ClassPrepare event handler and CompilerThread

Revert changes in JDK-8008962

Reviewed-by: coleenp, sspitsyn

hotspot/src/share/vm/ci/ciEnv.cpp

@@ -483,8 +483,7 @@ ciKlass* ciEnv::get_klass_by_index_impl(constantPoolHandle cpool,
     {
       // We have to lock the cpool to keep the oop from being resolved
       // while we are accessing it.
-      oop cplock = cpool->lock();
+        MonitorLockerEx ml(cpool->lock());
-      ObjectLocker ol(cplock, THREAD, cplock != NULL);
       constantTag tag = cpool->tag_at(index);
       if (tag.is_klass()) {
         // The klass has been inserted into the constant pool

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

@@ -40,7 +40,6 @@
 #include "runtime/init.hpp"
 #include "runtime/javaCalls.hpp"
 #include "runtime/signature.hpp"
-#include "runtime/synchronizer.hpp"
 #include "runtime/vframe.hpp"
 
 ConstantPool* ConstantPool::allocate(ClassLoaderData* loader_data, int length, TRAPS) {
@@ -70,6 +69,7 @@ ConstantPool::ConstantPool(Array<u1>* tags) {
 
   // only set to non-zero if constant pool is merged by RedefineClasses
   set_version(0);
+  set_lock(new Monitor(Monitor::nonleaf + 2, "A constant pool lock"));
 
   // initialize tag array
   int length = tags->length();
@@ -95,6 +95,9 @@ void ConstantPool::deallocate_contents(ClassLoaderData* loader_data) {
 void ConstantPool::release_C_heap_structures() {
   // walk constant pool and decrement symbol reference counts
   unreference_symbols();
+
+  delete _lock;
+  set_lock(NULL);
 }
 
 objArrayOop ConstantPool::resolved_references() const {
@@ -151,6 +154,9 @@ void ConstantPool::restore_unshareable_info(TRAPS) {
       ClassLoaderData* loader_data = pool_holder()->class_loader_data();
       set_resolved_references(loader_data->add_handle(refs_handle));
     }
+
+    // Also need to recreate the mutex.  Make sure this matches the constructor
+    set_lock(new Monitor(Monitor::nonleaf + 2, "A constant pool lock"));
   }
 }
 
@@ -161,23 +167,7 @@ void ConstantPool::remove_unshareable_info() {
   set_resolved_reference_length(
     resolved_references() != NULL ? resolved_references()->length() : 0);
   set_resolved_references(NULL);
-}
+  set_lock(NULL);
-
-oop ConstantPool::lock() {
-  if (_pool_holder) {
-    // We re-use the _pool_holder's init_lock to reduce footprint.
-    // Notes on deadlocks:
-    // [1] This lock is a Java oop, so it can be recursively locked by
-    //     the same thread without self-deadlocks.
-    // [2] Deadlock will happen if there is circular dependency between
-    //     the <clinit> of two Java classes. However, in this case,
-    //     the deadlock would have happened long before we reach
-    //     ConstantPool::lock(), so reusing init_lock does not
-    //     increase the possibility of deadlock.
-    return _pool_holder->init_lock();
-  } else {
-    return NULL;
-  }
 }
 
 int ConstantPool::cp_to_object_index(int cp_index) {
@@ -211,9 +201,7 @@ Klass* ConstantPool::klass_at_impl(constantPoolHandle this_oop, int which, TRAPS
 
   Symbol* name = NULL;
   Handle       loader;
-  {
+  {  MonitorLockerEx ml(this_oop->lock());
-    oop cplock = this_oop->lock();
-    ObjectLocker ol(cplock , THREAD, cplock != NULL);
 
     if (this_oop->tag_at(which).is_unresolved_klass()) {
       if (this_oop->tag_at(which).is_unresolved_klass_in_error()) {
@@ -260,8 +248,7 @@ Klass* ConstantPool::klass_at_impl(constantPoolHandle this_oop, int which, TRAPS
 
       bool throw_orig_error = false;
       {
-        oop cplock = this_oop->lock();
+        MonitorLockerEx ml(this_oop->lock());
-        ObjectLocker ol(cplock, THREAD, cplock != NULL);
 
         // some other thread has beaten us and has resolved the class.
         if (this_oop->tag_at(which).is_klass()) {
@@ -329,8 +316,7 @@ Klass* ConstantPool::klass_at_impl(constantPoolHandle this_oop, int which, TRAPS
       }
       return k();
     } else {
-      oop cplock = this_oop->lock();
+      MonitorLockerEx ml(this_oop->lock());
-      ObjectLocker ol(cplock, THREAD, cplock != NULL);
       // Only updated constant pool - if it is resolved.
       do_resolve = this_oop->tag_at(which).is_unresolved_klass();
       if (do_resolve) {
@@ -600,8 +586,7 @@ void ConstantPool::save_and_throw_exception(constantPoolHandle this_oop, int whi
                                      int tag, TRAPS) {
   ResourceMark rm;
   Symbol* error = PENDING_EXCEPTION->klass()->name();
-  oop cplock = this_oop->lock();
+  MonitorLockerEx ml(this_oop->lock());  // lock cpool to change tag.
-  ObjectLocker ol(cplock, THREAD, cplock != NULL);  // lock cpool to change tag.
 
   int error_tag = (tag == JVM_CONSTANT_MethodHandle) ?
            JVM_CONSTANT_MethodHandleInError : JVM_CONSTANT_MethodTypeInError;
@@ -762,8 +747,7 @@ oop ConstantPool::resolve_constant_at_impl(constantPoolHandle this_oop, int inde
   if (cache_index >= 0) {
     // Cache the oop here also.
     Handle result_handle(THREAD, result_oop);
-    oop cplock = this_oop->lock();
+    MonitorLockerEx ml(this_oop->lock());  // don't know if we really need this
-    ObjectLocker ol(cplock, THREAD, cplock != NULL);  // don't know if we really need this
     oop result = this_oop->resolved_references()->obj_at(cache_index);
     // Benign race condition:  resolved_references may already be filled in while we were trying to lock.
     // The important thing here is that all threads pick up the same result.

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

@@ -111,6 +111,7 @@ class ConstantPool : public Metadata {
     int                _version;
   } _saved;
 
+  Monitor*             _lock;
 
   void set_tags(Array<u1>* tags)               { _tags = tags; }
   void tag_at_put(int which, jbyte t)          { tags()->at_put(which, t); }
@@ -843,17 +844,8 @@ class ConstantPool : public Metadata {
 
   void set_resolved_reference_length(int length) { _saved._resolved_reference_length = length; }
   int  resolved_reference_length() const  { return _saved._resolved_reference_length; }
-
+  void set_lock(Monitor* lock)            { _lock = lock; }
-  // lock() may return null -- constant pool updates may happen before this lock is
+  Monitor* lock()                         { return _lock; }
-  // initialized, because the _pool_holder has not been fully initialized and
-  // has not been registered into the system dictionary. In this case, no other
-  // thread can be modifying this constantpool, so no synchronization is
-  // necessary.
-  //
-  // Use cplock() like this:
-  //    oop cplock = cp->lock();
-  //    ObjectLocker ol(cplock , THREAD, cplock != NULL);
-  oop lock();
 
   // Decrease ref counts of symbols that are in the constant pool
   // when the holder class is unloaded

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

@@ -284,8 +284,7 @@ void ConstantPoolCacheEntry::set_method_handle_common(constantPoolHandle cpool,
   // the lock, so that when the losing writer returns, he can use the linked
   // cache entry.
 
-  oop cplock = cpool->lock();
+  MonitorLockerEx ml(cpool->lock());
-  ObjectLocker ol(cplock, Thread::current(), cplock != NULL);
   if (!is_f1_null()) {
     return;
   }

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

@@ -498,13 +498,27 @@ objArrayOop InstanceKlass::signers() const {
 
 oop InstanceKlass::init_lock() const {
   // return the init lock from the mirror
-  return java_lang_Class::init_lock(java_mirror());
+  oop lock = java_lang_Class::init_lock(java_mirror());
+  assert((oop)lock != NULL || !is_not_initialized(), // initialized or in_error state
+         "only fully initialized state can have a null lock");
+  return lock;
+}
+
+// Set the initialization lock to null so the object can be GC'ed.  Any racing
+// threads to get this lock will see a null lock and will not lock.
+// That's okay because they all check for initialized state after getting
+// the lock and return.
+void InstanceKlass::fence_and_clear_init_lock() {
+  // make sure previous stores are all done, notably the init_state.
+  OrderAccess::storestore();
+  java_lang_Class::set_init_lock(java_mirror(), NULL);
+  assert(!is_not_initialized(), "class must be initialized now");
 }
 
 void InstanceKlass::eager_initialize_impl(instanceKlassHandle this_oop) {
   EXCEPTION_MARK;
   oop init_lock = this_oop->init_lock();
-  ObjectLocker ol(init_lock, THREAD);
+  ObjectLocker ol(init_lock, THREAD, init_lock != NULL);
 
   // abort if someone beat us to the initialization
   if (!this_oop->is_not_initialized()) return;  // note: not equivalent to is_initialized()
@@ -523,6 +537,7 @@ void InstanceKlass::eager_initialize_impl(instanceKlassHandle this_oop) {
   } else {
     // linking successfull, mark class as initialized
     this_oop->set_init_state (fully_initialized);
+    this_oop->fence_and_clear_init_lock();
     // trace
     if (TraceClassInitialization) {
       ResourceMark rm(THREAD);
@@ -649,7 +664,7 @@ bool InstanceKlass::link_class_impl(
   // verification & rewriting
   {
     oop init_lock = this_oop->init_lock();
-    ObjectLocker ol(init_lock, THREAD);
+    ObjectLocker ol(init_lock, THREAD, init_lock != NULL);
     // rewritten will have been set if loader constraint error found
     // on an earlier link attempt
     // don't verify or rewrite if already rewritten
@@ -772,7 +787,7 @@ void InstanceKlass::initialize_impl(instanceKlassHandle this_oop, TRAPS) {
   // Step 1
   {
     oop init_lock = this_oop->init_lock();
-    ObjectLocker ol(init_lock, THREAD);
+    ObjectLocker ol(init_lock, THREAD, init_lock != NULL);
 
     Thread *self = THREAD; // it's passed the current thread
 
@@ -920,8 +935,9 @@ void InstanceKlass::set_initialization_state_and_notify(ClassState state, TRAPS)
 
 void InstanceKlass::set_initialization_state_and_notify_impl(instanceKlassHandle this_oop, ClassState state, TRAPS) {
   oop init_lock = this_oop->init_lock();
-  ObjectLocker ol(init_lock, THREAD);
+  ObjectLocker ol(init_lock, THREAD, init_lock != NULL);
   this_oop->set_init_state(state);
+  this_oop->fence_and_clear_init_lock();
   ol.notify_all(CHECK);
 }
 

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

@@ -1023,6 +1023,7 @@ public:
   // It has to be an object not a Mutex because it's held through java calls.
   oop init_lock() const;
 private:
+  void fence_and_clear_init_lock();
 
   // Static methods that are used to implement member methods where an exposed this pointer
   // is needed due to possible GCs

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

@@ -259,8 +259,7 @@ JvmtiEnv::RetransformClasses(jint class_count, const jclass* classes) {
       // bytes to the InstanceKlass here because they have not been
       // validated and we're not at a safepoint.
       constantPoolHandle  constants(current_thread, ikh->constants());
-      oop cplock = constants->lock();
+      MonitorLockerEx ml(constants->lock());    // lock constant pool while we query it
-      ObjectLocker ol(cplock, current_thread, cplock != NULL);    // lock constant pool while we query it
 
       JvmtiClassFileReconstituter reconstituter(ikh);
       if (reconstituter.get_error() != JVMTI_ERROR_NONE) {
@@ -2418,8 +2417,7 @@ JvmtiEnv::GetConstantPool(oop k_mirror, jint* constant_pool_count_ptr, jint* con
 
   instanceKlassHandle ikh(thread, k_oop);
   constantPoolHandle  constants(thread, ikh->constants());
-  oop cplock = constants->lock();
+  MonitorLockerEx ml(constants->lock());    // lock constant pool while we query it
-  ObjectLocker ol(cplock, thread, cplock != NULL);    // lock constant pool while we query it
 
   JvmtiConstantPoolReconstituter reconstituter(ikh);
   if (reconstituter.get_error() != JVMTI_ERROR_NONE) {