From 894a5fa7d5cd5f2a66035fa91cfabe4ea3bb394e Mon Sep 17 00:00:00 2001 From: Ryan Macnak Date: Wed, 7 Jan 2026 12:47:21 -0800 Subject: [PATCH] [vm, mac] Disable TSAN during profiler sample collection. The suspended thread might hold a TSAN-internal lock to a location that will be read by the sampling thread, which would cause a dead lock. TEST=tsan Bug: https://github.com/dart-lang/sdk/issues/62332 Change-Id: I0ac6a05c25067749b52ed83b14f80ab1850013a3 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/471321 Commit-Queue: Ryan Macnak Reviewed-by: Alexander Aprelev --- runtime/platform/thread_sanitizer.h | 21 ++++++++++++ runtime/vm/profiler.cc | 46 ++++++++------------------ runtime/vm/profiler_test.cc | 14 ++++++++ runtime/vm/thread.cc | 10 ------ runtime/vm/thread.h | 12 ------- runtime/vm/thread_interrupter_macos.cc | 5 +++ runtime/vm/thread_state.h | 2 -- 7 files changed, 54 insertions(+), 56 deletions(-) diff --git a/runtime/platform/thread_sanitizer.h b/runtime/platform/thread_sanitizer.h index 0d9b423a15f..4c745ebec60 100644 --- a/runtime/platform/thread_sanitizer.h +++ b/runtime/platform/thread_sanitizer.h @@ -5,6 +5,7 @@ #ifndef RUNTIME_PLATFORM_THREAD_SANITIZER_H_ #define RUNTIME_PLATFORM_THREAD_SANITIZER_H_ +#include "platform/allocation.h" #include "platform/globals.h" #if __SANITIZE_THREAD__ @@ -54,6 +55,8 @@ extern "C" void __tsan_write8_pc(void* addr, void* pc); extern "C" void __tsan_write16_pc(void* addr, void* pc); extern "C" void __tsan_func_entry(void* pc); extern "C" void __tsan_func_exit(); +extern "C" void __tsan_ignore_thread_begin(); +extern "C" void __tsan_ignore_thread_end(); constexpr uintptr_t kExternalPCBit = 1ULL << 60; #else #define NO_SANITIZE_THREAD @@ -72,4 +75,22 @@ constexpr uintptr_t kExternalPCBit = 1ULL << 60; #define DO_IF_NOT_TSAN(CODE) CODE #endif +namespace dart { + +class TsanIgnoreScope : public ValueObject { + public: + TsanIgnoreScope() { +#if defined(USING_THREAD_SANITIZER) + __tsan_ignore_thread_begin(); +#endif + } + ~TsanIgnoreScope() { +#if defined(USING_THREAD_SANITIZER) + __tsan_ignore_thread_end(); +#endif + } +}; + +} // namespace dart + #endif // RUNTIME_PLATFORM_THREAD_SANITIZER_H_ diff --git a/runtime/vm/profiler.cc b/runtime/vm/profiler.cc index e388bbbd392..670d34958ff 100644 --- a/runtime/vm/profiler.cc +++ b/runtime/vm/profiler.cc @@ -199,25 +199,10 @@ class ProfilerStackWalker : public ValueObject { // MSAN/ASAN are unaware of frames initialized by generated code. NO_SANITIZE_ADDRESS NO_SANITIZE_MEMORY -#if defined(DART_HOST_OS_MACOS) -// Mac profiling is cross-thread and TSAN doesn't know that thread_suspend -// establishes synchronization. -NO_SANITIZE_THREAD -#endif static uword* LoadStackSlot(uword* ptr) { return reinterpret_cast(*ptr); } -#if defined(DART_HOST_OS_MACOS) -// Mac profiling is cross-thread and TSAN doesn't know that thread_suspend -// establishes synchronization. -#define IGNORE_RACE(x) x##_ignore_race -#define IGNORE_RACE2(x) x##IgnoreRace -#else -#define IGNORE_RACE(x) x -#define IGNORE_RACE2(x) x -#endif - // The layout of C stack frames. #if defined(HOST_ARCH_IA32) || defined(HOST_ARCH_X64) || \ defined(HOST_ARCH_ARM) || defined(HOST_ARCH_ARM64) @@ -388,8 +373,7 @@ static bool GetAndValidateThreadStackBounds(OSThread* os_thread, #if defined(DART_INCLUDE_SIMULATOR) const bool use_simulator_stack_bounds = - FLAG_use_simulator && thread != nullptr && - thread->IGNORE_RACE2(IsExecutingDartCode)(); + FLAG_use_simulator && thread != nullptr && thread->IsExecutingDartCode(); if (use_simulator_stack_bounds) { Isolate* isolate = thread->isolate(); ASSERT(isolate != nullptr); @@ -1073,13 +1057,12 @@ class ProfilerDartStackWalker : public ProfilerStackWalker { void walk() { RELEASE_ASSERT(StubCode::HasBeenInitialized()); - if (thread_->IGNORE_RACE2(IsDeoptimizing)()) { + if (thread_->IsDeoptimizing()) { sample_->set_ignore_sample(true); return; } - uword* exit_fp = - reinterpret_cast(thread_->IGNORE_RACE(top_exit_frame_info)()); + uword* exit_fp = reinterpret_cast(thread_->top_exit_frame_info()); bool has_exit_frame = exit_fp != nullptr; if (has_exit_frame) { // Exited from compiled code or interpreter. @@ -1090,14 +1073,13 @@ class ProfilerDartStackWalker : public ProfilerStackWalker { pc_ = CallerPC(); fp_ = CallerFP(); } else { - if (thread_->IGNORE_RACE(vm_tag)() == VMTag::kDartTagId) { + if (thread_->vm_tag() == VMTag::kDartTagId) { // Running compiled code. // Use the FP and PC from the thread interrupt or simulator; already set // in the constructor. #if defined(DART_DYNAMIC_MODULES) - } else if (thread_->IGNORE_RACE(vm_tag)() == - VMTag::kDartInterpretedTagId) { + } else if (thread_->vm_tag() == VMTag::kDartInterpretedTagId) { // Running interpreter. pc_ = reinterpret_cast(thread_->interpreter()->get_pc()); fp_ = reinterpret_cast(thread_->interpreter()->get_fp()); @@ -1300,7 +1282,7 @@ static Sample* SetupSample(Thread* thread, bool allocation_sample, ThreadId tid) { ASSERT(thread != nullptr); - Isolate* isolate = thread->IGNORE_RACE(isolate)(); + Isolate* isolate = thread->isolate(); SampleBlockBuffer* buffer = Profiler::sample_block_buffer(); Sample* sample = allocation_sample ? buffer->ReserveAllocationSample(isolate) : buffer->ReserveCPUSample(isolate); @@ -1308,7 +1290,7 @@ static Sample* SetupSample(Thread* thread, return nullptr; } sample->Init(isolate->main_port(), OS::GetCurrentMonotonicMicros(), tid); - uword vm_tag = thread->IGNORE_RACE(vm_tag)(); + uword vm_tag = thread->vm_tag(); #if defined(DART_INCLUDE_SIMULATOR) // When running in the simulator, the runtime entry function address // (stored as the vm tag) is the address of a redirect function. @@ -1321,7 +1303,7 @@ static Sample* SetupSample(Thread* thread, } #endif sample->set_vm_tag(vm_tag); - sample->set_user_tag(thread->IGNORE_RACE(user_tag)()); + sample->set_user_tag(thread->user_tag()); sample->set_thread_task(thread->task_kind()); return sample; } @@ -1405,7 +1387,7 @@ void Profiler::SampleThreadSingleFrame(Thread* thread, ASSERT(Profiler::sample_block_buffer() != nullptr); #if !defined(PRODUCT) - Isolate* isolate = thread->IGNORE_RACE(isolate)(); + Isolate* isolate = thread->isolate(); // Increment counter for vm tag. VMTagCounters* counters = isolate->vm_tag_counters(); @@ -1441,9 +1423,9 @@ void ReleaseToCurrentBlock(Isolate* isolate) { void Profiler::SampleThread(Thread* thread, const InterruptedThreadState& state) { ASSERT(thread != nullptr); - OSThread* os_thread = thread->IGNORE_RACE(os_thread)(); + OSThread* os_thread = thread->os_thread(); ASSERT(os_thread != nullptr); - Isolate* isolate = thread->IGNORE_RACE(isolate)(); + Isolate* isolate = thread->isolate(); // Double check if interrupts are disabled // after the thread interrupter decided to send a signal. @@ -1465,7 +1447,7 @@ void Profiler::SampleThread(Thread* thread, return; } - const bool in_dart_code = thread->IGNORE_RACE2(IsExecutingDartCode)(); + const bool in_dart_code = thread->IsExecutingDartCode(); uintptr_t sp = 0; uintptr_t fp = state.fp; @@ -1513,7 +1495,7 @@ void Profiler::SampleThread(Thread* thread, } if (thread->IsDartMutatorThread()) { - if (thread->IGNORE_RACE2(IsDeoptimizing)()) { + if (thread->IsDeoptimizing()) { counters_.single_frame_sample_deoptimizing.fetch_add(1); SampleThreadSingleFrame(thread, sample, pc); ReleaseToCurrentBlock(isolate); @@ -1547,7 +1529,7 @@ void Profiler::SampleThread(Thread* thread, Dart_Port port = (isolate != nullptr) ? isolate->main_port() : ILLEGAL_PORT; ProfilerNativeStackWalker native_stack_walker( &counters_, port, sample, isolate, stack_lower, stack_upper, pc, fp, sp); - const bool exited_dart_code = thread->IGNORE_RACE2(HasExitedDartCode)(); + const bool exited_dart_code = thread->HasExitedDartCode(); ProfilerDartStackWalker dart_stack_walker(thread, port, sample, isolate, pc, fp, sp, lr, /*allocation_sample=*/false); diff --git a/runtime/vm/profiler_test.cc b/runtime/vm/profiler_test.cc index de90bd6938b..c78ddf6d768 100644 --- a/runtime/vm/profiler_test.cc +++ b/runtime/vm/profiler_test.cc @@ -2461,6 +2461,20 @@ ISOLATE_UNIT_TEST_CASE(Profiler_EnterExitIsolate) { } } +// Poke a lot at OSThread::thread_interrupt_disabled_, which will be read by the +// sampling thread. +// https://github.com/dart-lang/sdk/issues/62332 +ISOLATE_UNIT_TEST_CASE(Profiler_ThreadEnableDisableProfiler) { + EnableProfiler(); + Profiler::UpdateFlagProfilePeriod(50); // Microseconds. + Profiler::UpdateSamplePeriod(); + + for (intptr_t i = 0; i < 100000; i++) { + Dart_ThreadDisableProfiling(); + Dart_ThreadEnableProfiling(); + } +} + ISOLATE_UNIT_TEST_CASE(Profiler_UpdateRunningState) { Isolate* isolate = Isolate::Current(); SampleFilter filter(isolate->main_port(), Thread::kMutatorTask, -1, -1); diff --git a/runtime/vm/thread.cc b/runtime/vm/thread.cc index 9b4f26a6201..844f7b44996 100644 --- a/runtime/vm/thread.cc +++ b/runtime/vm/thread.cc @@ -1056,20 +1056,10 @@ bool Thread::IsExecutingDartCode() const { return (top_exit_frame_info() == 0) && VMTag::IsDartTag(vm_tag()); } -bool Thread::IsExecutingDartCodeIgnoreRace() const { - return (top_exit_frame_info_ignore_race() == 0) && - VMTag::IsDartTag(vm_tag_ignore_race()); -} - bool Thread::HasExitedDartCode() const { return (top_exit_frame_info() != 0) && !VMTag::IsDartTag(vm_tag()); } -bool Thread::HasExitedDartCodeIgnoreRace() const { - return (top_exit_frame_info_ignore_race() != 0) && - !VMTag::IsDartTag(vm_tag_ignore_race()); -} - template C* Thread::AllocateReusableHandle() { C* handle = reinterpret_cast(reusable_handles_.AllocateScopedHandle()); diff --git a/runtime/vm/thread.h b/runtime/vm/thread.h index d5b9f8588a9..5222b37cbb3 100644 --- a/runtime/vm/thread.h +++ b/runtime/vm/thread.h @@ -580,8 +580,6 @@ class Thread : public ThreadState, public IntrusiveDListEntry { // The isolate that this thread is operating on, or nullptr if none. Isolate* isolate() const { return isolate_; } - NO_SANITIZE_THREAD - Isolate* isolate_ignore_race() const { return isolate_; } static intptr_t isolate_offset() { return OFFSET_OF(Thread, isolate_); } static intptr_t isolate_group_offset() { return OFFSET_OF(Thread, isolate_group_); @@ -637,11 +635,9 @@ class Thread : public ThreadState, public IntrusiveDListEntry { // Is |this| executing Dart code? bool IsExecutingDartCode() const; - bool IsExecutingDartCodeIgnoreRace() const; // Has |this| exited Dart code? bool HasExitedDartCode() const; - bool HasExitedDartCodeIgnoreRace() const; bool HasCompilerState() const { return compiler_state_ != nullptr; } @@ -749,8 +745,6 @@ class Thread : public ThreadState, public IntrusiveDListEntry { } uword top_exit_frame_info() const { return top_exit_frame_info_; } - NO_SANITIZE_THREAD - uword top_exit_frame_info_ignore_race() const { return top_exit_frame_info_; } void set_top_exit_frame_info(uword top_exit_frame_info) { top_exit_frame_info_ = top_exit_frame_info; } @@ -881,8 +875,6 @@ class Thread : public ThreadState, public IntrusiveDListEntry { #endif uword vm_tag() const { return vm_tag_; } - NO_SANITIZE_THREAD - uword vm_tag_ignore_race() const { return vm_tag_; } void set_vm_tag(uword tag) { vm_tag_ = tag; } static intptr_t vm_tag_offset() { return OFFSET_OF(Thread, vm_tag_); } @@ -1352,8 +1344,6 @@ class Thread : public ThreadState, public IntrusiveDListEntry { } bool IsDeoptimizing() const { return deopt_context_ != nullptr; } - NO_SANITIZE_THREAD - bool IsDeoptimizingIgnoreRace() const { return deopt_context_ != nullptr; } DeoptContext* deopt_context() const { return deopt_context_; } void set_deopt_context(DeoptContext* value) { ASSERT(value == nullptr || deopt_context_ == nullptr); @@ -1372,8 +1362,6 @@ class Thread : public ThreadState, public IntrusiveDListEntry { } uword user_tag() const { return user_tag_; } - NO_SANITIZE_THREAD - uword user_tag_ignore_race() const { return user_tag_; } static intptr_t user_tag_offset() { return OFFSET_OF(Thread, user_tag_); } static intptr_t current_tag_offset() { return OFFSET_OF(Thread, current_tag_); diff --git a/runtime/vm/thread_interrupter_macos.cc b/runtime/vm/thread_interrupter_macos.cc index c44e419b42b..85b51f9a3f3 100644 --- a/runtime/vm/thread_interrupter_macos.cc +++ b/runtime/vm/thread_interrupter_macos.cc @@ -76,6 +76,11 @@ class ThreadInterrupterMacOS { return; } ThreadInterruptScope signal_handler_scope; + // Mac profiling is cross-thread and TSAN doesn't know that thread_suspend + // establishes synchronization. Also, the suspended thread might hold a + // TSAN-internal lock for a location the sampling thread might read, which + // would cause a deadlock. + TsanIgnoreScope tsan_ignore_scope; Profiler::SampleThread(thread, ProcessState(state)); } diff --git a/runtime/vm/thread_state.h b/runtime/vm/thread_state.h index e41214e866f..8885a06f987 100644 --- a/runtime/vm/thread_state.h +++ b/runtime/vm/thread_state.h @@ -32,8 +32,6 @@ class ThreadState : public BaseThread { // OSThread corresponding to this thread. OSThread* os_thread() const { return os_thread_; } - NO_SANITIZE_THREAD - OSThread* os_thread_ignore_race() const { return os_thread_; } void set_os_thread(OSThread* os_thread) { os_thread_ = os_thread; } // The topmost zone used for allocation in this thread.