[vm] Call OSThread::Cleanup() during VM shutdown (as with all other Init/Cleanup functions) Change-Id: I3cfa51714247a62fe39951636ca1a75c34f6c95b Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/149293 Reviewed-by: Ben Konyi <bkonyi@google.com> Commit-Queue: Martin Kustermann <kustermann@google.com>
diff --git a/runtime/vm/dart.cc b/runtime/vm/dart.cc index 06afb21..979bd2b 100644 --- a/runtime/vm/dart.cc +++ b/runtime/vm/dart.cc
@@ -617,12 +617,7 @@ Timeline::Cleanup(); #endif Zone::Cleanup(); - // Delete the current thread's TLS and set it's TLS to null. - // If it is the last thread then the destructor would call - // OSThread::Cleanup. - OSThread* os_thread = OSThread::Current(); - OSThread::SetCurrent(NULL); - delete os_thread; + OSThread::Cleanup(); if (FLAG_trace_shutdown) { OS::PrintErr("[+%" Pd64 "ms] SHUTDOWN: Deleted os_thread\n", UptimeMillis());
diff --git a/runtime/vm/malloc_hooks_test.cc b/runtime/vm/malloc_hooks_test.cc index 76b789b..6d93f9c 100644 --- a/runtime/vm/malloc_hooks_test.cc +++ b/runtime/vm/malloc_hooks_test.cc
@@ -25,10 +25,17 @@ } } +// Only to be used in UNIT_TEST_CASE which runs without active VM. +class OSThreadSupport : public ValueObject { + public: + OSThreadSupport() { OSThread::Init(); } + + ~OSThreadSupport() { OSThread::Cleanup(); } +}; + class EnableMallocHooksScope : public ValueObject { public: EnableMallocHooksScope() { - OSThread::Current(); // Ensure not allocated during test. saved_enable_malloc_hooks_ = FLAG_profiler_native_memory; FLAG_profiler_native_memory = true; MallocHooks::Init(); @@ -66,6 +73,7 @@ }; UNIT_TEST_CASE(BasicMallocHookTest) { + OSThreadSupport os_thread_support; EnableMallocHooksScope scope; EXPECT_EQ(0L, MallocHooks::allocation_count()); @@ -84,6 +92,7 @@ } UNIT_TEST_CASE(FreeUnseenMemoryMallocHookTest) { + OSThreadSupport os_thread_support; EnableMallocHooksScope scope; const intptr_t pre_hook_buffer_size = 3;
diff --git a/runtime/vm/os_thread.cc b/runtime/vm/os_thread.cc index 61b929c..fe0e8c7 100644 --- a/runtime/vm/os_thread.cc +++ b/runtime/vm/os_thread.cc
@@ -134,21 +134,18 @@ return thread_interrupt_disabled_ == 0; } -static void DeleteThread(void* thread) { +static void DeleteOSThreadTLS(void* thread) { delete reinterpret_cast<OSThread*>(thread); } void OSThread::Init() { // Allocate the global OSThread lock. - if (thread_list_lock_ == NULL) { - thread_list_lock_ = new Mutex(); - } - ASSERT(thread_list_lock_ != NULL); + ASSERT(thread_list_lock_ == nullptr); + thread_list_lock_ = new Mutex(); // Create the thread local key. - if (thread_key_ == kUnsetThreadLocalKey) { - thread_key_ = CreateThreadLocal(DeleteThread); - } + ASSERT(thread_key_ == kUnsetThreadLocalKey); + thread_key_ = CreateThreadLocal(DeleteOSThreadTLS); ASSERT(thread_key_ != kUnsetThreadLocalKey); // Enable creation of OSThread structures in the VM. @@ -162,21 +159,25 @@ } void OSThread::Cleanup() { -// We cannot delete the thread local key and thread list lock, yet. -// See the note on thread_list_lock_ in os_thread.h. -#if 0 - if (thread_list_lock_ != NULL) { - // Delete the thread local key. - ASSERT(thread_key_ != kUnsetThreadLocalKey); - DeleteThreadLocal(thread_key_); - thread_key_ = kUnsetThreadLocalKey; + // Delete the current thread's TLS (if any). + OSThread* os_thread = OSThread::Current(); + OSThread::SetCurrent(nullptr); + delete os_thread; - // Delete the global OSThread lock. - ASSERT(thread_list_lock_ != NULL); - delete thread_list_lock_; - thread_list_lock_ = NULL; - } -#endif + // At this point all OSThread structures should have been deleted. + // If not we have a bug in the code where a thread is not correctly joined + // before `Dart::Cleanup()`. + RELEASE_ASSERT(OSThread::thread_list_head_ == nullptr); + + // Delete the thread local key. + ASSERT(thread_key_ != kUnsetThreadLocalKey); + DeleteThreadLocal(thread_key_); + thread_key_ = kUnsetThreadLocalKey; + + // Delete the global OSThread lock. + ASSERT(thread_list_lock_ != nullptr); + delete thread_list_lock_; + thread_list_lock_ = nullptr; } OSThread* OSThread::CreateAndSetUnknownThread() { @@ -245,7 +246,6 @@ } void OSThread::RemoveThreadFromList(OSThread* thread) { - bool final_thread = false; { ASSERT(thread != NULL); ASSERT(thread_list_lock_ != NULL); @@ -263,18 +263,12 @@ previous->thread_list_next_ = current->thread_list_next_; } thread->thread_list_next_ = NULL; - final_thread = !creation_enabled_ && (thread_list_head_ == NULL); break; } previous = current; current = current->thread_list_next_; } } - // Check if this is the last thread. The last thread does a cleanup - // which removes the thread local key and the associated mutex. - if (final_thread) { - Cleanup(); - } } void OSThread::SetCurrentTLS(BaseThread* value) {
diff --git a/runtime/vm/os_thread.h b/runtime/vm/os_thread.h index c35d68f..fb40b70 100644 --- a/runtime/vm/os_thread.h +++ b/runtime/vm/os_thread.h
@@ -229,6 +229,7 @@ // Called at VM startup and shutdown. static void Init(); + static void Cleanup(); static bool IsThreadInList(ThreadId id); @@ -255,7 +256,6 @@ ThreadState* thread() const { return thread_; } void set_thread(ThreadState* value) { thread_ = value; } - static void Cleanup(); #ifdef SUPPORT_TIMELINE static ThreadId GetCurrentThreadTraceId(); #endif // PRODUCT @@ -299,11 +299,8 @@ // protected and should only be read/written by the OSThread itself. void* owning_thread_pool_worker_ = nullptr; - // thread_list_lock_ cannot have a static lifetime because the order in which - // destructors run is undefined. At the moment this lock cannot be deleted - // either since otherwise, if a thread only begins to run after we have - // started to run TLS destructors for a call to exit(), there will be a race - // on its deletion in CreateOSThread(). + // [thread_list_lock_] cannot have a static lifetime because the order in + // which destructors run is undefined. static Mutex* thread_list_lock_; static OSThread* thread_list_head_; static bool creation_enabled_;
diff --git a/runtime/vm/thread_pool_test.cc b/runtime/vm/thread_pool_test.cc index 1dcf3df..35eaa36 100644 --- a/runtime/vm/thread_pool_test.cc +++ b/runtime/vm/thread_pool_test.cc
@@ -19,12 +19,7 @@ UNIT_TEST_CASE(name) { \ OSThread::Init(); \ name##helper(); \ - /* Delete the current thread's TLS and set it's TLS to null. */ \ - /* If it is the last thread then the destructor would call */ \ - /* OSThread::Cleanup. */ \ - OSThread* os_thread = OSThread::Current(); \ - OSThread::SetCurrent(nullptr); \ - delete os_thread; \ + OSThread::Cleanup(); \ } \ void name##helper()