Fix JDWP ObjectRegistry lock ordering.
Also make debugging lock ordering violations nicer by using our
LockLevel operator<<.
Change-Id: Ic15ebe70363a90a09f6491bd5c336a604e9d6c48
diff --git a/src/base/mutex-inl.h b/src/base/mutex-inl.h
index 0da0e18..3cb43a8 100644
--- a/src/base/mutex-inl.h
+++ b/src/base/mutex-inl.h
@@ -96,8 +96,9 @@
for (int i = level_; i >= 0; --i) {
BaseMutex* held_mutex = self->GetHeldMutex(static_cast<LockLevel>(i));
if (UNLIKELY(held_mutex != NULL)) {
- LOG(ERROR) << "Lock level violation: holding \"" << held_mutex->name_ << "\" (level " << i
- << ") while locking \"" << name_ << "\" (level " << static_cast<int>(level_) << ")";
+ LOG(ERROR) << "Lock level violation: holding \"" << held_mutex->name_ << "\" "
+ << "(level " << LockLevel(i) << ") while locking \"" << name_ << "\" "
+ << "(level " << level_ << ")";
if (i > kAbortLock) {
// Only abort in the check below if this is more than abort level lock.
bad_mutexes_held = true;
diff --git a/src/base/mutex.cc b/src/base/mutex.cc
index 393f2fc..1975f3b 100644
--- a/src/base/mutex.cc
+++ b/src/base/mutex.cc
@@ -149,13 +149,13 @@
if (kDebugLocking) {
CHECK(self->GetHeldMutex(level_) == this) << "Waiting on unacquired mutex: " << name_;
bool bad_mutexes_held = false;
- for (int i = kMaxMutexLevel; i >= 0; --i) {
+ for (int i = kLockLevelCount - 1; i >= 0; --i) {
if (i != level_) {
BaseMutex* held_mutex = self->GetHeldMutex(static_cast<LockLevel>(i));
if (held_mutex != NULL) {
- LOG(ERROR) << "Holding " << held_mutex->name_ << " (level " << i
- << ") while performing wait on: "
- << name_ << " (level " << static_cast<int>(level_) << ")";
+ LOG(ERROR) << "Holding \"" << held_mutex->name_ << "\" "
+ << "(level " << LockLevel(i) << ") while performing wait on "
+ << "\"" << name_ << "\" (level " << level_ << ")";
bad_mutexes_held = true;
}
}
diff --git a/src/debugger.h b/src/debugger.h
index 321bcf1..ad01011 100644
--- a/src/debugger.h
+++ b/src/debugger.h
@@ -102,7 +102,7 @@
*/
static void Connected();
static void GoActive() LOCKS_EXCLUDED(Locks::breakpoint_lock_);
- static void Disconnected();
+ static void Disconnected() SHARED_LOCKS_REQUIRED(Locks::mutator_lock_);
static void Disposed();
// Returns true if we're actually debugging with a real debugger, false if it's
diff --git a/src/jdwp/jdwp_main.cc b/src/jdwp/jdwp_main.cc
index 4a18d48..4e738ff 100644
--- a/src/jdwp/jdwp_main.cc
+++ b/src/jdwp/jdwp_main.cc
@@ -416,13 +416,15 @@
thread_->TransitionFromRunnableToSuspended(kWaitingInMainDebuggerLoop);
}
- /* release session state, e.g. remove breakpoint instructions */
{
ScopedObjectAccess soa(thread_);
+
+ // Release session state, e.g. remove breakpoint instructions.
ResetState();
+
+ // Tell the rest of the runtime that the debugger is no longer around.
+ Dbg::Disconnected();
}
- /* tell the interpreter that the debugger is no longer around */
- Dbg::Disconnected();
/* if we had threads suspended, resume them now */
Dbg::UndoDebuggerSuspensions();
diff --git a/src/jdwp/object_registry.cc b/src/jdwp/object_registry.cc
index 82898c1..9136506 100644
--- a/src/jdwp/object_registry.cc
+++ b/src/jdwp/object_registry.cc
@@ -30,7 +30,8 @@
return os;
}
-ObjectRegistry::ObjectRegistry() : lock_("ObjectRegistry lock"), next_id_(1) {
+ObjectRegistry::ObjectRegistry()
+ : lock_("ObjectRegistry lock", kJdwpObjectRegistryLock), next_id_(1) {
}
JDWP::RefTypeId ObjectRegistry::AddRefType(mirror::Class* c) {
diff --git a/src/jdwp/object_registry.h b/src/jdwp/object_registry.h
index 0d39806..e2893ca 100644
--- a/src/jdwp/object_registry.h
+++ b/src/jdwp/object_registry.h
@@ -63,12 +63,12 @@
bool Contains(mirror::Object* o) SHARED_LOCKS_REQUIRED(Locks::mutator_lock_);
- void Clear();
+ void Clear() SHARED_LOCKS_REQUIRED(Locks::mutator_lock_);
- void DisableCollection(JDWP::ObjectId id);
- void EnableCollection(JDWP::ObjectId id);
+ void DisableCollection(JDWP::ObjectId id) SHARED_LOCKS_REQUIRED(Locks::mutator_lock_);
+ void EnableCollection(JDWP::ObjectId id) SHARED_LOCKS_REQUIRED(Locks::mutator_lock_);
- bool IsCollected(JDWP::ObjectId id);
+ bool IsCollected(JDWP::ObjectId id) SHARED_LOCKS_REQUIRED(Locks::mutator_lock_);
void DisposeObject(JDWP::ObjectId id, uint32_t reference_count)
SHARED_LOCKS_REQUIRED(Locks::mutator_lock_);
@@ -79,8 +79,8 @@
private:
JDWP::ObjectId InternalAdd(mirror::Object* o) SHARED_LOCKS_REQUIRED(Locks::mutator_lock_);
mirror::Object* InternalGet(JDWP::ObjectId id) SHARED_LOCKS_REQUIRED(Locks::mutator_lock_);
- void Demote(ObjectRegistryEntry& entry);
- void Promote(ObjectRegistryEntry& entry);
+ void Demote(ObjectRegistryEntry& entry) SHARED_LOCKS_REQUIRED(Locks::mutator_lock_, lock_);
+ void Promote(ObjectRegistryEntry& entry) SHARED_LOCKS_REQUIRED(Locks::mutator_lock_, lock_);
Mutex lock_ DEFAULT_MUTEX_ACQUIRED_AFTER;
diff --git a/src/locks.h b/src/locks.h
index 5ad42f7..c0f6ae5 100644
--- a/src/locks.h
+++ b/src/locks.h
@@ -33,26 +33,28 @@
// [1] http://www.drdobbs.com/parallel/use-lock-hierarchies-to-avoid-deadlock/204801163
enum LockLevel {
kLoggingLock = 0,
- kUnexpectedSignalLock = 1,
- kThreadSuspendCountLock = 2,
- kAbortLock = 3,
- kDefaultMutexLevel = 4,
- kAllocSpaceLock = 5,
- kLoadLibraryLock = 6,
- kClassLinkerClassesLock = 7,
- kBreakpointLock = 8,
- kThreadListLock = 9,
- kBreakpointInvokeLock = 10,
- kJdwpEventListLock = 11,
- kJdwpAttachLock = 12,
- kJdwpStartLock = 13,
- kJdwpSerialLock = 14,
- kRuntimeShutdownLock = 15,
- kHeapBitmapLock = 16,
- kMonitorLock = 17,
- kMutatorLock = 18,
- kZygoteCreationLock = 19,
- kMaxMutexLevel = kZygoteCreationLock,
+ kUnexpectedSignalLock,
+ kThreadSuspendCountLock,
+ kAbortLock,
+ kDefaultMutexLevel,
+ kAllocSpaceLock,
+ kLoadLibraryLock,
+ kClassLinkerClassesLock,
+ kBreakpointLock,
+ kThreadListLock,
+ kBreakpointInvokeLock,
+ kJdwpObjectRegistryLock,
+ kJdwpEventListLock,
+ kJdwpAttachLock,
+ kJdwpStartLock,
+ kJdwpSerialLock,
+ kRuntimeShutdownLock,
+ kHeapBitmapLock,
+ kMonitorLock,
+ kMutatorLock,
+ kZygoteCreationLock,
+
+ kLockLevelCount // Must come last.
};
std::ostream& operator<<(std::ostream& os, const LockLevel& rhs);
diff --git a/src/thread-inl.h b/src/thread-inl.h
index cf92a1c..414b8d8 100644
--- a/src/thread-inl.h
+++ b/src/thread-inl.h
@@ -41,7 +41,7 @@
CHECK_EQ(0u, no_thread_suspension_) << last_no_thread_suspension_cause_;
if (check_locks) {
bool bad_mutexes_held = false;
- for (int i = kMaxMutexLevel; i >= 0; --i) {
+ for (int i = kLockLevelCount - 1; i >= 0; --i) {
// We expect no locks except the mutator_lock_.
if (i != kMutatorLock) {
BaseMutex* held_mutex = GetHeldMutex(static_cast<LockLevel>(i));
diff --git a/src/thread.cc b/src/thread.cc
index 0d3c5b9..ca97e89 100644
--- a/src/thread.cc
+++ b/src/thread.cc
@@ -644,7 +644,7 @@
}
// Release locks and come out of runnable state.
}
- for (int i = kMaxMutexLevel; i >= 0; --i) {
+ for (int i = kLockLevelCount - 1; i >= 0; --i) {
BaseMutex* held_mutex = Thread::Current()->GetHeldMutex(static_cast<LockLevel>(i));
if (held_mutex != NULL) {
LOG(FATAL) << "Holding " << held_mutex->GetName()
diff --git a/src/thread.h b/src/thread.h
index 7f8bd7e..1992dc9 100644
--- a/src/thread.h
+++ b/src/thread.h
@@ -769,7 +769,7 @@
pthread_t pthread_self_;
// Support for Mutex lock hierarchy bug detection.
- BaseMutex* held_mutexes_[kMaxMutexLevel + 1];
+ BaseMutex* held_mutexes_[kLockLevelCount];
// A positive value implies we're in a region where thread suspension isn't expected.
uint32_t no_thread_suspension_;