From 35b456a05af864da6580dd01c033efb0ed3e3e44 Mon Sep 17 00:00:00 2001 From: Hiroshi Yamauchi Date: Wed, 22 Feb 2017 18:06:55 -0800 Subject: [PATCH] Add jdwp event_list_lock_ to expected_mutexes_on_weak_ref_access_. To avoid a DCHECK failure where event_list_lock_ is unexpectedly held during a weak ref access. Also move event_list_lock to Locks. Bug: 35360959 Test: test-art-host. Test: jdwp test. Change-Id: I6315e1f7152058656f2479ad7b4e4f3defd15555 --- runtime/base/mutex.cc | 8 ++++++++ runtime/base/mutex.h | 6 +++++- runtime/jdwp/jdwp.h | 42 +++++++++++++++++++++++------------------- runtime/jdwp/jdwp_event.cc | 22 +++++++++++----------- runtime/jdwp/jdwp_main.cc | 3 +-- 5 files changed, 48 insertions(+), 33 deletions(-) diff --git a/runtime/base/mutex.cc b/runtime/base/mutex.cc index b93b29343..24846e5ce 100644 --- a/runtime/base/mutex.cc +++ b/runtime/base/mutex.cc @@ -46,6 +46,7 @@ Mutex* Locks::deoptimization_lock_ = nullptr; ReaderWriterMutex* Locks::heap_bitmap_lock_ = nullptr; Mutex* Locks::instrument_entrypoints_lock_ = nullptr; Mutex* Locks::intern_table_lock_ = nullptr; +Mutex* Locks::jdwp_event_list_lock_ = nullptr; Mutex* Locks::jni_function_table_lock_ = nullptr; Mutex* Locks::jni_libraries_lock_ = nullptr; Mutex* Locks::logging_lock_ = nullptr; @@ -998,6 +999,7 @@ void Locks::Init() { DCHECK(verifier_deps_lock_ != nullptr); DCHECK(host_dlopen_handles_lock_ != nullptr); DCHECK(intern_table_lock_ != nullptr); + DCHECK(jdwp_event_list_lock_ != nullptr); DCHECK(jni_function_table_lock_ != nullptr); DCHECK(jni_libraries_lock_ != nullptr); DCHECK(logging_lock_ != nullptr); @@ -1040,6 +1042,10 @@ void Locks::Init() { DCHECK(runtime_shutdown_lock_ == nullptr); runtime_shutdown_lock_ = new Mutex("runtime shutdown lock", current_lock_level); + UPDATE_CURRENT_LOCK_LEVEL(kJdwpEventListLock); + DCHECK(jdwp_event_list_lock_ == nullptr); + jdwp_event_list_lock_ = new Mutex("JDWP event list lock", current_lock_level); + UPDATE_CURRENT_LOCK_LEVEL(kProfilerLock); DCHECK(profiler_lock_ == nullptr); profiler_lock_ = new Mutex("profiler lock", current_lock_level); @@ -1167,6 +1173,8 @@ void Locks::Init() { expected_mutexes_on_weak_ref_access_.push_back(dex_lock_); classlinker_classes_lock_->SetShouldRespondToEmptyCheckpointRequest(true); expected_mutexes_on_weak_ref_access_.push_back(classlinker_classes_lock_); + jdwp_event_list_lock_->SetShouldRespondToEmptyCheckpointRequest(true); + expected_mutexes_on_weak_ref_access_.push_back(jdwp_event_list_lock_); jni_libraries_lock_->SetShouldRespondToEmptyCheckpointRequest(true); expected_mutexes_on_weak_ref_access_.push_back(jni_libraries_lock_); diff --git a/runtime/base/mutex.h b/runtime/base/mutex.h index 9b6938f9b..c59664b9c 100644 --- a/runtime/base/mutex.h +++ b/runtime/base/mutex.h @@ -630,8 +630,12 @@ class Locks { // Guards shutdown of the runtime. static Mutex* runtime_shutdown_lock_ ACQUIRED_AFTER(heap_bitmap_lock_); + static Mutex* jdwp_event_list_lock_ + ACQUIRED_AFTER(runtime_shutdown_lock_) + ACQUIRED_BEFORE(breakpoint_lock_); + // Guards background profiler global state. - static Mutex* profiler_lock_ ACQUIRED_AFTER(runtime_shutdown_lock_); + static Mutex* profiler_lock_ ACQUIRED_AFTER(jdwp_event_list_lock_); // Guards trace (ie traceview) requests. static Mutex* trace_lock_ ACQUIRED_AFTER(profiler_lock_); diff --git a/runtime/jdwp/jdwp.h b/runtime/jdwp/jdwp.h index 86af6d44d..af2946806 100644 --- a/runtime/jdwp/jdwp.h +++ b/runtime/jdwp/jdwp.h @@ -203,7 +203,8 @@ struct JdwpState { */ void PostLocationEvent(const EventLocation* pLoc, mirror::Object* thisPtr, int eventFlags, const JValue* returnValue) - REQUIRES(!event_list_lock_, !jdwp_token_lock_) REQUIRES_SHARED(Locks::mutator_lock_); + REQUIRES(!Locks::jdwp_event_list_lock_, !jdwp_token_lock_) + REQUIRES_SHARED(Locks::mutator_lock_); /* * A field of interest has been accessed or modified. This is used for field access and field @@ -214,7 +215,8 @@ struct JdwpState { */ void PostFieldEvent(const EventLocation* pLoc, ArtField* field, mirror::Object* thisPtr, const JValue* fieldValue, bool is_modification) - REQUIRES(!event_list_lock_, !jdwp_token_lock_) REQUIRES_SHARED(Locks::mutator_lock_); + REQUIRES(!Locks::jdwp_event_list_lock_, !jdwp_token_lock_) + REQUIRES_SHARED(Locks::mutator_lock_); /* * An exception has been thrown. @@ -223,19 +225,22 @@ struct JdwpState { */ void PostException(const EventLocation* pThrowLoc, mirror::Throwable* exception_object, const EventLocation* pCatchLoc, mirror::Object* thisPtr) - REQUIRES(!event_list_lock_, !jdwp_token_lock_) REQUIRES_SHARED(Locks::mutator_lock_); + REQUIRES(!Locks::jdwp_event_list_lock_, !jdwp_token_lock_) + REQUIRES_SHARED(Locks::mutator_lock_); /* * A thread has started or stopped. */ void PostThreadChange(Thread* thread, bool start) - REQUIRES(!event_list_lock_, !jdwp_token_lock_) REQUIRES_SHARED(Locks::mutator_lock_); + REQUIRES(!Locks::jdwp_event_list_lock_, !jdwp_token_lock_) + REQUIRES_SHARED(Locks::mutator_lock_); /* * Class has been prepared. */ void PostClassPrepare(mirror::Class* klass) - REQUIRES(!event_list_lock_, !jdwp_token_lock_) REQUIRES_SHARED(Locks::mutator_lock_); + REQUIRES(!Locks::jdwp_event_list_lock_, !jdwp_token_lock_) + REQUIRES_SHARED(Locks::mutator_lock_); /* * The VM is about to stop. @@ -259,7 +264,7 @@ struct JdwpState { void SendRequest(ExpandBuf* pReq); void ResetState() - REQUIRES(!event_list_lock_) + REQUIRES(!Locks::jdwp_event_list_lock_) REQUIRES_SHARED(Locks::mutator_lock_); /* atomic ops to get next serial number */ @@ -268,7 +273,7 @@ struct JdwpState { void Run() REQUIRES(!Locks::mutator_lock_, !Locks::thread_suspend_count_lock_, !thread_start_lock_, - !attach_lock_, !event_list_lock_); + !attach_lock_, !Locks::jdwp_event_list_lock_); /* * Register an event by adding it to the event list. @@ -277,25 +282,25 @@ struct JdwpState { * may discard its pointer after calling this. */ JdwpError RegisterEvent(JdwpEvent* pEvent) - REQUIRES(!event_list_lock_) + REQUIRES(!Locks::jdwp_event_list_lock_) REQUIRES_SHARED(Locks::mutator_lock_); /* * Unregister an event, given the requestId. */ void UnregisterEventById(uint32_t requestId) - REQUIRES(!event_list_lock_) + REQUIRES(!Locks::jdwp_event_list_lock_) REQUIRES_SHARED(Locks::mutator_lock_); void UnregisterLocationEventsOnClass(ObjPtr klass) - REQUIRES(!event_list_lock_) + REQUIRES(!Locks::jdwp_event_list_lock_) REQUIRES_SHARED(Locks::mutator_lock_); /* * Unregister all events. */ void UnregisterAll() - REQUIRES(!event_list_lock_) + REQUIRES(!Locks::jdwp_event_list_lock_) REQUIRES_SHARED(Locks::mutator_lock_); private: @@ -310,16 +315,16 @@ struct JdwpState { ObjectId threadId) REQUIRES_SHARED(Locks::mutator_lock_) REQUIRES(!jdwp_token_lock_); void CleanupMatchList(const std::vector& match_list) - REQUIRES(event_list_lock_) REQUIRES_SHARED(Locks::mutator_lock_); + REQUIRES(Locks::jdwp_event_list_lock_) REQUIRES_SHARED(Locks::mutator_lock_); void EventFinish(ExpandBuf* pReq); bool FindMatchingEvents(JdwpEventKind eventKind, const ModBasket& basket, std::vector* match_list) - REQUIRES(!event_list_lock_) REQUIRES_SHARED(Locks::mutator_lock_); + REQUIRES(!Locks::jdwp_event_list_lock_) REQUIRES_SHARED(Locks::mutator_lock_); void FindMatchingEventsLocked(JdwpEventKind eventKind, const ModBasket& basket, std::vector* match_list) - REQUIRES(event_list_lock_) REQUIRES_SHARED(Locks::mutator_lock_); + REQUIRES(Locks::jdwp_event_list_lock_) REQUIRES_SHARED(Locks::mutator_lock_); void UnregisterEvent(JdwpEvent* pEvent) - REQUIRES(event_list_lock_) REQUIRES_SHARED(Locks::mutator_lock_); + REQUIRES(Locks::jdwp_event_list_lock_) REQUIRES_SHARED(Locks::mutator_lock_); void SendBufferedRequest(uint32_t type, const std::vector& iov); /* @@ -387,9 +392,8 @@ struct JdwpState { AtomicInteger event_serial_; // Linked list of events requested by the debugger (breakpoints, class prep, etc). - Mutex event_list_lock_ DEFAULT_MUTEX_ACQUIRED_AFTER ACQUIRED_BEFORE(Locks::breakpoint_lock_); - JdwpEvent* event_list_ GUARDED_BY(event_list_lock_); - size_t event_list_size_ GUARDED_BY(event_list_lock_); // Number of elements in event_list_. + JdwpEvent* event_list_ GUARDED_BY(Locks::jdwp_event_list_lock_); + size_t event_list_size_ GUARDED_BY(Locks::jdwp_event_list_lock_); // Number of elements in event_list_. // Used to synchronize JDWP command handler thread and event threads so only one // thread does JDWP stuff at a time. This prevent from interleaving command handling @@ -410,7 +414,7 @@ struct JdwpState { // When the runtime shuts down, it needs to stop JDWP command handler thread by closing the // JDWP connection. However, if the JDWP thread is processing a command, it needs to wait // for the command to finish so we can send its reply before closing the connection. - Mutex shutdown_lock_ ACQUIRED_AFTER(event_list_lock_); + Mutex shutdown_lock_ ACQUIRED_AFTER(Locks::jdwp_event_list_lock_); ConditionVariable shutdown_cond_ GUARDED_BY(shutdown_lock_); bool processing_request_ GUARDED_BY(shutdown_lock_); }; diff --git a/runtime/jdwp/jdwp_event.cc b/runtime/jdwp/jdwp_event.cc index 96249f9b5..36d733ea0 100644 --- a/runtime/jdwp/jdwp_event.cc +++ b/runtime/jdwp/jdwp_event.cc @@ -237,7 +237,7 @@ JdwpError JdwpState::RegisterEvent(JdwpEvent* pEvent) { /* * Add to list. */ - MutexLock mu(Thread::Current(), event_list_lock_); + MutexLock mu(Thread::Current(), *Locks::jdwp_event_list_lock_); if (event_list_ != nullptr) { pEvent->next = event_list_; event_list_->prev = pEvent; @@ -256,7 +256,7 @@ void JdwpState::UnregisterLocationEventsOnClass(ObjPtr klass) { StackHandleScope<1> hs(Thread::Current()); Handle h_klass(hs.NewHandle(klass)); std::vector to_remove; - MutexLock mu(Thread::Current(), event_list_lock_); + MutexLock mu(Thread::Current(), *Locks::jdwp_event_list_lock_); for (JdwpEvent* cur_event = event_list_; cur_event != nullptr; cur_event = cur_event->next) { // Fill in the to_remove list bool found_event = false; @@ -356,7 +356,7 @@ void JdwpState::UnregisterEvent(JdwpEvent* pEvent) { void JdwpState::UnregisterEventById(uint32_t requestId) { bool found = false; { - MutexLock mu(Thread::Current(), event_list_lock_); + MutexLock mu(Thread::Current(), *Locks::jdwp_event_list_lock_); for (JdwpEvent* pEvent = event_list_; pEvent != nullptr; pEvent = pEvent->next) { if (pEvent->requestId == requestId) { @@ -383,7 +383,7 @@ void JdwpState::UnregisterEventById(uint32_t requestId) { * Remove all entries from the event list. */ void JdwpState::UnregisterAll() { - MutexLock mu(Thread::Current(), event_list_lock_); + MutexLock mu(Thread::Current(), *Locks::jdwp_event_list_lock_); JdwpEvent* pEvent = event_list_; while (pEvent != nullptr) { @@ -593,7 +593,7 @@ void JdwpState::FindMatchingEventsLocked(JdwpEventKind event_kind, const ModBask */ bool JdwpState::FindMatchingEvents(JdwpEventKind event_kind, const ModBasket& basket, std::vector* match_list) { - MutexLock mu(Thread::Current(), event_list_lock_); + MutexLock mu(Thread::Current(), *Locks::jdwp_event_list_lock_); match_list->reserve(event_list_size_); FindMatchingEventsLocked(event_kind, basket, match_list); return !match_list->empty(); @@ -908,7 +908,7 @@ void JdwpState::PostLocationEvent(const EventLocation* pLoc, mirror::Object* thi std::vector match_list; { // We use the locked version because we have multiple possible match events. - MutexLock mu(Thread::Current(), event_list_lock_); + MutexLock mu(Thread::Current(), *Locks::jdwp_event_list_lock_); match_list.reserve(event_list_size_); if ((eventFlags & Dbg::kBreakpoint) != 0) { FindMatchingEventsLocked(EK_BREAKPOINT, basket, &match_list); @@ -955,7 +955,7 @@ void JdwpState::PostLocationEvent(const EventLocation* pLoc, mirror::Object* thi } { - MutexLock mu(Thread::Current(), event_list_lock_); + MutexLock mu(Thread::Current(), *Locks::jdwp_event_list_lock_); CleanupMatchList(match_list); } @@ -1041,7 +1041,7 @@ void JdwpState::PostFieldEvent(const EventLocation* pLoc, ArtField* field, } { - MutexLock mu(Thread::Current(), event_list_lock_); + MutexLock mu(Thread::Current(), *Locks::jdwp_event_list_lock_); CleanupMatchList(match_list); } @@ -1103,7 +1103,7 @@ void JdwpState::PostThreadChange(Thread* thread, bool start) { } { - MutexLock mu(Thread::Current(), event_list_lock_); + MutexLock mu(Thread::Current(), *Locks::jdwp_event_list_lock_); CleanupMatchList(match_list); } @@ -1213,7 +1213,7 @@ void JdwpState::PostException(const EventLocation* pThrowLoc, mirror::Throwable* } { - MutexLock mu(Thread::Current(), event_list_lock_); + MutexLock mu(Thread::Current(), *Locks::jdwp_event_list_lock_); CleanupMatchList(match_list); } @@ -1295,7 +1295,7 @@ void JdwpState::PostClassPrepare(mirror::Class* klass) { } { - MutexLock mu(Thread::Current(), event_list_lock_); + MutexLock mu(Thread::Current(), *Locks::jdwp_event_list_lock_); CleanupMatchList(match_list); } diff --git a/runtime/jdwp/jdwp_main.cc b/runtime/jdwp/jdwp_main.cc index 7707ba493..64ed724af 100644 --- a/runtime/jdwp/jdwp_main.cc +++ b/runtime/jdwp/jdwp_main.cc @@ -227,7 +227,6 @@ JdwpState::JdwpState(const JdwpOptions* options) last_activity_time_ms_(0), request_serial_(0x10000000), event_serial_(0x20000000), - event_list_lock_("JDWP event list lock", kJdwpEventListLock), event_list_(nullptr), event_list_size_(0), jdwp_token_lock_("JDWP token lock"), @@ -331,7 +330,7 @@ void JdwpState::ResetState() { UnregisterAll(); { - MutexLock mu(Thread::Current(), event_list_lock_); + MutexLock mu(Thread::Current(), *Locks::jdwp_event_list_lock_); CHECK(event_list_ == nullptr); } -- 2.11.0