8242427: JVMTI frame pop operations should use Thread-Local Handshakes

Reviewed-by: dcubed, dholmes, pchilanomate, rehn, sspitsyn

src/hotspot/share/prims/jvmtiEnv.cpp

@@ -1714,15 +1714,18 @@ JvmtiEnv::PopFrame(JavaThread* java_thread) {
     // shall be posted for this PopFrame.
 
     // It is only safe to perform the direct operation on the current
-    // thread. All other usage needs to use a vm-safepoint-op for safety.
+    // thread. All other usage needs to use a handshake for safety.
-    if (java_thread == JavaThread::current()) {
+    {
-      state->update_for_pop_top_frame();
+      MutexLocker mu(JvmtiThreadState_lock);
-    } else {
+      if (java_thread == JavaThread::current()) {
-      VM_UpdateForPopTopFrame op(state);
+        state->update_for_pop_top_frame();
-      VMThread::execute(&op);
+      } else {
-      jvmtiError err = op.result();
+        UpdateForPopTopFrameClosure op(state);
-      if (err != JVMTI_ERROR_NONE) {
+        bool executed = Handshake::execute_direct(&op, java_thread);
-        return err;
+        jvmtiError err = executed ? op.result() : JVMTI_ERROR_THREAD_NOT_ALIVE;
+        if (err != JVMTI_ERROR_NONE) {
+          return err;
+        }
       }
     }
 
@@ -1796,13 +1799,14 @@ JvmtiEnv::NotifyFramePop(JavaThread* java_thread, jint depth) {
 
   // It is only safe to perform the direct operation on the current
   // thread. All other usage needs to use a vm-safepoint-op for safety.
+  MutexLocker mu(JvmtiThreadState_lock);
   if (java_thread == JavaThread::current()) {
     int frame_number = state->count_frames() - depth;
     state->env_thread_state(this)->set_frame_pop(frame_number);
   } else {
-    VM_SetFramePop op(this, state, depth);
+    SetFramePopClosure op(this, state, depth);
-    VMThread::execute(&op);
+    bool executed = Handshake::execute_direct(&op, java_thread);
-    err = op.result();
+    err = executed ? op.result() : JVMTI_ERROR_THREAD_NOT_ALIVE;
   }
   return err;
 } /* end NotifyFramePop */

src/hotspot/share/prims/jvmtiEnvBase.cpp

@@ -1504,25 +1504,23 @@ JvmtiModuleClosure::get_all_modules(JvmtiEnv* env, jint* module_count_ptr, jobje
 }
 
 void
-VM_UpdateForPopTopFrame::doit() {
+UpdateForPopTopFrameClosure::do_thread(Thread *target) {
   JavaThread* jt = _state->get_thread();
-  ThreadsListHandle tlh;
+  assert(jt == target, "just checking");
-  if (jt != NULL && tlh.includes(jt) && !jt->is_exiting() && jt->threadObj() != NULL) {
+  if (!jt->is_exiting() && jt->threadObj() != NULL) {
     _state->update_for_pop_top_frame();
-  } else {
+    _result = JVMTI_ERROR_NONE;
-    _result = JVMTI_ERROR_THREAD_NOT_ALIVE;
   }
 }
 
 void
-VM_SetFramePop::doit() {
+SetFramePopClosure::do_thread(Thread *target) {
   JavaThread* jt = _state->get_thread();
-  ThreadsListHandle tlh;
+  assert(jt == target, "just checking");
-  if (jt != NULL && tlh.includes(jt) && !jt->is_exiting() && jt->threadObj() != NULL) {
+  if (!jt->is_exiting() && jt->threadObj() != NULL) {
     int frame_number = _state->count_frames() - _depth;
     _state->env_thread_state((JvmtiEnvBase*)_env)->set_frame_pop(frame_number);
-  } else {
+    _result = JVMTI_ERROR_NONE;
-    _result = JVMTI_ERROR_THREAD_NOT_ALIVE;
   }
 }
 

src/hotspot/share/prims/jvmtiEnvBase.hpp

@@ -336,24 +336,23 @@ class JvmtiEnvIterator : public StackObj {
   JvmtiEnv* next(JvmtiEnvBase* env) { return env->next_environment(); }
 };
 
-// VM operation to update for pop top frame.
+// HandshakeClosure to update for pop top frame.
-class VM_UpdateForPopTopFrame : public VM_Operation {
+class UpdateForPopTopFrameClosure : public HandshakeClosure {
 private:
   JvmtiThreadState* _state;
   jvmtiError _result;
 
 public:
-  VM_UpdateForPopTopFrame(JvmtiThreadState* state) {
+  UpdateForPopTopFrameClosure(JvmtiThreadState* state)
-    _state = state;
+    : HandshakeClosure("UpdateForPopTopFrame"),
-    _result = JVMTI_ERROR_NONE;
+      _state(state),
-  }
+      _result(JVMTI_ERROR_THREAD_NOT_ALIVE) {}
-  VMOp_Type type() const { return VMOp_UpdateForPopTopFrame; }
   jvmtiError result() { return _result; }
-  void doit();
+  void do_thread(Thread *target);
 };
 
-// VM operation to set frame pop.
+// HandshakeClosure to set frame pop.
-class VM_SetFramePop : public VM_Operation {
+class SetFramePopClosure : public HandshakeClosure {
 private:
   JvmtiEnv *_env;
   JvmtiThreadState* _state;
@@ -361,15 +360,14 @@ private:
   jvmtiError _result;
 
 public:
-  VM_SetFramePop(JvmtiEnv *env, JvmtiThreadState* state, jint depth) {
+  SetFramePopClosure(JvmtiEnv *env, JvmtiThreadState* state, jint depth)
-    _env = env;
+    : HandshakeClosure("SetFramePop"),
-    _state = state;
+      _env(env),
-    _depth = depth;
+      _state(state),
-    _result = JVMTI_ERROR_NONE;
+      _depth(depth),
-  }
+      _result(JVMTI_ERROR_THREAD_NOT_ALIVE) {}
-  VMOp_Type type() const { return VMOp_SetFramePop; }
   jvmtiError result() { return _result; }
-  void doit();
+  void do_thread(Thread *target);
 };
 
 

src/hotspot/share/prims/jvmtiEnvThreadState.cpp

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2003, 2017, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2003, 2020, 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
@@ -191,8 +191,11 @@ void JvmtiEnvThreadState::compare_and_set_current_location(Method* new_method,
 
 
 JvmtiFramePops* JvmtiEnvThreadState::get_frame_pops() {
-  assert(get_thread() == Thread::current() || SafepointSynchronize::is_at_safepoint(),
+#ifdef ASSERT
-         "frame pop data only accessible from same thread or at safepoint");
+  Thread *current = Thread::current();
+#endif
+  assert(get_thread() == current || current == get_thread()->active_handshaker(),
+         "frame pop data only accessible from same thread or direct handshake");
   if (_frame_pops == NULL) {
     _frame_pops = new JvmtiFramePops();
     assert(_frame_pops != NULL, "_frame_pops != NULL");
@@ -206,32 +209,44 @@ bool JvmtiEnvThreadState::has_frame_pops() {
 }
 
 void JvmtiEnvThreadState::set_frame_pop(int frame_number) {
-  assert(get_thread() == Thread::current() || SafepointSynchronize::is_at_safepoint(),
+#ifdef ASSERT
-         "frame pop data only accessible from same thread or at safepoint");
+  Thread *current = Thread::current();
+#endif
+  assert(get_thread() == current || current == get_thread()->active_handshaker(),
+         "frame pop data only accessible from same thread or direct handshake");
   JvmtiFramePop fpop(frame_number);
   JvmtiEventController::set_frame_pop(this, fpop);
 }
 
 
 void JvmtiEnvThreadState::clear_frame_pop(int frame_number) {
-  assert(get_thread() == Thread::current() || SafepointSynchronize::is_at_safepoint(),
+#ifdef ASSERT
-         "frame pop data only accessible from same thread or at safepoint");
+  Thread *current = Thread::current();
+#endif
+  assert(get_thread() == current || current == get_thread()->active_handshaker(),
+         "frame pop data only accessible from same thread or direct handshake");
   JvmtiFramePop fpop(frame_number);
   JvmtiEventController::clear_frame_pop(this, fpop);
 }
 
 
 void JvmtiEnvThreadState::clear_to_frame_pop(int frame_number)  {
-  assert(get_thread() == Thread::current() || SafepointSynchronize::is_at_safepoint(),
+#ifdef ASSERT
-         "frame pop data only accessible from same thread or at safepoint");
+  Thread *current = Thread::current();
+#endif
+  assert(get_thread() == current || current == get_thread()->active_handshaker(),
+         "frame pop data only accessible from same thread or direct handshake");
   JvmtiFramePop fpop(frame_number);
   JvmtiEventController::clear_to_frame_pop(this, fpop);
 }
 
 
 bool JvmtiEnvThreadState::is_frame_pop(int cur_frame_number) {
-  assert(get_thread() == Thread::current() || SafepointSynchronize::is_at_safepoint(),
+#ifdef ASSERT
-         "frame pop data only accessible from same thread or at safepoint");
+  Thread *current = Thread::current();
+#endif
+  assert(get_thread() == current || current == get_thread()->active_handshaker(),
+         "frame pop data only accessible from same thread or direct handshake");
   if (!get_thread()->is_interp_only_mode() || _frame_pops == NULL) {
     return false;
   }
@@ -240,25 +255,25 @@ bool JvmtiEnvThreadState::is_frame_pop(int cur_frame_number) {
 }
 
 
-class VM_GetCurrentLocation : public VM_Operation {
+class GetCurrentLocationClosure : public HandshakeClosure {
  private:
-   JavaThread *_thread;
    jmethodID _method_id;
    int _bci;
 
  public:
-  VM_GetCurrentLocation(JavaThread *thread) {
+  GetCurrentLocationClosure()
-     _thread = thread;
+    : HandshakeClosure("GetCurrentLocation"),
-   }
+      _method_id(NULL),
-  VMOp_Type type() const { return VMOp_GetCurrentLocation; }
+      _bci(0) {}
-  void doit() {
+  void do_thread(Thread *target) {
-    ResourceMark rmark; // _thread != Thread::current()
+    JavaThread *jt = (JavaThread *)target;
-    RegisterMap rm(_thread, false);
+    ResourceMark rmark; // jt != Thread::current()
+    RegisterMap rm(jt, false);
     // There can be a race condition between a VM_Operation reaching a safepoint
     // and the target thread exiting from Java execution.
     // We must recheck the last Java frame still exists.
-    if (!_thread->is_exiting() && _thread->has_last_Java_frame()) {
+    if (!jt->is_exiting() && jt->has_last_Java_frame()) {
-      javaVFrame* vf = _thread->last_java_vframe(&rm);
+      javaVFrame* vf = jt->last_java_vframe(&rm);
       assert(vf != NULL, "must have last java frame");
       Method* method = vf->method();
       _method_id = method->jmethod_id();
@@ -307,9 +322,15 @@ void JvmtiEnvThreadState::reset_current_location(jvmtiEvent event_type, bool ena
       jmethodID method_id;
       int bci;
       // The java thread stack may not be walkable for a running thread
-      // so get current location at safepoint.
+      // so get current location with direct handshake.
-      VM_GetCurrentLocation op(_thread);
+      GetCurrentLocationClosure op;
-      VMThread::execute(&op);
+      Thread *current = Thread::current();
+      if (current == _thread || _thread->active_handshaker() == current) {
+        op.do_thread(_thread);
+      } else {
+        bool executed = Handshake::execute_direct(&op, _thread);
+        guarantee(executed, "Direct handshake failed. Target thread is not alive?");
+      }
       op.get_current_location(&method_id, &bci);
       set_current_location(method_id, bci);
     }

src/hotspot/share/prims/jvmtiEventController.cpp

@@ -331,10 +331,14 @@ void JvmtiEventControllerPrivate::enter_interp_only_mode(JvmtiThreadState *state
   EC_TRACE(("[%s] # Entering interpreter only mode",
             JvmtiTrace::safe_get_thread_name(state->get_thread())));
   EnterInterpOnlyModeClosure hs;
-  if (SafepointSynchronize::is_at_safepoint()) {
+  assert(state->get_thread()->is_Java_thread(), "just checking");
-    hs.do_thread(state->get_thread());
+  JavaThread *target = (JavaThread *)state->get_thread();
+  Thread *current = Thread::current();
+  if (target == current || target->active_handshaker() == current) {
+    hs.do_thread(target);
   } else {
-    Handshake::execute_direct(&hs, state->get_thread());
+    bool executed = Handshake::execute_direct(&hs, target);
+    guarantee(executed, "Direct handshake failed. Target thread is not alive?");
   }
 }
 
@@ -980,21 +984,21 @@ JvmtiEventController::set_extension_event_callback(JvmtiEnvBase *env,
 
 void
 JvmtiEventController::set_frame_pop(JvmtiEnvThreadState *ets, JvmtiFramePop fpop) {
-  MutexLocker mu(SafepointSynchronize::is_at_safepoint() ? NULL : JvmtiThreadState_lock);
+  assert_lock_strong(JvmtiThreadState_lock);
   JvmtiEventControllerPrivate::set_frame_pop(ets, fpop);
 }
 
 
 void
 JvmtiEventController::clear_frame_pop(JvmtiEnvThreadState *ets, JvmtiFramePop fpop) {
-  MutexLocker mu(SafepointSynchronize::is_at_safepoint() ? NULL : JvmtiThreadState_lock);
+  assert_lock_strong(JvmtiThreadState_lock);
   JvmtiEventControllerPrivate::clear_frame_pop(ets, fpop);
 }
 
 
 void
 JvmtiEventController::clear_to_frame_pop(JvmtiEnvThreadState *ets, JvmtiFramePop fpop) {
-  MutexLocker mu(SafepointSynchronize::is_at_safepoint() ? NULL : JvmtiThreadState_lock);
+  assert_lock_strong(JvmtiThreadState_lock);
   JvmtiEventControllerPrivate::clear_to_frame_pop(ets, fpop);
 }
 

src/hotspot/share/prims/jvmtiExport.cpp

@@ -1645,7 +1645,10 @@ void JvmtiExport::post_method_exit(JavaThread *thread, Method* method, frame cur
           }
         }
         // remove the frame's entry
-        ets->clear_frame_pop(cur_frame_number);
+        {
+          MutexLocker mu(JvmtiThreadState_lock);
+          ets->clear_frame_pop(cur_frame_number);
+        }
       }
     }
   }

src/hotspot/share/prims/jvmtiThreadState.cpp

@@ -272,9 +272,9 @@ void JvmtiThreadState::decr_cur_stack_depth() {
 }
 
 int JvmtiThreadState::cur_stack_depth() {
-  guarantee(SafepointSynchronize::is_at_safepoint() ||
+  Thread *current = Thread::current();
-    (JavaThread *)Thread::current() == get_thread(),
+  guarantee(current == get_thread() || current == get_thread()->active_handshaker(),
-    "must be current thread or at safepoint");
+            "must be current thread or direct handshake");
 
   if (!is_interp_only_mode() || _cur_stack_depth == UNKNOWN_STACK_DEPTH) {
     _cur_stack_depth = count_frames();

src/hotspot/share/runtime/handshake.cpp

@@ -383,9 +383,9 @@ HandshakeState::HandshakeState() :
   _operation_direct(NULL),
   _handshake_turn_sem(1),
   _processing_sem(1),
-  _thread_in_process_handshake(false)
+  _thread_in_process_handshake(false),
+  _active_handshaker(NULL)
 {
-  DEBUG_ONLY(_active_handshaker = NULL;)
 }
 
 void HandshakeState::set_operation(HandshakeOperation* op) {
@@ -510,9 +510,9 @@ HandshakeState::ProcessResult HandshakeState::try_process(HandshakeOperation* op
   if (can_process_handshake()) {
     guarantee(!_processing_sem.trywait(), "we should already own the semaphore");
     log_trace(handshake)("Processing handshake by %s", Thread::current()->is_VM_thread() ? "VMThread" : "Handshaker");
-    DEBUG_ONLY(_active_handshaker = Thread::current();)
+    _active_handshaker = Thread::current();
     op->do_handshake(_handshakee);
-    DEBUG_ONLY(_active_handshaker = NULL;)
+    _active_handshaker = NULL;
     // Disarm after we have executed the operation.
     clear_handshake(is_direct);
     pr = _success;

src/hotspot/share/runtime/handshake.hpp

@@ -106,11 +106,8 @@ public:
   };
   ProcessResult try_process(HandshakeOperation* op);
 
-#ifdef ASSERT
   Thread* _active_handshaker;
   Thread* active_handshaker() const { return _active_handshaker; }
-#endif
-
 };
 
 #endif // SHARE_RUNTIME_HANDSHAKE_HPP

src/hotspot/share/runtime/thread.hpp

@@ -1365,11 +1365,9 @@ class JavaThread: public Thread {
     return _handshake.try_process(op);
   }
 
-#ifdef ASSERT
   Thread* active_handshaker() const {
     return _handshake.active_handshaker();
   }
-#endif
 
   // Suspend/resume support for JavaThread
  private:

src/hotspot/share/runtime/vmOperations.hpp

@@ -76,14 +76,11 @@
   template(PopulateDumpSharedSpace)               \
   template(JNIFunctionTableCopier)                \
   template(RedefineClasses)                       \
-  template(UpdateForPopTopFrame)                  \
-  template(SetFramePop)                           \
   template(GetObjectMonitorUsage)                 \
   template(GetAllStackTraces)                     \
   template(GetThreadListStackTraces)              \
   template(ChangeBreakpoints)                     \
   template(GetOrSetLocal)                         \
-  template(GetCurrentLocation)                    \
   template(ChangeSingleStep)                      \
   template(HeapWalkOperation)                     \
   template(HeapIterateOperation)                  \