[vm] More TSAN profiler issues.

TEST=tsan
Bug: #61594
Change-Id: I82598218a98b764d66deb671f8ec841c8e1c3cab
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/452280
Commit-Queue: Ryan Macnak <[email protected]>
Reviewed-by: Alexander Aprelev <[email protected]>

Author: rmacnak-google

runtime/vm/isolate.h

@@ -1195,6 +1195,9 @@ class Isolate : public IntrusiveDListEntry<Isolate> {
   void set_current_sample_block(SampleBlock* block) {
     current_sample_block_ = block;
   }
+  SampleBlock* exchange_current_sample_block(SampleBlock* block) {
+    return current_sample_block_.exchange(block, std::memory_order_acq_rel);
+  }
   void ProcessFreeSampleBlocks(Thread* thread);
 
   // Returns the current SampleBlock used to track Dart allocation samples.
@@ -1204,6 +1207,10 @@ class Isolate : public IntrusiveDListEntry<Isolate> {
   void set_current_allocation_sample_block(SampleBlock* block) {
     current_allocation_sample_block_ = block;
   }
+  SampleBlock* exchange_current_allocation_sample_block(SampleBlock* block) {
+    return current_allocation_sample_block_.exchange(block,
+                                                     std::memory_order_acq_rel);
+  }
 
   bool TakeHasCompletedBlocks() {
     return has_completed_blocks_.exchange(0) != 0;

runtime/vm/profiler.cc

@@ -794,15 +794,14 @@ void SampleBlockBuffer::FreeCompletedBlocks() {
 static void FlushSampleBlocks(Isolate* isolate) {
   ASSERT(isolate != nullptr);
 
-  SampleBlock* block = isolate->current_sample_block();
+  SampleBlock* block = isolate->exchange_current_sample_block(nullptr);
   if (block != nullptr) {
-    isolate->set_current_sample_block(nullptr);
     block->MarkCompleted();
   }
 
-  block = isolate->current_allocation_sample_block();
+  block = isolate->exchange_current_allocation_sample_block(nullptr);
   if (block != nullptr) {
-    isolate->set_current_allocation_sample_block(nullptr);
+    // Allocation samples are collected synchronously.
     block->MarkCompleted();
   }
 }
@@ -1416,6 +1415,25 @@ void Profiler::SampleThreadSingleFrame(Thread* thread,
   sample->SetAt(0, pc);
 }
 
+void ReleaseToCurrentBlock(Isolate* isolate) {
+#if defined(DART_HOST_OS_MACOS) || defined(DART_HOST_OS_WINDOWS) ||            \
+    defined(DART_HOST_OS_FUCHSIA)
+  // The sample is collected by a different thread. The sample appears all at
+  // once from the profiled thread's point of view. Establish the isolate
+  // flushing its own current block happens-after the most recent sample
+  // written in that block by dumping a dependency through the current block.
+  // TSAN doesn't otherwise know this is already true because it doesn't have
+  // special treatment for thread_suspend/resume.
+  SampleBlock* block = isolate->current_sample_block();
+  isolate->exchange_current_sample_block(block);
+#elif defined(DART_HOST_OS_LINUX) || defined(DART_HOST_OS_ANDROID)
+  // The sample is collected by a signal handler on the same thread being
+  // sampled.
+#else
+#error What kind of sampler?
+#endif
+}
+
 void Profiler::SampleThread(Thread* thread,
                             const InterruptedThreadState& state) {
   ASSERT(thread != nullptr);
@@ -1494,6 +1512,7 @@ void Profiler::SampleThread(Thread* thread,
     if (thread->IGNORE_RACE2(IsDeoptimizing)()) {
       counters_.single_frame_sample_deoptimizing.fetch_add(1);
       SampleThreadSingleFrame(thread, sample, pc);
+      ReleaseToCurrentBlock(isolate);
       return;
     }
   }
@@ -1505,6 +1524,7 @@ void Profiler::SampleThread(Thread* thread,
     counters_.single_frame_sample_get_and_validate_stack_bounds.fetch_add(1);
     // Could not get stack boundary.
     SampleThreadSingleFrame(thread, sample, pc);
+    ReleaseToCurrentBlock(isolate);
     return;
   }
 
@@ -1531,6 +1551,7 @@ void Profiler::SampleThread(Thread* thread,
   CollectSample(isolate, exited_dart_code, in_dart_code, sample,
                 &native_stack_walker, &dart_stack_walker, pc, fp, sp,
                 &counters_);
+  ReleaseToCurrentBlock(isolate);
 }
 
 CodeDescriptor::CodeDescriptor(const AbstractCode code) : code_(code) {}

runtime/vm/profiler_test.cc

@@ -4,6 +4,7 @@
 
 #include "platform/assert.h"
 
+#include "platform/thread_sanitizer.h"
 #include "vm/dart_api_impl.h"
 #include "vm/dart_api_state.h"
 #include "vm/flags.h"
@@ -29,19 +30,27 @@ const int64_t kValidTimeStamp = 1;
 // SampleVisitor ignores samples with pc == 0.
 const uword kValidPc = 0xFF;
 
+NO_SANITIZE_THREAD
+static void SetIgnoreRace(bool* flag, bool value) {
+  *flag = value;
+}
+
 // Some tests are written assuming native stack trace profiling is disabled.
 class DisableNativeProfileScope : public ValueObject {
  public:
   DisableNativeProfileScope()
       : FLAG_profile_vm_(FLAG_profile_vm),
         FLAG_profile_vm_allocation_(FLAG_profile_vm_allocation) {
-    FLAG_profile_vm = false;
-    FLAG_profile_vm_allocation = false;
+    // Ignore race: we generally don't try to ensure the profiler is disabled
+    // when changing setting for unit tests. On Linux/Android, there is no way
+    // to wait for outstanding SIGPROFs.
+    SetIgnoreRace(&FLAG_profile_vm, false);
+    SetIgnoreRace(&FLAG_profile_vm_allocation, false);
   }
 
   ~DisableNativeProfileScope() {
-    FLAG_profile_vm = FLAG_profile_vm_;
-    FLAG_profile_vm_allocation = FLAG_profile_vm_allocation_;
+    SetIgnoreRace(&FLAG_profile_vm, FLAG_profile_vm_);
+    SetIgnoreRace(&FLAG_profile_vm_allocation, FLAG_profile_vm_allocation_);
   }
 
  private: