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_stack.cc | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) (limited to 'openjdkjvmti/ti_stack.cc') 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; -- cgit v1.2.3-59-g8ed1b