8143571: [JVMCI] Double unregistering of nmethod during unloading

Reviewed-by: iveresov, twisti

hotspot/src/share/vm/code/nmethod.cpp

@@ -1348,6 +1348,9 @@ void nmethod::make_unloaded(BoolObjectClosure* is_alive, oop cause) {
 
   _state = unloaded;
 
+  // Log the unloading.
+  log_state_change();
+
 #if INCLUDE_JVMCI
   // The method can only be unloaded after the pointer to the installed code
   // Java wrapper is no longer alive. Here we need to clear out this weak
@@ -1355,11 +1358,12 @@ void nmethod::make_unloaded(BoolObjectClosure* is_alive, oop cause) {
   // after the method is unregistered since the original value may be still
   // tracked by the rset.
   maybe_invalidate_installed_code();
+  // Clear these out after the nmethod has been unregistered and any
+  // updates to the InstalledCode instance have been performed.
+  _jvmci_installed_code = NULL;
+  _speculation_log = NULL;
 #endif
 
-  // Log the unloading.
-  log_state_change();
-
   // The Method* is gone at this point
   assert(_method == NULL, "Tautology");
 
@@ -1470,6 +1474,9 @@ bool nmethod::make_not_entrant_or_zombie(unsigned int state) {
     // Log the transition once
     log_state_change();
 
+    // Invalidate while holding the patching lock
+    JVMCI_ONLY(maybe_invalidate_installed_code());
+
     // Remove nmethod from method.
     // We need to check if both the _code and _from_compiled_code_entry_point
     // refer to this nmethod because there is a race in setting these two fields
@@ -1496,6 +1503,10 @@ bool nmethod::make_not_entrant_or_zombie(unsigned int state) {
       MutexLockerEx mu(CodeCache_lock, Mutex::_no_safepoint_check_flag);
       if (nmethod_needs_unregister) {
         Universe::heap()->unregister_nmethod(this);
+#ifdef JVMCI
+        _jvmci_installed_code = NULL;
+        _speculation_log = NULL;
+#endif
       }
       flush_dependencies(NULL);
     }
@@ -1519,8 +1530,6 @@ bool nmethod::make_not_entrant_or_zombie(unsigned int state) {
     assert(state == not_entrant, "other cases may need to be handled differently");
   }
 
-  JVMCI_ONLY(maybe_invalidate_installed_code());
-
   if (TraceCreateZombies) {
     ResourceMark m;
     tty->print_cr("nmethod <" INTPTR_FORMAT "> %s code made %s", p2i(this), this->method() ? this->method()->name_and_sig_as_C_string() : "null", (state == not_entrant) ? "not entrant" : "zombie");
@@ -3403,26 +3412,80 @@ void nmethod::print_statistics() {
 
 #if INCLUDE_JVMCI
 void nmethod::clear_jvmci_installed_code() {
-  // This must be done carefully to maintain nmethod remembered sets properly
-  BarrierSet* bs = Universe::heap()->barrier_set();
-  bs->write_ref_nmethod_pre(&_jvmci_installed_code, this);
-  _jvmci_installed_code = NULL;
-  bs->write_ref_nmethod_post(&_jvmci_installed_code, this);
+  // write_ref_method_pre/post can only be safely called at a
+  // safepoint or while holding the CodeCache_lock
+  assert(CodeCache_lock->is_locked() ||
+         SafepointSynchronize::is_at_safepoint(), "should be performed under a lock for consistency");
+  if (_jvmci_installed_code != NULL) {
+    // This must be done carefully to maintain nmethod remembered sets properly
+    BarrierSet* bs = Universe::heap()->barrier_set();
+    bs->write_ref_nmethod_pre(&_jvmci_installed_code, this);
+    _jvmci_installed_code = NULL;
+    bs->write_ref_nmethod_post(&_jvmci_installed_code, this);
+  }
 }
 
 void nmethod::maybe_invalidate_installed_code() {
-  if (_jvmci_installed_code != NULL) {
-     if (!is_alive()) {
-       // Break the link between nmethod and InstalledCode such that the nmethod
-       // can subsequently be flushed safely.  The link must be maintained while
-       // the method could have live activations since invalidateInstalledCode
-       // might want to invalidate all existing activations.
-       InstalledCode::set_address(_jvmci_installed_code, 0);
-       InstalledCode::set_entryPoint(_jvmci_installed_code, 0);
-       clear_jvmci_installed_code();
-     } else if (is_not_entrant()) {
-       InstalledCode::set_entryPoint(_jvmci_installed_code, 0);
-     }
+  assert(Patching_lock->is_locked() ||
+         SafepointSynchronize::is_at_safepoint(), "should be performed under a lock for consistency");
+  oop installed_code = jvmci_installed_code();
+  if (installed_code != NULL) {
+    nmethod* nm = (nmethod*)InstalledCode::address(installed_code);
+    if (nm == NULL || nm != this) {
+      // The link has been broken or the InstalledCode instance is
+      // associated with another nmethod so do nothing.
+      return;
+    }
+    if (!is_alive()) {
+      // Break the link between nmethod and InstalledCode such that the nmethod
+      // can subsequently be flushed safely.  The link must be maintained while
+      // the method could have live activations since invalidateInstalledCode
+      // might want to invalidate all existing activations.
+      InstalledCode::set_address(installed_code, 0);
+      InstalledCode::set_entryPoint(installed_code, 0);
+    } else if (is_not_entrant()) {
+      // Remove the entry point so any invocation will fail but keep
+      // the address link around that so that existing activations can
+      // be invalidated.
+      InstalledCode::set_entryPoint(installed_code, 0);
+    }
+  }
+}
+
+void nmethod::invalidate_installed_code(Handle installedCode, TRAPS) {
+  if (installedCode() == NULL) {
+    THROW(vmSymbols::java_lang_NullPointerException());
+  }
+  jlong nativeMethod = InstalledCode::address(installedCode);
+  nmethod* nm = (nmethod*)nativeMethod;
+  if (nm == NULL) {
+    // Nothing to do
+    return;
+  }
+
+  nmethodLocker nml(nm);
+#ifdef ASSERT
+  {
+    MutexLockerEx pl(Patching_lock, Mutex::_no_safepoint_check_flag);
+    // This relationship can only be checked safely under a lock
+    assert(nm == NULL || !nm->is_alive() || nm->jvmci_installed_code() == installedCode(), "sanity check");
+  }
+#endif
+
+  if (nm->is_alive()) {
+    // The nmethod state machinery maintains the link between the
+    // HotSpotInstalledCode and nmethod* so as long as the nmethod appears to be
+    // alive assume there is work to do and deoptimize the nmethod.
+    nm->mark_for_deoptimization();
+    VM_Deoptimize op;
+    VMThread::execute(&op);
+  }
+
+  MutexLockerEx pl(Patching_lock, Mutex::_no_safepoint_check_flag);
+  // Check that it's still associated with the same nmethod and break
+  // the link if it is.
+  if (InstalledCode::address(installedCode) == nativeMethod) {
+    InstalledCode::set_address(installedCode, 0);
   }
 }
 

hotspot/src/share/vm/code/nmethod.hpp

@@ -607,10 +607,20 @@ public:
 #if INCLUDE_JVMCI
   oop jvmci_installed_code() { return _jvmci_installed_code ; }
   char* jvmci_installed_code_name(char* buf, size_t buflen);
-  void clear_jvmci_installed_code();
+
+  // Update the state of any InstalledCode instance associated with
+  // this nmethod based on the current value of _state.
   void maybe_invalidate_installed_code();
+
+  // Helper function to invalidate InstalledCode instances
+  static void invalidate_installed_code(Handle installed_code, TRAPS);
+
   oop speculation_log() { return _speculation_log ; }
-  void set_speculation_log(oop speculation_log) { _speculation_log = speculation_log;  }
+
+ private:
+  void clear_jvmci_installed_code();
+
+ public:
 #endif
 
   // GC support

hotspot/src/share/vm/jvmci/jvmciCompilerToVM.cpp

@@ -84,24 +84,6 @@ oop CompilerToVM::get_jvmci_type(KlassHandle klass, TRAPS) {
   return NULL;
 }
 
-void CompilerToVM::invalidate_installed_code(Handle installedCode, TRAPS) {
-  if (installedCode() == NULL) {
-    THROW(vmSymbols::java_lang_NullPointerException());
-  }
-  jlong nativeMethod = InstalledCode::address(installedCode);
-  nmethod* nm = (nmethod*)nativeMethod;
-  assert(nm == NULL || nm->jvmci_installed_code() == installedCode(), "sanity check");
-  if (nm != NULL && nm->is_alive()) {
-    // The nmethod state machinery maintains the link between the
-    // HotSpotInstalledCode and nmethod* so as long as the nmethod appears to be
-    // alive assume there is work to do and deoptimize the nmethod.
-    nm->mark_for_deoptimization();
-    VM_Deoptimize op;
-    VMThread::execute(&op);
-  }
-  InstalledCode::set_address(installedCode, 0);
-}
-
 extern "C" {
 extern VMStructEntry* gHotSpotVMStructs;
 extern uint64_t gHotSpotVMStructEntryTypeNameOffset;
@@ -688,18 +670,22 @@ C2V_VMENTRY(jint, installCode, (JNIEnv *jniEnv, jobject, jobject target, jobject
   } else {
     if (!installed_code_handle.is_null()) {
       assert(installed_code_handle->is_a(InstalledCode::klass()), "wrong type");
-      CompilerToVM::invalidate_installed_code(installed_code_handle, CHECK_0);
-      InstalledCode::set_address(installed_code_handle, (jlong) cb);
-      InstalledCode::set_version(installed_code_handle, InstalledCode::version(installed_code_handle) + 1);
-      if (cb->is_nmethod()) {
-        InstalledCode::set_entryPoint(installed_code_handle, (jlong) cb->as_nmethod_or_null()->verified_entry_point());
-      } else {
-        InstalledCode::set_entryPoint(installed_code_handle, (jlong) cb->code_begin());
-      }
-      if (installed_code_handle->is_a(HotSpotInstalledCode::klass())) {
-        HotSpotInstalledCode::set_size(installed_code_handle, cb->size());
-        HotSpotInstalledCode::set_codeStart(installed_code_handle, (jlong) cb->code_begin());
-        HotSpotInstalledCode::set_codeSize(installed_code_handle, cb->code_size());
+      nmethod::invalidate_installed_code(installed_code_handle, CHECK_0);
+      {
+        // Ensure that all updates to the InstalledCode fields are consistent.
+        MutexLockerEx pl(Patching_lock, Mutex::_no_safepoint_check_flag);
+        InstalledCode::set_address(installed_code_handle, (jlong) cb);
+        InstalledCode::set_version(installed_code_handle, InstalledCode::version(installed_code_handle) + 1);
+        if (cb->is_nmethod()) {
+          InstalledCode::set_entryPoint(installed_code_handle, (jlong) cb->as_nmethod_or_null()->verified_entry_point());
+        } else {
+          InstalledCode::set_entryPoint(installed_code_handle, (jlong) cb->code_begin());
+        }
+        if (installed_code_handle->is_a(HotSpotInstalledCode::klass())) {
+          HotSpotInstalledCode::set_size(installed_code_handle, cb->size());
+          HotSpotInstalledCode::set_codeStart(installed_code_handle, (jlong) cb->code_begin());
+          HotSpotInstalledCode::set_codeSize(installed_code_handle, cb->code_size());
+        }
       }
       nmethod* nm = cb->as_nmethod_or_null();
       if (nm != NULL && installed_code_handle->is_scavengable()) {
@@ -971,7 +957,7 @@ C2V_END
 
 C2V_VMENTRY(void, invalidateInstalledCode, (JNIEnv*, jobject, jobject installed_code))
   Handle installed_code_handle = JNIHandles::resolve(installed_code);
-  CompilerToVM::invalidate_installed_code(installed_code_handle, CHECK);
+  nmethod::invalidate_installed_code(installed_code_handle, CHECK);
 C2V_END
 
 C2V_VMENTRY(jobject, readUncompressedOop, (JNIEnv*, jobject, jlong addr))

hotspot/src/share/vm/jvmci/jvmciCompilerToVM.hpp

@@ -97,8 +97,6 @@ public:
   static oop get_jvmci_method(const methodHandle& method, TRAPS);
 
   static oop get_jvmci_type(KlassHandle klass, TRAPS);
-
-  static void invalidate_installed_code(Handle installedCode, TRAPS);
 };
 
 class JavaArgumentUnboxer : public SignatureIterator {