Visit invalid roots of only suspended threads
Since this always happens with suspended threads or self, you can
just visit these threads and do not require a suspend all. This
will not miss any roots if the caller was marking a thread root.
Fixes issues like transitioning to suspended and back blocking on a
thread suspension request from another thread. This could cause
deadlocks previously.
Bug: 29062271
Change-Id: I2fef149387aacf0cdc9a773d4f172c42fa53e4dc
diff --git a/runtime/gc/collector/mark_sweep.cc b/runtime/gc/collector/mark_sweep.cc
index 894ceba..ac5931f 100644
--- a/runtime/gc/collector/mark_sweep.cc
+++ b/runtime/gc/collector/mark_sweep.cc
@@ -444,27 +444,8 @@
}
PrintFileToLog("/proc/self/maps", LogSeverity::INTERNAL_FATAL);
MemMap::DumpMaps(LOG(INTERNAL_FATAL), true);
- {
- LOG(INTERNAL_FATAL) << "Attempting see if it's a bad root";
- Thread* self = Thread::Current();
- if (Locks::mutator_lock_->IsExclusiveHeld(self)) {
- mark_sweep_->VerifyRoots();
- } else {
- const bool heap_bitmap_exclusive_locked =
- Locks::heap_bitmap_lock_->IsExclusiveHeld(self);
- if (heap_bitmap_exclusive_locked) {
- Locks::heap_bitmap_lock_->ExclusiveUnlock(self);
- }
- {
- ScopedThreadSuspension(self, kSuspended);
- ScopedSuspendAll ssa(__FUNCTION__);
- mark_sweep_->VerifyRoots();
- }
- if (heap_bitmap_exclusive_locked) {
- Locks::heap_bitmap_lock_->ExclusiveLock(self);
- }
- }
- }
+ LOG(INTERNAL_FATAL) << "Attempting see if it's a bad thread root\n";
+ mark_sweep_->VerifySuspendedThreadRoots();
LOG(FATAL) << "Can't mark invalid object";
}
}
@@ -591,15 +572,15 @@
if (heap->GetLiveBitmap()->GetContinuousSpaceBitmap(root) == nullptr) {
space::LargeObjectSpace* large_object_space = heap->GetLargeObjectsSpace();
if (large_object_space != nullptr && !large_object_space->Contains(root)) {
- LOG(INTERNAL_FATAL) << "Found invalid root: " << root << " " << info;
+ LOG(INTERNAL_FATAL) << "Found invalid root: " << root << " " << info << "\n";
}
}
}
};
-void MarkSweep::VerifyRoots() {
+void MarkSweep::VerifySuspendedThreadRoots() {
VerifyRootVisitor visitor;
- Runtime::Current()->GetThreadList()->VisitRoots(&visitor);
+ Runtime::Current()->GetThreadList()->VisitRootsForSuspendedThreads(&visitor);
}
void MarkSweep::MarkRoots(Thread* self) {
diff --git a/runtime/gc/collector/mark_sweep.h b/runtime/gc/collector/mark_sweep.h
index c19107a..7168f96 100644
--- a/runtime/gc/collector/mark_sweep.h
+++ b/runtime/gc/collector/mark_sweep.h
@@ -250,8 +250,8 @@
// Verify the roots of the heap and print out information related to any invalid roots.
// Called in MarkObject, so may we may not hold the mutator lock.
- void VerifyRoots()
- NO_THREAD_SAFETY_ANALYSIS;
+ void VerifySuspendedThreadRoots()
+ SHARED_REQUIRES(Locks::mutator_lock_);
// Expand mark stack to 2x its current size.
void ExpandMarkStack()
diff --git a/runtime/thread_list.cc b/runtime/thread_list.cc
index 80b99fc..d90bd8d 100644
--- a/runtime/thread_list.cc
+++ b/runtime/thread_list.cc
@@ -1300,6 +1300,39 @@
}
}
+void ThreadList::VisitRootsForSuspendedThreads(RootVisitor* visitor) {
+ Thread* const self = Thread::Current();
+ std::vector<Thread*> threads_to_visit;
+
+ // Tell threads to suspend and copy them into list.
+ {
+ MutexLock mu(self, *Locks::thread_list_lock_);
+ MutexLock mu2(self, *Locks::thread_suspend_count_lock_);
+ for (Thread* thread : list_) {
+ thread->ModifySuspendCount(self, +1, nullptr, false);
+ if (thread == self || thread->IsSuspended()) {
+ threads_to_visit.push_back(thread);
+ } else {
+ thread->ModifySuspendCount(self, -1, nullptr, false);
+ }
+ }
+ }
+
+ // Visit roots without holding thread_list_lock_ and thread_suspend_count_lock_ to prevent lock
+ // order violations.
+ for (Thread* thread : threads_to_visit) {
+ thread->VisitRoots(visitor);
+ }
+
+ // Restore suspend counts.
+ {
+ MutexLock mu2(self, *Locks::thread_suspend_count_lock_);
+ for (Thread* thread : threads_to_visit) {
+ thread->ModifySuspendCount(self, -1, nullptr, false);
+ }
+ }
+}
+
void ThreadList::VisitRoots(RootVisitor* visitor) const {
MutexLock mu(Thread::Current(), *Locks::thread_list_lock_);
for (const auto& thread : list_) {
diff --git a/runtime/thread_list.h b/runtime/thread_list.h
index df81ad1..49f65e1 100644
--- a/runtime/thread_list.h
+++ b/runtime/thread_list.h
@@ -144,6 +144,10 @@
void VisitRoots(RootVisitor* visitor) const
SHARED_REQUIRES(Locks::mutator_lock_);
+ void VisitRootsForSuspendedThreads(RootVisitor* visitor)
+ REQUIRES(!Locks::thread_list_lock_, !Locks::thread_suspend_count_lock_)
+ SHARED_REQUIRES(Locks::mutator_lock_);
+
// Return a copy of the thread list.
std::list<Thread*> GetList() REQUIRES(Locks::thread_list_lock_) {
return list_;