Fixes for ThreadStress test
- Fix deadlock when ThreadGroup.remove(Thread) was run with
thread_list_lock_ but needed to GC to allocate an iterator
- Fix ~MonitorList to clean ownership of any locks that might be held
by JNI or daemon threads on shutdown.
Change-Id: I95e23c3b7c745f6a8387789949f3ec849458a27d
diff --git a/src/monitor.cc b/src/monitor.cc
index b6f91bc..16298ae 100644
--- a/src/monitor.cc
+++ b/src/monitor.cc
@@ -129,14 +129,9 @@
DCHECK_EQ(LW_SHAPE(*obj_->GetRawLockWordAddress()), LW_SHAPE_FAT);
#ifndef NDEBUG
- /* This lock is associated with an object
- * that's being swept. The only possible way
- * anyone could be holding this lock would be
- * if some JNI code locked but didn't unlock
- * the object, in which case we've got some bad
- * native code somewhere.
- */
- DCHECK(lock_.TryLock());
+ // This lock is associated with an object that's being swept.
+ bool locked = lock_.TryLock();
+ DCHECK(locked) << obj_;
lock_.Unlock();
#endif
}
@@ -820,6 +815,20 @@
MonitorList::~MonitorList() {
MutexLock mu(lock_);
+
+ // In case there is a daemon thread with the monitor locked, clear
+ // the owner here so we can destroy the mutex, which will otherwise
+ // fail in pthread_mutex_destroy.
+ typedef std::list<Monitor*>::iterator It; // TODO: C++0x auto
+ for (It it = list_.begin(); it != list_.end(); it++) {
+ Monitor* monitor = *it;
+ Mutex& lock = monitor->lock_;
+ if (lock.GetOwner() != 0) {
+ DCHECK_EQ(lock.GetOwner(), monitor->owner_->GetTid());
+ lock.ClearOwner();
+ }
+ }
+
STLDeleteElements(&list_);
}
diff --git a/src/monitor.h b/src/monitor.h
index 632c5d3..be704ba 100644
--- a/src/monitor.h
+++ b/src/monitor.h
@@ -101,27 +101,26 @@
static bool is_verbose_;
static uint32_t lock_profiling_threshold_;
- /* Which thread currently owns the lock? */
+ // Which thread currently owns the lock?
Thread* owner_;
- /* Owner's recursive lock depth */
+ // Owner's recursive lock depth.
int lock_count_;
- /* What object are we part of (for debugging). */
+ // What object are we part of (for debugging).
Object* obj_;
- /* Threads currently waiting on this monitor. */
+ // Threads currently waiting on this monitor.
Thread* wait_set_;
Mutex lock_;
- /*
- * Who last acquired this monitor, when lock sampling is enabled.
- * Even when enabled, ownerFileName may be NULL.
- */
+ // Who last acquired this monitor, when lock sampling is enabled.
+ // Even when enabled, owner_filename_ may be NULL.
const char* owner_filename_;
uint32_t owner_line_number_;
+ friend class MonitorList;
friend class Object;
DISALLOW_COPY_AND_ASSIGN(Monitor);
};
@@ -142,9 +141,7 @@
DISALLOW_COPY_AND_ASSIGN(MonitorList);
};
-/*
- * Relative timed wait on condition
- */
+// Relative timed wait on condition
int dvmRelativeCondWait(pthread_cond_t* cond, pthread_mutex_t* mutex, int64_t msec, int32_t nsec);
} // namespace art
diff --git a/src/mutex.cc b/src/mutex.cc
index ee096a3..25d8b76 100644
--- a/src/mutex.cc
+++ b/src/mutex.cc
@@ -83,6 +83,23 @@
#endif
}
+void Mutex::ClearOwner() {
+#if defined(__BIONIC__)
+ mutex_.value = 0;
+#elif defined(__GLIBC__)
+ struct __attribute__((__may_alias__)) glibc_pthread_t {
+ int lock;
+ unsigned int count;
+ int owner;
+ // ...other stuff we don't care about.
+ };
+ reinterpret_cast<glibc_pthread_t*>(&mutex_)->owner = 0;
+#else
+ UNIMPLEMENTED(FATAL);
+ return 0;
+#endif
+}
+
pid_t Mutex::GetTid() {
return art::GetTid();
}
diff --git a/src/mutex.h b/src/mutex.h
index a276b5d..f22c5dc 100644
--- a/src/mutex.h
+++ b/src/mutex.h
@@ -57,10 +57,13 @@
private:
static pid_t GetTid();
+ void ClearOwner();
+
std::string name_;
pthread_mutex_t mutex_;
+ friend class MonitorList; // for ClearOwner
DISALLOW_COPY_AND_ASSIGN(Mutex);
};
diff --git a/src/thread.cc b/src/thread.cc
index 1800834..cbd1bda 100644
--- a/src/thread.cc
+++ b/src/thread.cc
@@ -745,22 +745,12 @@
}
Thread::~Thread() {
- SetState(Thread::kRunnable);
-
// On thread detach, all monitors entered with JNI MonitorEnter are automatically exited.
if (jni_env_ != NULL) {
jni_env_->monitors.VisitRoots(MonitorExitVisitor, NULL);
}
if (peer_ != NULL) {
- // this.group.removeThread(this);
- // group can be null if we're in the compiler or a test.
- Object* group = gThread_group->GetObject(peer_);
- if (group != NULL) {
- Method* m = group->GetClass()->FindVirtualMethodForVirtualOrInterface(gThreadGroup_removeThread);
- Object* args = peer_;
- m->Invoke(this, group, reinterpret_cast<byte*>(&args), NULL);
- }
// this.vmData = 0;
SetVmData(peer_, NULL);
@@ -795,8 +785,6 @@
return;
}
- ScopedThreadStateChange tsc(this, Thread::kRunnable);
-
// Get and clear the exception.
Object* exception = GetException();
ClearException();
@@ -819,6 +807,17 @@
ClearException();
}
+void Thread::RemoveFromThreadGroup() {
+ // this.group.removeThread(this);
+ // group can be null if we're in the compiler or a test.
+ Object* group = gThread_group->GetObject(peer_);
+ if (group != NULL) {
+ Method* m = group->GetClass()->FindVirtualMethodForVirtualOrInterface(gThreadGroup_removeThread);
+ Object* args = peer_;
+ m->Invoke(this, group, reinterpret_cast<byte*>(&args), NULL);
+ }
+}
+
size_t Thread::NumSirtReferences() {
size_t count = 0;
for (StackIndirectReferenceTable* cur = top_sirt_; cur; cur = cur->GetLink()) {
diff --git a/src/thread.h b/src/thread.h
index e54fea2..8edeaf2 100644
--- a/src/thread.h
+++ b/src/thread.h
@@ -474,6 +474,7 @@
static void* CreateCallback(void* arg);
void HandleUncaughtExceptions();
+ void RemoveFromThreadGroup();
void InitCpu();
void InitFunctionPointers();
diff --git a/src/thread_list.cc b/src/thread_list.cc
index 8969868..dfce2a2 100644
--- a/src/thread_list.cc
+++ b/src/thread_list.cc
@@ -29,8 +29,13 @@
// simultaneously by going to kVmWait if the lock cannot be
// immediately acquired.
if (!thread_list_->thread_list_lock_.TryLock()) {
- ScopedThreadStateChange tsc(Thread::Current(), Thread::kVmWait);
- thread_list_->thread_list_lock_.Lock();
+ Thread* self = Thread::Current();
+ if (self == NULL) {
+ thread_list_->thread_list_lock_.Lock();
+ } else {
+ ScopedThreadStateChange tsc(self, Thread::kVmWait);
+ thread_list_->thread_list_lock_.Lock();
+ }
}
}
@@ -294,9 +299,18 @@
LOG(INFO) << "ThreadList::Unregister() " << *self;
}
- // This may need to call user-supplied managed code. Make sure we do this before we start tearing
- // down the Thread* and removing it from the thread list (or start taking any locks).
- self->HandleUncaughtExceptions();
+ if (self->GetPeer() != NULL) {
+ self->SetState(Thread::kRunnable);
+
+ // This may need to call user-supplied managed code. Make sure we do this before we start tearing
+ // down the Thread* and removing it from the thread list (or start taking any locks).
+ self->HandleUncaughtExceptions();
+
+ // Make sure we remove from ThreadGroup before taking the
+ // thread_list_lock_ since it allocates an Iterator which can cause
+ // a GC which will want to suspend.
+ self->RemoveFromThreadGroup();
+ }
ThreadListLocker locker(this);