Revert "Perform SetEvent under the user_code_suspend_count_lock_"
This reverts commit 3fa8b6db7b556035f192d037b35210677250798e.
Bug: 130150240
Reason for revert: Causes lock-level violations on gcstress
Change-Id: Ie580be5658da63f951652ba74cfbf25fa845765c
diff --git a/openjdkjvmti/events.cc b/openjdkjvmti/events.cc
index beb864b..2c5ebfe 100644
--- a/openjdkjvmti/events.cc
+++ b/openjdkjvmti/events.cc
@@ -38,7 +38,6 @@
#include "art_field-inl.h"
#include "art_jvmti.h"
#include "art_method-inl.h"
-#include "base/mutex.h"
#include "deopt_manager.h"
#include "dex/dex_file_types.h"
#include "gc/allocation_listener.h"
@@ -1268,65 +1267,57 @@
return ERR(MUST_POSSESS_CAPABILITY);
}
- if (thread != nullptr && !IsThreadControllable(event)) {
- return ERR(ILLEGAL_ARGUMENT);
+ art::Thread* art_thread = nullptr;
+ if (thread != nullptr) {
+ if (!IsThreadControllable(event)) {
+ return ERR(ILLEGAL_ARGUMENT);
+ }
+ art::ScopedObjectAccess soa(art::Thread::Current());
+ art::MutexLock mu(soa.Self(), *art::Locks::thread_list_lock_);
+ jvmtiError err = ERR(INTERNAL);
+ if (!ThreadUtil::GetAliveNativeThread(thread, soa, &art_thread, &err)) {
+ return err;
+ } else if (art_thread->IsStillStarting()) {
+ return ERR(THREAD_NOT_ALIVE);
+ }
+ art::ThreadState state = art_thread->GetState();
+ if (state == art::ThreadState::kStarting || state == art::ThreadState::kTerminated) {
+ return ERR(THREAD_NOT_ALIVE);
+ }
}
- art::Thread* self = art::Thread::Current();
- art::Thread* target = nullptr;
- do {
- ThreadUtil::SuspendCheck(self);
- art::MutexLock ucsl_mu(self, *art::Locks::user_code_suspension_lock_);
- // 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 (ThreadUtil::WouldSuspendForUserCodeLocked(self)) {
- continue;
- }
- bool old_state;
- bool new_state;
- {
- // From now on we know we cannot get suspended by user-code.
- // NB This does a SuspendCheck (during thread state change) so we need to
- // make sure we don't have the 'suspend_lock' locked here.
- art::ScopedObjectAccess soa(self);
- art::WriterMutexLock el_mu(self, envs_lock_);
- art::MutexLock tll_mu(self, *art::Locks::thread_list_lock_);
- jvmtiError err = ERR(INTERNAL);
- if (thread != nullptr) {
- if (!ThreadUtil::GetAliveNativeThread(thread, soa, &target, &err)) {
- return err;
- } else if (target->IsStillStarting() ||
- target->GetState() == art::ThreadState::kStarting) {
- target->Dump(LOG_STREAM(WARNING) << "Is not alive: ");
- return ERR(THREAD_NOT_ALIVE);
- }
- }
+ // TODO We use art_thread simply as a global unique identifier here. It is not safe to actually
+ // use it without holding the thread_list_lock_.
+ bool old_state;
+ bool new_state;
- {
- art::WriterMutexLock ei_mu(self, env->event_info_mutex_);
- old_state = global_mask.Test(event);
- if (mode == JVMTI_ENABLE) {
- env->event_masks.EnableEvent(env, target, event);
- global_mask.Set(event);
- new_state = true;
- } else {
- DCHECK_EQ(mode, JVMTI_DISABLE);
+ {
+ // Change the event masks atomically.
+ art::Thread* self = art::Thread::Current();
+ art::WriterMutexLock mu(self, envs_lock_);
+ art::WriterMutexLock mu_env_info(self, env->event_info_mutex_);
+ old_state = global_mask.Test(event);
+ if (mode == JVMTI_ENABLE) {
+ env->event_masks.EnableEvent(env, art_thread, event);
+ global_mask.Set(event);
+ new_state = true;
+ } else {
+ DCHECK_EQ(mode, JVMTI_DISABLE);
- env->event_masks.DisableEvent(env, target, event);
- RecalculateGlobalEventMaskLocked(event);
- new_state = global_mask.Test(event);
- }
- }
+ // TODO Replace art_thread with a uintptr_t or something to indicate we cannot read from it.
+ env->event_masks.DisableEvent(env, art_thread, event);
+ RecalculateGlobalEventMaskLocked(event);
+ new_state = global_mask.Test(event);
}
- // Handle any special work required for the event type. We still have the
- // user_code_suspend_count_lock_ so there won't be any interleaving here.
- if (new_state != old_state) {
- return HandleEventType(event, thread, mode == JVMTI_ENABLE);
- }
- return OK;
- } while (true);
+ }
+
+ // Handle any special work required for the event type.
+ if (new_state != old_state) {
+ return HandleEventType(event, thread, mode == JVMTI_ENABLE);
+ }
+
+ return ERR(NONE);
}
void EventHandler::HandleBreakpointEventsChanged(bool added) {
@@ -1349,7 +1340,7 @@
}
EventHandler::EventHandler()
- : envs_lock_("JVMTI Environment List Lock", art::LockLevel::kPostMutatorTopLockLevel),
+ : envs_lock_("JVMTI Environment List Lock", art::LockLevel::kTopLockLevel),
frame_pop_enabled(false) {
alloc_listener_.reset(new JvmtiAllocationListener(this));
ddm_listener_.reset(new JvmtiDdmChunkListener(this));
diff --git a/openjdkjvmti/events.h b/openjdkjvmti/events.h
index 2c3c7a0..e48772c 100644
--- a/openjdkjvmti/events.h
+++ b/openjdkjvmti/events.h
@@ -21,7 +21,6 @@
#include <vector>
#include <android-base/logging.h>
-#include <android-base/thread_annotations.h>
#include "base/macros.h"
#include "base/mutex.h"
@@ -324,9 +323,9 @@
// need to be able to remove arbitrary elements from it.
std::list<ArtJvmTiEnv*> envs GUARDED_BY(envs_lock_);
- // Close to top level lock. Nothing should be held when we lock this (except for mutator_lock_
- // which is needed when setting new events).
- mutable art::ReaderWriterMutex envs_lock_ ACQUIRED_AFTER(art::Locks::mutator_lock_);
+ // Top level lock. Nothing at all should be held when we lock this.
+ mutable art::ReaderWriterMutex envs_lock_
+ ACQUIRED_BEFORE(art::Locks::instrument_entrypoints_lock_);
// A union of all enabled events, anywhere.
EventMask global_mask;
diff --git a/runtime/base/locks.cc b/runtime/base/locks.cc
index 4349be0..a7922a2 100644
--- a/runtime/base/locks.cc
+++ b/runtime/base/locks.cc
@@ -158,9 +158,9 @@
DCHECK(runtime_thread_pool_lock_ != nullptr);
} else {
// Create global locks in level order from highest lock level to lowest.
- LockLevel current_lock_level = kUserCodeSuspensionLock;
- DCHECK(user_code_suspension_lock_ == nullptr);
- user_code_suspension_lock_ = new Mutex("user code suspension lock", current_lock_level);
+ LockLevel current_lock_level = kInstrumentEntrypointsLock;
+ DCHECK(instrument_entrypoints_lock_ == nullptr);
+ instrument_entrypoints_lock_ = new Mutex("instrument entrypoint lock", current_lock_level);
#define UPDATE_CURRENT_LOCK_LEVEL(new_level) \
if ((new_level) >= current_lock_level) { \
@@ -171,9 +171,9 @@
} \
current_lock_level = new_level;
- UPDATE_CURRENT_LOCK_LEVEL(kInstrumentEntrypointsLock);
- DCHECK(instrument_entrypoints_lock_ == nullptr);
- instrument_entrypoints_lock_ = new Mutex("instrument entrypoint lock", current_lock_level);
+ UPDATE_CURRENT_LOCK_LEVEL(kUserCodeSuspensionLock);
+ DCHECK(user_code_suspension_lock_ == nullptr);
+ user_code_suspension_lock_ = new Mutex("user code suspension lock", current_lock_level);
UPDATE_CURRENT_LOCK_LEVEL(kMutatorLock);
DCHECK(mutator_lock_ == nullptr);
diff --git a/runtime/base/locks.h b/runtime/base/locks.h
index b15fd32..b7d8e31 100644
--- a/runtime/base/locks.h
+++ b/runtime/base/locks.h
@@ -121,14 +121,9 @@
kRuntimeShutdownLock,
kTraceLock,
kHeapBitmapLock,
-
- // This is a generic lock level for a top-level lock meant to be gained after having the
- // mutator_lock_.
- kPostMutatorTopLockLevel,
-
kMutatorLock,
- kInstrumentEntrypointsLock,
kUserCodeSuspensionLock,
+ kInstrumentEntrypointsLock,
kZygoteCreationLock,
// The highest valid lock level. Use this if there is code that should only be called with no
@@ -179,13 +174,13 @@
// Check if the given mutex is in expected_mutexes_on_weak_ref_access_.
static bool IsExpectedOnWeakRefAccess(BaseMutex* mutex);
+ // Guards allocation entrypoint instrumenting.
+ static Mutex* instrument_entrypoints_lock_;
+
// Guards code that deals with user-code suspension. This mutex must be held when suspending or
// resuming threads with SuspendReason::kForUserCode. It may be held by a suspended thread, but
// only if the suspension is not due to SuspendReason::kForUserCode.
- static Mutex* user_code_suspension_lock_;
-
- // Guards allocation entrypoint instrumenting.
- static Mutex* instrument_entrypoints_lock_ ACQUIRED_AFTER(user_code_suspension_lock_);
+ static Mutex* user_code_suspension_lock_ ACQUIRED_AFTER(instrument_entrypoints_lock_);
// A barrier is used to synchronize the GC/Debugger thread with mutator threads. When GC/Debugger
// thread wants to suspend all mutator threads, it needs to wait for all mutator threads to pass
@@ -222,7 +217,7 @@
// state is changed | .. running ..
// - if the CAS operation fails then goto x | .. running ..
// .. running .. | .. running ..
- static MutatorMutex* mutator_lock_ ACQUIRED_AFTER(instrument_entrypoints_lock_);
+ static MutatorMutex* mutator_lock_ ACQUIRED_AFTER(user_code_suspension_lock_);
// Allow reader-writer mutual exclusion on the mark and live bitmaps of the heap.
static ReaderWriterMutex* heap_bitmap_lock_ ACQUIRED_AFTER(mutator_lock_);