Add more checking to WrappedSuspend1Barrier am: 32578abac2 am: f1bc391c03
Original change: https://android-review.googlesource.com/c/platform/art/+/2966069
Change-Id: I6953c42f02474b18c01af6be9f144141491e8b0b
Signed-off-by: Automerger Merge Worker <android-build-automerger-merge-worker@system.gserviceaccount.com>
diff --git a/runtime/thread-inl.h b/runtime/thread-inl.h
index 2fcc4b0..57d606c 100644
--- a/runtime/thread-inl.h
+++ b/runtime/thread-inl.h
@@ -279,7 +279,21 @@
}
}
+inline void Thread::CheckBarrierInactive(WrappedSuspend1Barrier* suspend1_barrier) {
+ for (WrappedSuspend1Barrier* w = tlsPtr_.active_suspend1_barriers; w != nullptr; w = w->next_) {
+ CHECK_EQ(w->magic_, WrappedSuspend1Barrier::kMagic)
+ << "first = " << tlsPtr_.active_suspend1_barriers << " current = " << w
+ << " next = " << w->next_;
+ CHECK_NE(w, suspend1_barrier);
+ }
+}
+
inline void Thread::AddSuspend1Barrier(WrappedSuspend1Barrier* suspend1_barrier) {
+ if (tlsPtr_.active_suspend1_barriers != nullptr) {
+ CHECK_EQ(tlsPtr_.active_suspend1_barriers->magic_, WrappedSuspend1Barrier::kMagic)
+ << "first = " << tlsPtr_.active_suspend1_barriers;
+ }
+ CHECK_EQ(suspend1_barrier->magic_, WrappedSuspend1Barrier::kMagic);
suspend1_barrier->next_ = tlsPtr_.active_suspend1_barriers;
tlsPtr_.active_suspend1_barriers = suspend1_barrier;
}
diff --git a/runtime/thread.cc b/runtime/thread.cc
index 0363288..2ebbe13 100644
--- a/runtime/thread.cc
+++ b/runtime/thread.cc
@@ -1513,6 +1513,9 @@
tlsPtr_.active_suspendall_barrier = nullptr;
}
for (WrappedSuspend1Barrier* w = tlsPtr_.active_suspend1_barriers; w != nullptr; w = w->next_) {
+ CHECK_EQ(w->magic_, WrappedSuspend1Barrier::kMagic)
+ << "first = " << tlsPtr_.active_suspend1_barriers << " current = " << w
+ << " next = " << w->next_;
pass_barriers.push_back(&(w->barrier_));
}
tlsPtr_.active_suspend1_barriers = nullptr;
@@ -1685,9 +1688,9 @@
// Although this is a thread suspension, the target thread only blocks while we run the
// checkpoint, which is presumed to terminate quickly even if other threads are blocked.
// Note: IncrementSuspendCount also expects the thread_list_lock to be held unless this == self.
+ WrappedSuspend1Barrier wrapped_barrier{};
{
bool is_suspended = false;
- WrappedSuspend1Barrier wrapped_barrier{};
{
MutexLock suspend_count_mu(self, *Locks::thread_suspend_count_lock_);
@@ -1767,6 +1770,9 @@
DCHECK_NE(GetState(), ThreadState::kRunnable);
DCHECK_GT(GetSuspendCount(), 0);
DecrementSuspendCount(self);
+ if (kIsDebugBuild) {
+ CheckBarrierInactive(&wrapped_barrier);
+ }
resume_cond_->Broadcast(self);
}
diff --git a/runtime/thread.h b/runtime/thread.h
index 5dcdb8b..8bcd815 100644
--- a/runtime/thread.h
+++ b/runtime/thread.h
@@ -199,8 +199,11 @@
// See Thread.tlsPtr_.active_suspend1_barriers below for explanation.
struct WrappedSuspend1Barrier {
- WrappedSuspend1Barrier() : barrier_(1), next_(nullptr) {}
- AtomicInteger barrier_; // Only updated while holding thread_suspend_count_lock_ .
+ // TODO(b/23668816): At least weaken CHECKs to DCHECKs once the bug is fixed.
+ static constexpr int kMagic = 0xba8;
+ WrappedSuspend1Barrier() : magic_(kMagic), barrier_(1), next_(nullptr) {}
+ int magic_;
+ AtomicInteger barrier_;
struct WrappedSuspend1Barrier* next_ GUARDED_BY(Locks::thread_suspend_count_lock_);
};
@@ -1769,6 +1772,10 @@
ALWAYS_INLINE bool HasActiveSuspendBarrier() REQUIRES(Locks::thread_suspend_count_lock_);
+ // CHECK that the given barrier is no longer on our list.
+ ALWAYS_INLINE void CheckBarrierInactive(WrappedSuspend1Barrier* suspend1_barrier)
+ REQUIRES(Locks::thread_suspend_count_lock_);
+
// Registers the current thread as the jit sensitive thread. Should be called just once.
static void SetJitSensitiveThread() {
if (jit_sensitive_thread_ == nullptr) {
diff --git a/runtime/thread_list.cc b/runtime/thread_list.cc
index b841eaa..02e3f54 100644
--- a/runtime/thread_list.cc
+++ b/runtime/thread_list.cc
@@ -1089,6 +1089,8 @@
if (thread->IsSuspended()) {
// See the discussion in mutator_gc_coord.md and SuspendAllInternal for the race here.
thread->RemoveFirstSuspend1Barrier(&wrapped_barrier);
+ // PassActiveSuspendBarriers couldn't have seen our barrier, since it also acquires
+ // 'thread_suspend_count_lock_'. `wrapped_barrier` will not be accessed.
if (!thread->HasActiveSuspendBarrier()) {
thread->AtomicClearFlag(ThreadFlag::kActiveSuspendBarrier);
}
@@ -1185,7 +1187,7 @@
}
is_suspended = true;
}
- // wrapped_barrier.barrier_ has been decremented and will no longer be accessed.
+ // wrapped_barrier.barrier_ will no longer be accessed.
VLOG(threads) << func_name << " suspended: " << *thread;
if (ATraceEnabled()) {
std::string name;
@@ -1194,7 +1196,11 @@
StringPrintf("%s suspended %s for tid=%d", func_name, name.c_str(), thread->GetTid())
.c_str());
}
- DCHECK(thread->IsSuspended());
+ if (kIsDebugBuild) {
+ CHECK(thread->IsSuspended());
+ MutexLock suspend_count_mu(self, *Locks::thread_suspend_count_lock_);
+ thread->CheckBarrierInactive(&wrapped_barrier);
+ }
return true;
}