From 8bc6a58df7046b4d6f4b51eb274c7e60fea396ff Mon Sep 17 00:00:00 2001 From: Hans Boehm Date: Tue, 19 Dec 2023 18:48:15 +0000 Subject: Revert^17 "Thread suspension cleanup and deadlock fix" This reverts commit c6371b52df0da31acc174a3526274417b7aac0a7. Reason for revert: This seems to have two remaining issues: 1. The second DCHECK in WaitForFlipFunction is not completely guaranteed to hold, resulting in failures for 658-fp-read-barrier. 2. WaitForSuspendBarrier seems to time out occasionally, possibly spuriously so. We fail when the futex times out once. That's probably incompatible with the app freezer. We should retry a few times. Change-Id: Ibd8909b31083fc29e6d4f1fcde003d08eb16fc0a --- openjdkjvmti/ti_heap.cc | 7 ------ openjdkjvmti/ti_object.cc | 14 ++++------- openjdkjvmti/ti_stack.cc | 12 ++++------ openjdkjvmti/ti_thread.cc | 54 +++++++++++++++++------------------------- openjdkjvmti/ti_threadgroup.cc | 26 +++++++------------- 5 files changed, 41 insertions(+), 72 deletions(-) (limited to 'openjdkjvmti') diff --git a/openjdkjvmti/ti_heap.cc b/openjdkjvmti/ti_heap.cc index 80bfa0ff43..662f464ad2 100644 --- a/openjdkjvmti/ti_heap.cc +++ b/openjdkjvmti/ti_heap.cc @@ -975,13 +975,6 @@ class FollowReferencesHelper final { jvmtiHeapReferenceKind GetReferenceKind(const art::RootInfo& info, jvmtiHeapReferenceInfo* ref_info) REQUIRES_SHARED(art::Locks::mutator_lock_) { - // We do not necessarily hold thread_list_lock_ here, but we may if we are called from - // VisitThreadRoots, which can happen from JVMTI FollowReferences. If it was acquired in - // ThreadList::VisitRoots, it's unsafe to temporarily release it. Thus we act as if we did - // not hold the thread_list_lock_ here, and relax CHECKs appropriately. If it does happen, - // we are in a SuspendAll situation with concurrent GC disabled, and should not need to run - // flip functions. TODO: Find a way to clean this up. - // TODO: Fill in ref_info. memset(ref_info, 0, sizeof(jvmtiHeapReferenceInfo)); diff --git a/openjdkjvmti/ti_object.cc b/openjdkjvmti/ti_object.cc index 3333a8a834..f37df86048 100644 --- a/openjdkjvmti/ti_object.cc +++ b/openjdkjvmti/ti_object.cc @@ -105,17 +105,13 @@ jvmtiError ObjectUtil::GetObjectMonitorUsage( notify_wait.push_back(jni->AddLocalReference(thd->GetPeerFromOtherThread())); wait.push_back(jni->AddLocalReference(thd->GetPeerFromOtherThread())); } - // Scan all threads to see which are waiting on this particular monitor. - std::list thread_list; { - // Since we're in a SuspendAll, exiting threads are not a concern. + // Scan all threads to see which are waiting on this particular monitor. art::MutexLock tll(self, *art::Locks::thread_list_lock_); - thread_list = art::Runtime::Current()->GetThreadList()->GetList(); - } - for (art::Thread* thd : thread_list) { - if (thd != info.owner_ && target.Ptr() == thd->GetMonitorEnterObject()) { - art::mirror::Object* peer = thd->GetPeerFromOtherThread(); - wait.push_back(jni->AddLocalReference(peer)); + for (art::Thread* thd : art::Runtime::Current()->GetThreadList()->GetList()) { + if (thd != info.owner_ && target.Ptr() == thd->GetMonitorEnterObject()) { + wait.push_back(jni->AddLocalReference(thd->GetPeerFromOtherThread())); + } } } } diff --git a/openjdkjvmti/ti_stack.cc b/openjdkjvmti/ti_stack.cc index a56081408a..9af8861260 100644 --- a/openjdkjvmti/ti_stack.cc +++ b/openjdkjvmti/ti_stack.cc @@ -363,13 +363,7 @@ static void RunCheckpointAndWait(Data* data, size_t max_frame_count) REQUIRES_SHARED(art::Locks::mutator_lock_) { // Note: requires the mutator lock as the checkpoint requires the mutator lock. GetAllStackTracesVectorClosure closure(max_frame_count, data); - // TODO(b/253671779): Replace this use of RunCheckpointUnchecked() with RunCheckpoint(). This is - // currently not possible, since the following undesirable call chain (abbreviated here) is then - // possible and exercised by current tests: (jvmti) GetAllStackTraces -> -> - // RunCheckpoint -> GetStackTraceVisitor -> EncodeMethodId -> Class::EnsureMethodIds -> - // Class::Alloc -> AllocObjectWithAllocator -> potentially suspends, or runs GC, etc. -> CHECK - // failure. - size_t barrier_count = art::Runtime::Current()->GetThreadList()->RunCheckpointUnchecked(&closure); + size_t barrier_count = art::Runtime::Current()->GetThreadList()->RunCheckpoint(&closure, nullptr); if (barrier_count == 0) { return; } @@ -550,6 +544,7 @@ jvmtiError StackUtil::GetThreadListStackTraces(jvmtiEnv* env, // Found the thread. art::MutexLock mu(self, mutex); + threads.push_back(thread); thread_list_indices.push_back(index); frames.emplace_back(new std::vector()); @@ -567,6 +562,7 @@ jvmtiError StackUtil::GetThreadListStackTraces(jvmtiEnv* env, // Storage. Only access directly after completion. + std::vector threads; std::vector thread_list_indices; std::vector>> frames; @@ -604,10 +600,12 @@ jvmtiError StackUtil::GetThreadListStackTraces(jvmtiEnv* env, jvmtiStackInfo& stack_info = stack_info_array.get()[index]; memset(&stack_info, 0, sizeof(jvmtiStackInfo)); + art::Thread* self = data.threads[index]; const std::vector& thread_frames = *data.frames[index].get(); // For the time being, set the thread to null. We don't have good ScopedLocalRef // infrastructure. + DCHECK(self->GetPeerFromOtherThread() != nullptr); stack_info.thread = nullptr; stack_info.state = JVMTI_THREAD_STATE_SUSPENDED; diff --git a/openjdkjvmti/ti_thread.cc b/openjdkjvmti/ti_thread.cc index 191b63c050..13eebbff04 100644 --- a/openjdkjvmti/ti_thread.cc +++ b/openjdkjvmti/ti_thread.cc @@ -289,9 +289,7 @@ jvmtiError ThreadUtil::GetThreadInfo(jvmtiEnv* env, jthread thread, jvmtiThreadI info_ptr->is_daemon = target->IsDaemon(); - art::ObjPtr peer = target->LockedGetPeerFromOtherThread(); - // *target may be invalid here since we may have temporarily released thread_list_lock_. - target = nullptr; // Value should not be used. + art::ObjPtr peer = target->GetPeerFromOtherThread(); // ThreadGroup. if (peer != nullptr) { @@ -549,7 +547,6 @@ static jint GetJavaStateFromInternal(const InternalThreadState& state) { // Suspends the current thread if it has any suspend requests on it. void ThreadUtil::SuspendCheck(art::Thread* self) { - DCHECK(!self->ReadFlag(art::ThreadFlag::kSuspensionImmune)); art::ScopedObjectAccess soa(self); // Really this is only needed if we are in FastJNI and actually have the mutator_lock_ already. self->FullSuspendCheck(); @@ -643,30 +640,20 @@ jvmtiError ThreadUtil::GetAllThreads(jvmtiEnv* env, art::MutexLock mu(current, *art::Locks::thread_list_lock_); std::list thread_list = art::Runtime::Current()->GetThreadList()->GetList(); - // We have to be careful with threads exiting while we build this list. - std::vector tefs(thread_list.size()); - auto i = tefs.begin(); - for (art::Thread* thd : thread_list) { - thd->NotifyOnThreadExit(&*i++); - } - DCHECK(i == tefs.end()); std::vector> peers; - i = tefs.begin(); for (art::Thread* thread : thread_list) { - art::ThreadExitFlag* tef = &*i++; - // Skip threads that have since exited or are still starting. - if (!tef->HasExited() && !thread->IsStillStarting()) { - // LockedGetPeerFromOtherThreads() may release lock! - art::ObjPtr peer = thread->LockedGetPeerFromOtherThread(tef); - if (peer != nullptr) { - peers.push_back(peer); - } + // Skip threads that are still starting. + if (thread->IsStillStarting()) { + continue; + } + + art::ObjPtr peer = thread->GetPeerFromOtherThread(); + if (peer != nullptr) { + peers.push_back(peer); } - thread->UnregisterThreadExitFlag(tef); } - DCHECK(i == tefs.end()); if (peers.empty()) { *threads_count_ptr = 0; @@ -678,8 +665,8 @@ jvmtiError ThreadUtil::GetAllThreads(jvmtiEnv* env, return data_result; } jthread* threads = reinterpret_cast(data); - for (size_t j = 0; j != peers.size(); ++j) { - threads[j] = soa.AddLocalReference(peers[j]); + for (size_t i = 0; i != peers.size(); ++i) { + threads[i] = soa.AddLocalReference(peers[i]); } *threads_count_ptr = static_cast(peers.size()); @@ -913,13 +900,17 @@ jvmtiError ThreadUtil::SuspendOther(art::Thread* self, } } } + bool timeout = true; art::Thread* ret_target = art::Runtime::Current()->GetThreadList()->SuspendThreadByPeer( - target_jthread, art::SuspendReason::kForUserCode); - if (ret_target == nullptr) { + target_jthread, + art::SuspendReason::kForUserCode, + &timeout); + if (ret_target == nullptr && !timeout) { // TODO It would be good to get more information about why exactly the thread failed to // suspend. return ERR(INTERNAL); - } else { + } else if (!timeout) { + // we didn't time out and got a result. return OK; } // We timed out. Just go around and try again. @@ -936,11 +927,10 @@ jvmtiError ThreadUtil::SuspendSelf(art::Thread* self) { // This can only happen if we race with another thread to suspend 'self' and we lose. return ERR(THREAD_SUSPENDED); } - { - // IncrementSuspendCount normally needs thread_list_lock_ to ensure the thread stays - // around. In this case we are the target thread, so we fake it. - art::FakeMutexLock fmu(*art::Locks::thread_list_lock_); - self->IncrementSuspendCount(self, nullptr, nullptr, art::SuspendReason::kForUserCode); + // We shouldn't be able to fail this. + if (!self->ModifySuspendCount(self, +1, nullptr, art::SuspendReason::kForUserCode)) { + // TODO More specific error would be nice. + return ERR(INTERNAL); } } // Once we have requested the suspend we actually go to sleep. We need to do this after releasing diff --git a/openjdkjvmti/ti_threadgroup.cc b/openjdkjvmti/ti_threadgroup.cc index bfe6b51791..120024e8b2 100644 --- a/openjdkjvmti/ti_threadgroup.cc +++ b/openjdkjvmti/ti_threadgroup.cc @@ -171,26 +171,18 @@ static void GetThreads(art::Handle thread_group, CHECK(thread_group != nullptr); art::MutexLock mu(art::Thread::Current(), *art::Locks::thread_list_lock_); - std::list thread_list = art::Runtime::Current()->GetThreadList()->GetList(); - // We have to be careful with threads exiting while we build this list. - std::vector tefs(thread_list.size()); - auto i = tefs.begin(); - for (art::Thread* thd : thread_list) { - thd->NotifyOnThreadExit(&*i++); - } - DCHECK(i == tefs.end()); - - i = tefs.begin(); - for (art::Thread* t : thread_list) { - art::ThreadExitFlag* tef = &*i++; - art::ObjPtr peer = t->LockedGetPeerFromOtherThread(tef); - if (peer != nullptr && !tef->HasExited() && !t->IsStillStarting() && - IsInDesiredThreadGroup(thread_group, peer)) { + for (art::Thread* t : art::Runtime::Current()->GetThreadList()->GetList()) { + if (t->IsStillStarting()) { + continue; + } + art::ObjPtr peer = t->GetPeerFromOtherThread(); + if (peer == nullptr) { + continue; + } + if (IsInDesiredThreadGroup(thread_group, peer)) { thread_peers->push_back(peer); } - t->UnregisterThreadExitFlag(tef); } - DCHECK(i == tefs.end()); } static void GetChildThreadGroups(art::Handle thread_group, -- cgit v1.2.3-59-g8ed1b