Fix issue with RawMonitors around thread suspension.
Investigation of real-world JVMTI agents revealed that some rely on
the RawMonitorEnter function acting as a Java suspend point. If it
fails to act as one the agent could end up deadlocked.
Test: ./test.py --host -j50
Bug: 62821960
Bug: 34415266
Change-Id: I3daf5c49c1c9870e1f69eebfd4c6f2ad15224510
diff --git a/openjdkjvmti/ti_monitor.cc b/openjdkjvmti/ti_monitor.cc
index 1ec566b..adaa48c 100644
--- a/openjdkjvmti/ti_monitor.cc
+++ b/openjdkjvmti/ti_monitor.cc
@@ -40,6 +40,7 @@
#include "runtime.h"
#include "scoped_thread_state_change-inl.h"
#include "thread-current-inl.h"
+#include "ti_thread.h"
namespace openjdkjvmti {
@@ -51,8 +52,7 @@
class JvmtiMonitor {
public:
- JvmtiMonitor() : owner_(nullptr), count_(0) {
- }
+ JvmtiMonitor() : owner_(nullptr), count_(0) { }
static bool Destroy(art::Thread* self, JvmtiMonitor* monitor) NO_THREAD_SAFETY_ANALYSIS {
// Check whether this thread holds the monitor, or nobody does.
@@ -72,13 +72,41 @@
}
void MonitorEnter(art::Thread* self) NO_THREAD_SAFETY_ANALYSIS {
- // Check for recursive enter.
- if (IsOwner(self)) {
- count_++;
- return;
- }
+ // Perform a suspend-check. The spec doesn't require this but real-world agents depend on this
+ // behavior. We do this by performing a suspend-check then retrying if the thread is suspended
+ // before or after locking the internal mutex.
+ do {
+ ThreadUtil::SuspendCheck(self);
+ if (ThreadUtil::WouldSuspendForUserCode(self)) {
+ continue;
+ }
- mutex_.lock();
+ // Check for recursive enter.
+ if (IsOwner(self)) {
+ count_++;
+ return;
+ }
+
+ // Checking for user-code suspension takes acquiring 2 art::Mutexes so we want to avoid doing
+ // that if possible. To avoid it we try to get the internal mutex without sleeping. If we do
+ // this we don't bother doing another suspend check since it can linearize after the lock.
+ if (mutex_.try_lock()) {
+ break;
+ } else {
+ // Lock with sleep. We will need to check for suspension after this to make sure that agents
+ // won't deadlock.
+ mutex_.lock();
+ if (!ThreadUtil::WouldSuspendForUserCode(self)) {
+ break;
+ } else {
+ // We got suspended in the middle of waiting for the mutex. We should release the mutex
+ // and try again so we can get it while not suspended. This lets some other
+ // (non-suspended) thread acquire the mutex in case it's waiting to wake us up.
+ mutex_.unlock();
+ continue;
+ }
+ }
+ } while (true);
DCHECK(owner_.load(std::memory_order_relaxed) == nullptr);
owner_.store(self, std::memory_order_relaxed);
@@ -123,7 +151,7 @@
}
private:
- bool IsOwner(art::Thread* self) {
+ bool IsOwner(art::Thread* self) const {
// There's a subtle correctness argument here for a relaxed load outside the critical section.
// A thread is guaranteed to see either its own latest store or another thread's store. If a
// thread sees another thread's store than it cannot be holding the lock.
diff --git a/openjdkjvmti/ti_thread.cc b/openjdkjvmti/ti_thread.cc
index b0a1a85..27d01ea 100644
--- a/openjdkjvmti/ti_thread.cc
+++ b/openjdkjvmti/ti_thread.cc
@@ -85,7 +85,8 @@
if (name != "JDWP" &&
name != "Signal Catcher" &&
!android::base::StartsWith(name, "Jit thread pool")) {
- LOG(FATAL) << "Unexpected thread before start: " << name;
+ LOG(FATAL) << "Unexpected thread before start: " << name << " id: "
+ << self->GetThreadId();
}
}
return;
@@ -413,13 +414,24 @@
}
// Suspends the current thread if it has any suspend requests on it.
-static void SuspendCheck(art::Thread* self)
- REQUIRES(!art::Locks::mutator_lock_, !art::Locks::user_code_suspension_lock_) {
+void ThreadUtil::SuspendCheck(art::Thread* self) {
art::ScopedObjectAccess soa(self);
// Really this is only needed if we are in FastJNI and actually have the mutator_lock_ already.
self->FullSuspendCheck();
}
+bool ThreadUtil::WouldSuspendForUserCodeLocked(art::Thread* self) {
+ DCHECK(self == art::Thread::Current());
+ art::MutexLock tscl_mu(self, *art::Locks::thread_suspend_count_lock_);
+ return self->GetUserCodeSuspendCount() != 0;
+}
+
+bool ThreadUtil::WouldSuspendForUserCode(art::Thread* self) {
+ DCHECK(self == art::Thread::Current());
+ art::MutexLock ucsl_mu(self, *art::Locks::user_code_suspension_lock_);
+ return WouldSuspendForUserCodeLocked(self);
+}
+
jvmtiError ThreadUtil::GetThreadState(jvmtiEnv* env ATTRIBUTE_UNUSED,
jthread thread,
jint* thread_state_ptr) {
@@ -435,13 +447,10 @@
do {
SuspendCheck(self);
art::MutexLock ucsl_mu(self, *art::Locks::user_code_suspension_lock_);
- {
- art::MutexLock tscl_mu(self, *art::Locks::thread_suspend_count_lock_);
- if (self->GetUserCodeSuspendCount() != 0) {
- // Make sure we won't be suspended in the middle of holding the thread_suspend_count_lock_
- // by a user-code suspension. We retry and do another SuspendCheck to clear this.
- continue;
- }
+ if (WouldSuspendForUserCodeLocked(self)) {
+ // Make sure we won't be suspended in the middle of holding the thread_suspend_count_lock_ by
+ // a user-code suspension. We retry and do another SuspendCheck to clear this.
+ continue;
}
art::ScopedObjectAccess soa(self);
art::MutexLock tll_mu(self, *art::Locks::thread_list_lock_);
@@ -657,6 +666,9 @@
jvmtiStartFunction proc,
const void* arg,
jint priority) {
+ if (!PhaseUtil::IsLivePhase()) {
+ return ERR(WRONG_PHASE);
+ }
if (priority < JVMTI_THREAD_MIN_PRIORITY || priority > JVMTI_THREAD_MAX_PRIORITY) {
return ERR(INVALID_PRIORITY);
}
@@ -703,15 +715,12 @@
// before continuing.
SuspendCheck(self);
art::MutexLock mu(self, *art::Locks::user_code_suspension_lock_);
- {
- art::MutexLock thread_suspend_count_mu(self, *art::Locks::thread_suspend_count_lock_);
+ if (WouldSuspendForUserCodeLocked(self)) {
// Make sure we won't be suspended in the middle of holding the thread_suspend_count_lock_ by
// a user-code suspension. We retry and do another SuspendCheck to clear this.
- if (self->GetUserCodeSuspendCount() != 0) {
- continue;
- }
- // We are not going to be suspended by user code from now on.
+ continue;
}
+ // We are not going to be suspended by user code from now on.
{
art::ScopedObjectAccess soa(self);
art::MutexLock thread_list_mu(self, *art::Locks::thread_list_lock_);
@@ -798,13 +807,10 @@
do {
SuspendCheck(self);
art::MutexLock ucsl_mu(self, *art::Locks::user_code_suspension_lock_);
- {
- art::MutexLock tscl_mu(self, *art::Locks::thread_suspend_count_lock_);
+ if (WouldSuspendForUserCodeLocked(self)) {
// Make sure we won't be suspended in the middle of holding the thread_suspend_count_lock_ by
// a user-code suspension. We retry and do another SuspendCheck to clear this.
- if (self->GetUserCodeSuspendCount() != 0) {
- continue;
- }
+ continue;
}
// From now on we know we cannot get suspended by user-code.
{
diff --git a/openjdkjvmti/ti_thread.h b/openjdkjvmti/ti_thread.h
index a19974a..57b1943 100644
--- a/openjdkjvmti/ti_thread.h
+++ b/openjdkjvmti/ti_thread.h
@@ -98,6 +98,22 @@
REQUIRES_SHARED(art::Locks::mutator_lock_)
REQUIRES(art::Locks::thread_list_lock_);
+ // Go to sleep if this thread is suspended.
+ static void SuspendCheck(art::Thread* self)
+ REQUIRES(!art::Locks::mutator_lock_, !art::Locks::user_code_suspension_lock_);
+
+ // Returns true if the thread would be suspended if it locks the mutator-lock or calls
+ // SuspendCheck. This function is called with the user_code_suspension_lock already held.
+ static bool WouldSuspendForUserCodeLocked(art::Thread* self)
+ REQUIRES(art::Locks::user_code_suspension_lock_,
+ !art::Locks::thread_suspend_count_lock_);
+
+ // Returns true if this thread would go to sleep if it locks the mutator-lock or calls
+ // SuspendCheck.
+ static bool WouldSuspendForUserCode(art::Thread* self)
+ REQUIRES(!art::Locks::user_code_suspension_lock_,
+ !art::Locks::thread_suspend_count_lock_);
+
private:
// We need to make sure only one thread tries to suspend threads at a time so we can get the
// 'suspend-only-once' behavior the spec requires. Internally, ART considers suspension to be a