changeset 57441:26bb0fe2270a

8235829: graal crashes with Zombie.java test Summary: Start ServiceThread before compiler threads, and run nmethod barriers for zgc before adding to the service thread queues, or posting events from the java thread. Reviewed-by: pliden, dholmes, rehn
author coleenp
date Wed, 18 Dec 2019 11:51:22 -0500
parents 1c844398e041
children 2069b4bfd23b
files src/hotspot/share/code/compiledMethod.cpp src/hotspot/share/code/compiledMethod.hpp src/hotspot/share/code/nmethod.cpp src/hotspot/share/prims/jvmtiCodeBlobEvents.cpp src/hotspot/share/prims/jvmtiImpl.cpp src/hotspot/share/prims/jvmtiImpl.hpp src/hotspot/share/prims/jvmtiThreadState.cpp src/hotspot/share/prims/jvmtiThreadState.hpp src/hotspot/share/runtime/serviceThread.cpp src/hotspot/share/runtime/thread.cpp src/hotspot/share/services/management.cpp test/hotspot/jtreg/serviceability/jvmti/CompiledMethodLoad/Zombie.java
diffstat 12 files changed, 91 insertions(+), 32 deletions(-) [+]
line wrap: on
line diff
--- a/src/hotspot/share/code/compiledMethod.cpp	Wed Dec 18 14:36:49 2019 +0300
+++ b/src/hotspot/share/code/compiledMethod.cpp	Wed Dec 18 11:51:22 2019 -0500
@@ -549,6 +549,21 @@
   return true;
 }
 
+void CompiledMethod::run_nmethod_entry_barrier() {
+  BarrierSetNMethod* bs_nm = BarrierSet::barrier_set()->barrier_set_nmethod();
+  if (bs_nm != NULL) {
+    // We want to keep an invariant that nmethods found through iterations of a Thread's
+    // nmethods found in safepoints have gone through an entry barrier and are not armed.
+    // By calling this nmethod entry barrier, it plays along and acts
+    // like any other nmethod found on the stack of a thread (fewer surprises).
+    nmethod* nm = as_nmethod_or_null();
+    if (nm != NULL) {
+      bool alive = bs_nm->nmethod_entry_barrier(nm);
+      assert(alive, "should be alive");
+    }
+  }
+}
+
 void CompiledMethod::cleanup_inline_caches(bool clean_all) {
   for (;;) {
     ICRefillVerifier ic_refill_verifier;
@@ -557,18 +572,8 @@
         return;
       }
     }
-    BarrierSetNMethod* bs_nm = BarrierSet::barrier_set()->barrier_set_nmethod();
-    if (bs_nm != NULL) {
-      // We want to keep an invariant that nmethods found through iterations of a Thread's
-      // nmethods found in safepoints have gone through an entry barrier and are not armed.
-      // By calling this nmethod entry barrier from the sweeper, it plays along and acts
-      // like any other nmethod found on the stack of a thread (fewer surprises).
-      nmethod* nm = as_nmethod_or_null();
-      if (nm != NULL) {
-        bool alive = bs_nm->nmethod_entry_barrier(nm);
-        assert(alive, "should be alive");
-      }
-    }
+    // Call this nmethod entry barrier from the sweeper.
+    run_nmethod_entry_barrier();
     InlineCacheBuffer::refill_ic_stubs();
   }
 }
--- a/src/hotspot/share/code/compiledMethod.hpp	Wed Dec 18 14:36:49 2019 +0300
+++ b/src/hotspot/share/code/compiledMethod.hpp	Wed Dec 18 11:51:22 2019 -0500
@@ -366,6 +366,9 @@
   virtual void clear_inline_caches();
   void clear_ic_callsites();
 
+  // Execute nmethod barrier code, as if entering through nmethod call.
+  void run_nmethod_entry_barrier();
+
   // Verify and count cached icholder relocations.
   int  verify_icholder_relocations();
   void verify_oop_relocations();
--- a/src/hotspot/share/code/nmethod.cpp	Wed Dec 18 14:36:49 2019 +0300
+++ b/src/hotspot/share/code/nmethod.cpp	Wed Dec 18 11:51:22 2019 -0500
@@ -1567,6 +1567,12 @@
 // Transfer information from compilation to jvmti
 void nmethod::post_compiled_method_load_event(JvmtiThreadState* state) {
 
+  // Don't post this nmethod load event if it is already dying
+  // because the sweeper might already be deleting this nmethod.
+  if (is_not_entrant() && can_convert_to_zombie()) {
+    return;
+  }
+
   // This is a bad time for a safepoint.  We don't want
   // this nmethod to get unloaded while we're queueing the event.
   NoSafepointVerifier nsv;
@@ -1585,15 +1591,16 @@
   if (JvmtiExport::should_post_compiled_method_load()) {
     // Only post unload events if load events are found.
     set_load_reported();
-    // Keep sweeper from turning this into zombie until it is posted.
-    mark_as_seen_on_stack();
-
     // If a JavaThread hasn't been passed in, let the Service thread
     // (which is a real Java thread) post the event
     JvmtiDeferredEvent event = JvmtiDeferredEvent::compiled_method_load_event(this);
     if (state == NULL) {
+      // Execute any barrier code for this nmethod as if it's called, since
+      // keeping it alive looks like stack walking.
+      run_nmethod_entry_barrier();
       ServiceThread::enqueue_deferred_event(&event);
     } else {
+      // This enters the nmethod barrier outside in the caller.
       state->enqueue_event(&event);
     }
   }
--- a/src/hotspot/share/prims/jvmtiCodeBlobEvents.cpp	Wed Dec 18 14:36:49 2019 +0300
+++ b/src/hotspot/share/prims/jvmtiCodeBlobEvents.cpp	Wed Dec 18 11:51:22 2019 -0500
@@ -34,6 +34,7 @@
 #include "prims/jvmtiExport.hpp"
 #include "prims/jvmtiThreadState.inline.hpp"
 #include "runtime/handles.inline.hpp"
+#include "runtime/safepointVerifiers.hpp"
 #include "runtime/vmThread.hpp"
 
 // Support class to collect a list of the non-nmethod CodeBlobs in
@@ -222,16 +223,22 @@
 jvmtiError JvmtiCodeBlobEvents::generate_compiled_method_load_events(JvmtiEnv* env) {
   JvmtiThreadState* state = JvmtiThreadState::state_for(JavaThread::current());
   {
-    // Walk the CodeCache notifying for live nmethods, don't release the CodeCache_lock
-    // because the sweeper may be running concurrently.
-    // Save events to the queue for posting outside the CodeCache_lock.
-    MutexLocker mu(CodeCache_lock, Mutex::_no_safepoint_check_flag);
-    // Iterate over non-profiled and profiled nmethods
-    NMethodIterator iter(NMethodIterator::only_alive_and_not_unloading);
-    while(iter.next()) {
-      nmethod* current = iter.method();
-      current->post_compiled_method_load_event(state);
+    NoSafepointVerifier nsv;  // safepoints are not safe while collecting methods to post.
+    {
+      // Walk the CodeCache notifying for live nmethods, don't release the CodeCache_lock
+      // because the sweeper may be running concurrently.
+      // Save events to the queue for posting outside the CodeCache_lock.
+      MutexLocker mu(CodeCache_lock, Mutex::_no_safepoint_check_flag);
+      // Iterate over non-profiled and profiled nmethods
+      NMethodIterator iter(NMethodIterator::only_alive_and_not_unloading);
+      while(iter.next()) {
+        nmethod* current = iter.method();
+        current->post_compiled_method_load_event(state);
+      }
     }
+
+    // Enter nmethod barrier code if present outside CodeCache_lock
+    state->run_nmethod_entry_barriers();
   }
 
   // Now post all the events outside the CodeCache_lock.
--- a/src/hotspot/share/prims/jvmtiImpl.cpp	Wed Dec 18 14:36:49 2019 +0300
+++ b/src/hotspot/share/prims/jvmtiImpl.cpp	Wed Dec 18 11:51:22 2019 -0500
@@ -25,6 +25,7 @@
 #include "precompiled.hpp"
 #include "classfile/symbolTable.hpp"
 #include "classfile/systemDictionary.hpp"
+#include "code/nmethod.hpp"
 #include "interpreter/interpreter.hpp"
 #include "interpreter/oopMapCache.hpp"
 #include "jvmtifiles/jvmtiEnv.hpp"
@@ -992,6 +993,12 @@
   JvmtiExport::post_compiled_method_load(env, nm);
 }
 
+void JvmtiDeferredEvent::run_nmethod_entry_barriers() {
+  if (_type == TYPE_COMPILED_METHOD_LOAD) {
+    _event_data.compiled_method_load->run_nmethod_entry_barrier();
+  }
+}
+
 
 // Keep the nmethod for compiled_method_load from being unloaded.
 void JvmtiDeferredEvent::oops_do(OopClosure* f, CodeBlobClosure* cf) {
@@ -1008,8 +1015,16 @@
   }
 }
 
+
 bool JvmtiDeferredEventQueue::has_events() {
-  return _queue_head != NULL;
+  // We save the queued events before the live phase and post them when it starts.
+  // This code could skip saving the events on the queue before the live
+  // phase and ignore them, but this would change how we do things now.
+  // Starting the service thread earlier causes this to be called before the live phase begins.
+  // The events on the queue should all be posted after the live phase so this is an
+  // ok check.  Before the live phase, DynamicCodeGenerated events are posted directly.
+  // If we add other types of events to the deferred queue, this could get ugly.
+  return JvmtiEnvBase::get_phase() == JVMTI_PHASE_LIVE  && _queue_head != NULL;
 }
 
 void JvmtiDeferredEventQueue::enqueue(JvmtiDeferredEvent event) {
@@ -1057,6 +1072,13 @@
   }
 }
 
+void JvmtiDeferredEventQueue::run_nmethod_entry_barriers() {
+  for(QueueNode* node = _queue_head; node != NULL; node = node->next()) {
+     node->event().run_nmethod_entry_barriers();
+  }
+}
+
+
 void JvmtiDeferredEventQueue::oops_do(OopClosure* f, CodeBlobClosure* cf) {
   for(QueueNode* node = _queue_head; node != NULL; node = node->next()) {
      node->event().oops_do(f, cf);
--- a/src/hotspot/share/prims/jvmtiImpl.hpp	Wed Dec 18 14:36:49 2019 +0300
+++ b/src/hotspot/share/prims/jvmtiImpl.hpp	Wed Dec 18 11:51:22 2019 -0500
@@ -481,6 +481,7 @@
   // Actually posts the event.
   void post() NOT_JVMTI_RETURN;
   void post_compiled_method_load_event(JvmtiEnv* env) NOT_JVMTI_RETURN;
+  void run_nmethod_entry_barriers() NOT_JVMTI_RETURN;
   // Sweeper support to keep nmethods from being zombied while in the queue.
   void nmethods_do(CodeBlobClosure* cf) NOT_JVMTI_RETURN;
   // GC support to keep nmethod from being unloaded while in the queue.
@@ -522,6 +523,7 @@
   // Post all events in the queue for the current Jvmti environment
   void post(JvmtiEnv* env) NOT_JVMTI_RETURN;
   void enqueue(JvmtiDeferredEvent event) NOT_JVMTI_RETURN;
+  void run_nmethod_entry_barriers();
 
   // Sweeper support to keep nmethods from being zombied while in the queue.
   void nmethods_do(CodeBlobClosure* cf) NOT_JVMTI_RETURN;
--- a/src/hotspot/share/prims/jvmtiThreadState.cpp	Wed Dec 18 14:36:49 2019 +0300
+++ b/src/hotspot/share/prims/jvmtiThreadState.cpp	Wed Dec 18 11:51:22 2019 -0500
@@ -432,3 +432,8 @@
   }
 }
 
+void JvmtiThreadState::run_nmethod_entry_barriers() {
+  if (_jvmti_event_queue != NULL) {
+    _jvmti_event_queue->run_nmethod_entry_barriers();
+  }
+}
--- a/src/hotspot/share/prims/jvmtiThreadState.hpp	Wed Dec 18 14:36:49 2019 +0300
+++ b/src/hotspot/share/prims/jvmtiThreadState.hpp	Wed Dec 18 11:51:22 2019 -0500
@@ -398,6 +398,7 @@
   // Thread local event queue, which doesn't require taking the Service_lock.
   void enqueue_event(JvmtiDeferredEvent* event);
   void post_events(JvmtiEnv* env);
+  void run_nmethod_entry_barriers();
 };
 
 class RedefineVerifyMark : public StackObj {
--- a/src/hotspot/share/runtime/serviceThread.cpp	Wed Dec 18 14:36:49 2019 +0300
+++ b/src/hotspot/share/runtime/serviceThread.cpp	Wed Dec 18 11:51:22 2019 -0500
@@ -48,7 +48,8 @@
 ServiceThread* ServiceThread::_instance = NULL;
 JvmtiDeferredEvent* ServiceThread::_jvmti_event = NULL;
 // The service thread has it's own static deferred event queue.
-// Events can be posted before the service thread is created.
+// Events can be posted before JVMTI vm_start, so it's too early to call JvmtiThreadState::state_for
+// to add this field to the per-JavaThread event queue.  TODO: fix this sometime later
 JvmtiDeferredEventQueue ServiceThread::_jvmti_service_queue;
 
 void ServiceThread::initialize() {
@@ -195,6 +196,10 @@
 
 void ServiceThread::enqueue_deferred_event(JvmtiDeferredEvent* event) {
   MutexLocker ml(Service_lock, Mutex::_no_safepoint_check_flag);
+  // If you enqueue events before the service thread runs, gc and the sweeper
+  // cannot keep the nmethod alive.  This could be restricted to compiled method
+  // load and unload events, if we wanted to be picky.
+  assert(_instance != NULL, "cannot enqueue events before the service thread runs");
   _jvmti_service_queue.enqueue(*event);
   Service_lock->notify_all();
  }
--- a/src/hotspot/share/runtime/thread.cpp	Wed Dec 18 14:36:49 2019 +0300
+++ b/src/hotspot/share/runtime/thread.cpp	Wed Dec 18 11:51:22 2019 -0500
@@ -88,6 +88,7 @@
 #include "runtime/safepoint.hpp"
 #include "runtime/safepointMechanism.inline.hpp"
 #include "runtime/safepointVerifiers.hpp"
+#include "runtime/serviceThread.hpp"
 #include "runtime/sharedRuntime.hpp"
 #include "runtime/statSampler.hpp"
 #include "runtime/stubRoutines.hpp"
@@ -3995,6 +3996,10 @@
     Chunk::start_chunk_pool_cleaner_task();
   }
 
+  // Start the service thread
+  // The service thread enqueues JVMTI deferred events and does various hashtable
+  // and other cleanups.  Needs to start before the compilers start posting events.
+  ServiceThread::initialize();
 
   // initialize compiler(s)
 #if defined(COMPILER1) || COMPILER2_OR_JVMCI
--- a/src/hotspot/share/services/management.cpp	Wed Dec 18 14:36:49 2019 +0300
+++ b/src/hotspot/share/services/management.cpp	Wed Dec 18 11:51:22 2019 -0500
@@ -46,7 +46,6 @@
 #include "runtime/jniHandles.inline.hpp"
 #include "runtime/notificationThread.hpp"
 #include "runtime/os.hpp"
-#include "runtime/serviceThread.hpp"
 #include "runtime/thread.inline.hpp"
 #include "runtime/threadSMR.hpp"
 #include "services/classLoadingService.hpp"
@@ -147,8 +146,6 @@
 }
 
 void Management::initialize(TRAPS) {
-  // Start the service thread
-  ServiceThread::initialize();
   if (UseNotificationThread) {
     NotificationThread::initialize();
   }
--- a/test/hotspot/jtreg/serviceability/jvmti/CompiledMethodLoad/Zombie.java	Wed Dec 18 14:36:49 2019 +0300
+++ b/test/hotspot/jtreg/serviceability/jvmti/CompiledMethodLoad/Zombie.java	Wed Dec 18 11:51:22 2019 -0500
@@ -26,9 +26,9 @@
  * @bug 8212159
  * @summary Generate compiled method load events without crashing
  * @run main/othervm/native -agentlib:CompiledZombie -Xcomp -XX:ReservedCodeCacheSize=20m Zombie
- *
- * The stress test that made this fail was -jar SwingSet2.jar from demos (without DISPLAY set so it exits)
- */
+ **/
+
+ // The stress test that made this fail was -jar SwingSet2.jar from demos (without DISPLAY set so it exits)
 
 public class Zombie {
     public static void main(java.lang.String[] unused) {