changeset 60816:bace1a49abc5

8242427: JVMTI frame pop operations should use Thread-Local Handshakes Reviewed-by: dcubed, dholmes, pchilanomate, rehn, sspitsyn
author ysuenaga
date Sat, 05 Sep 2020 12:42:05 +0900
parents e5d81a790eae
children bdc20ee1a68d
files src/hotspot/share/prims/jvmtiEnv.cpp src/hotspot/share/prims/jvmtiEnvBase.cpp src/hotspot/share/prims/jvmtiEnvBase.hpp src/hotspot/share/prims/jvmtiEnvThreadState.cpp src/hotspot/share/prims/jvmtiEventController.cpp src/hotspot/share/prims/jvmtiExport.cpp src/hotspot/share/prims/jvmtiThreadState.cpp src/hotspot/share/runtime/handshake.cpp src/hotspot/share/runtime/handshake.hpp src/hotspot/share/runtime/thread.hpp src/hotspot/share/runtime/vmOperations.hpp
diffstat 11 files changed, 107 insertions(+), 87 deletions(-) [+]
line wrap: on
line diff
--- a/src/hotspot/share/prims/jvmtiEnv.cpp	Fri Sep 04 13:44:48 2020 -0400
+++ b/src/hotspot/share/prims/jvmtiEnv.cpp	Sat Sep 05 12:42:05 2020 +0900
@@ -1714,15 +1714,18 @@
     // 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.
-    if (java_thread == JavaThread::current()) {
-      state->update_for_pop_top_frame();
-    } else {
-      VM_UpdateForPopTopFrame op(state);
-      VMThread::execute(&op);
-      jvmtiError err = op.result();
-      if (err != JVMTI_ERROR_NONE) {
-        return err;
+    // thread. All other usage needs to use a handshake for safety.
+    {
+      MutexLocker mu(JvmtiThreadState_lock);
+      if (java_thread == JavaThread::current()) {
+        state->update_for_pop_top_frame();
+      } else {
+        UpdateForPopTopFrameClosure op(state);
+        bool executed = Handshake::execute_direct(&op, java_thread);
+        jvmtiError err = executed ? op.result() : JVMTI_ERROR_THREAD_NOT_ALIVE;
+        if (err != JVMTI_ERROR_NONE) {
+          return err;
+        }
       }
     }
 
@@ -1796,13 +1799,14 @@
 
   // 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);
-    VMThread::execute(&op);
-    err = op.result();
+    SetFramePopClosure op(this, state, depth);
+    bool executed = Handshake::execute_direct(&op, java_thread);
+    err = executed ? op.result() : JVMTI_ERROR_THREAD_NOT_ALIVE;
   }
   return err;
 } /* end NotifyFramePop */
--- a/src/hotspot/share/prims/jvmtiEnvBase.cpp	Fri Sep 04 13:44:48 2020 -0400
+++ b/src/hotspot/share/prims/jvmtiEnvBase.cpp	Sat Sep 05 12:42:05 2020 +0900
@@ -1504,25 +1504,23 @@
 }
 
 void
-VM_UpdateForPopTopFrame::doit() {
+UpdateForPopTopFrameClosure::do_thread(Thread *target) {
   JavaThread* jt = _state->get_thread();
-  ThreadsListHandle tlh;
-  if (jt != NULL && tlh.includes(jt) && !jt->is_exiting() && jt->threadObj() != NULL) {
+  assert(jt == target, "just checking");
+  if (!jt->is_exiting() && jt->threadObj() != NULL) {
     _state->update_for_pop_top_frame();
-  } else {
-    _result = JVMTI_ERROR_THREAD_NOT_ALIVE;
+    _result = JVMTI_ERROR_NONE;
   }
 }
 
 void
-VM_SetFramePop::doit() {
+SetFramePopClosure::do_thread(Thread *target) {
   JavaThread* jt = _state->get_thread();
-  ThreadsListHandle tlh;
-  if (jt != NULL && tlh.includes(jt) && !jt->is_exiting() && jt->threadObj() != NULL) {
+  assert(jt == target, "just checking");
+  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_THREAD_NOT_ALIVE;
+    _result = JVMTI_ERROR_NONE;
   }
 }
 
--- a/src/hotspot/share/prims/jvmtiEnvBase.hpp	Fri Sep 04 13:44:48 2020 -0400
+++ b/src/hotspot/share/prims/jvmtiEnvBase.hpp	Sat Sep 05 12:42:05 2020 +0900
@@ -336,24 +336,23 @@
   JvmtiEnv* next(JvmtiEnvBase* env) { return env->next_environment(); }
 };
 
-// VM operation to update for pop top frame.
-class VM_UpdateForPopTopFrame : public VM_Operation {
+// HandshakeClosure to update for pop top frame.
+class UpdateForPopTopFrameClosure : public HandshakeClosure {
 private:
   JvmtiThreadState* _state;
   jvmtiError _result;
 
 public:
-  VM_UpdateForPopTopFrame(JvmtiThreadState* state) {
-    _state = state;
-    _result = JVMTI_ERROR_NONE;
-  }
-  VMOp_Type type() const { return VMOp_UpdateForPopTopFrame; }
+  UpdateForPopTopFrameClosure(JvmtiThreadState* state)
+    : HandshakeClosure("UpdateForPopTopFrame"),
+      _state(state),
+      _result(JVMTI_ERROR_THREAD_NOT_ALIVE) {}
   jvmtiError result() { return _result; }
-  void doit();
+  void do_thread(Thread *target);
 };
 
-// VM operation to set frame pop.
-class VM_SetFramePop : public VM_Operation {
+// HandshakeClosure to set frame pop.
+class SetFramePopClosure : public HandshakeClosure {
 private:
   JvmtiEnv *_env;
   JvmtiThreadState* _state;
@@ -361,15 +360,14 @@
   jvmtiError _result;
 
 public:
-  VM_SetFramePop(JvmtiEnv *env, JvmtiThreadState* state, jint depth) {
-    _env = env;
-    _state = state;
-    _depth = depth;
-    _result = JVMTI_ERROR_NONE;
-  }
-  VMOp_Type type() const { return VMOp_SetFramePop; }
+  SetFramePopClosure(JvmtiEnv *env, JvmtiThreadState* state, jint depth)
+    : HandshakeClosure("SetFramePop"),
+      _env(env),
+      _state(state),
+      _depth(depth),
+      _result(JVMTI_ERROR_THREAD_NOT_ALIVE) {}
   jvmtiError result() { return _result; }
-  void doit();
+  void do_thread(Thread *target);
 };
 
 
--- a/src/hotspot/share/prims/jvmtiEnvThreadState.cpp	Fri Sep 04 13:44:48 2020 -0400
+++ b/src/hotspot/share/prims/jvmtiEnvThreadState.cpp	Sat Sep 05 12:42:05 2020 +0900
@@ -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 @@
 
 
 JvmtiFramePops* JvmtiEnvThreadState::get_frame_pops() {
-  assert(get_thread() == Thread::current() || SafepointSynchronize::is_at_safepoint(),
-         "frame pop data only accessible from same thread or at safepoint");
+#ifdef ASSERT
+  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 @@
 }
 
 void JvmtiEnvThreadState::set_frame_pop(int frame_number) {
-  assert(get_thread() == Thread::current() || SafepointSynchronize::is_at_safepoint(),
-         "frame pop data only accessible from same thread or at safepoint");
+#ifdef ASSERT
+  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(),
-         "frame pop data only accessible from same thread or at safepoint");
+#ifdef ASSERT
+  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(),
-         "frame pop data only accessible from same thread or at safepoint");
+#ifdef ASSERT
+  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(),
-         "frame pop data only accessible from same thread or at safepoint");
+#ifdef ASSERT
+  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 @@
 }
 
 
-class VM_GetCurrentLocation : public VM_Operation {
+class GetCurrentLocationClosure : public HandshakeClosure {
  private:
-   JavaThread *_thread;
    jmethodID _method_id;
    int _bci;
 
  public:
-  VM_GetCurrentLocation(JavaThread *thread) {
-     _thread = thread;
-   }
-  VMOp_Type type() const { return VMOp_GetCurrentLocation; }
-  void doit() {
-    ResourceMark rmark; // _thread != Thread::current()
-    RegisterMap rm(_thread, false);
+  GetCurrentLocationClosure()
+    : HandshakeClosure("GetCurrentLocation"),
+      _method_id(NULL),
+      _bci(0) {}
+  void do_thread(Thread *target) {
+    JavaThread *jt = (JavaThread *)target;
+    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()) {
-      javaVFrame* vf = _thread->last_java_vframe(&rm);
+    if (!jt->is_exiting() && jt->has_last_Java_frame()) {
+      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 @@
       jmethodID method_id;
       int bci;
       // The java thread stack may not be walkable for a running thread
-      // so get current location at safepoint.
-      VM_GetCurrentLocation op(_thread);
-      VMThread::execute(&op);
+      // so get current location with direct handshake.
+      GetCurrentLocationClosure 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);
     }
--- a/src/hotspot/share/prims/jvmtiEventController.cpp	Fri Sep 04 13:44:48 2020 -0400
+++ b/src/hotspot/share/prims/jvmtiEventController.cpp	Sat Sep 05 12:42:05 2020 +0900
@@ -331,10 +331,14 @@
   EC_TRACE(("[%s] # Entering interpreter only mode",
             JvmtiTrace::safe_get_thread_name(state->get_thread())));
   EnterInterpOnlyModeClosure hs;
-  if (SafepointSynchronize::is_at_safepoint()) {
-    hs.do_thread(state->get_thread());
+  assert(state->get_thread()->is_Java_thread(), "just checking");
+  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 @@
 
 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);
 }
 
--- a/src/hotspot/share/prims/jvmtiExport.cpp	Fri Sep 04 13:44:48 2020 -0400
+++ b/src/hotspot/share/prims/jvmtiExport.cpp	Sat Sep 05 12:42:05 2020 +0900
@@ -1645,7 +1645,10 @@
           }
         }
         // remove the frame's entry
-        ets->clear_frame_pop(cur_frame_number);
+        {
+          MutexLocker mu(JvmtiThreadState_lock);
+          ets->clear_frame_pop(cur_frame_number);
+        }
       }
     }
   }
--- a/src/hotspot/share/prims/jvmtiThreadState.cpp	Fri Sep 04 13:44:48 2020 -0400
+++ b/src/hotspot/share/prims/jvmtiThreadState.cpp	Sat Sep 05 12:42:05 2020 +0900
@@ -272,9 +272,9 @@
 }
 
 int JvmtiThreadState::cur_stack_depth() {
-  guarantee(SafepointSynchronize::is_at_safepoint() ||
-    (JavaThread *)Thread::current() == get_thread(),
-    "must be current thread or at safepoint");
+  Thread *current = Thread::current();
+  guarantee(current == get_thread() || current == get_thread()->active_handshaker(),
+            "must be current thread or direct handshake");
 
   if (!is_interp_only_mode() || _cur_stack_depth == UNKNOWN_STACK_DEPTH) {
     _cur_stack_depth = count_frames();
--- a/src/hotspot/share/runtime/handshake.cpp	Fri Sep 04 13:44:48 2020 -0400
+++ b/src/hotspot/share/runtime/handshake.cpp	Sat Sep 05 12:42:05 2020 +0900
@@ -383,9 +383,9 @@
   _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 @@
   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;
--- a/src/hotspot/share/runtime/handshake.hpp	Fri Sep 04 13:44:48 2020 -0400
+++ b/src/hotspot/share/runtime/handshake.hpp	Sat Sep 05 12:42:05 2020 +0900
@@ -106,11 +106,8 @@
   };
   ProcessResult try_process(HandshakeOperation* op);
 
-#ifdef ASSERT
   Thread* _active_handshaker;
   Thread* active_handshaker() const { return _active_handshaker; }
-#endif
-
 };
 
 #endif // SHARE_RUNTIME_HANDSHAKE_HPP
--- a/src/hotspot/share/runtime/thread.hpp	Fri Sep 04 13:44:48 2020 -0400
+++ b/src/hotspot/share/runtime/thread.hpp	Sat Sep 05 12:42:05 2020 +0900
@@ -1365,11 +1365,9 @@
     return _handshake.try_process(op);
   }
 
-#ifdef ASSERT
   Thread* active_handshaker() const {
     return _handshake.active_handshaker();
   }
-#endif
 
   // Suspend/resume support for JavaThread
  private:
--- a/src/hotspot/share/runtime/vmOperations.hpp	Fri Sep 04 13:44:48 2020 -0400
+++ b/src/hotspot/share/runtime/vmOperations.hpp	Sat Sep 05 12:42:05 2020 +0900
@@ -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)                  \