Fix tsan failure

runtime/vm/profiler.cc

Index: runtime/vm/profiler.cc
diff --git a/runtime/vm/profiler.cc b/runtime/vm/profiler.cc
index 05f830292c76c03c54568a4fac18debcebb55717..7aaa3657ee1c20ac684c8c35230f040c59497c68 100644
--- a/runtime/vm/profiler.cc
+++ b/runtime/vm/profiler.cc
@@ -418,67 +418,54 @@ class ProfilerStackWalker : public ValueObject {
 };
 
 
-// Given an exit frame, walk the Dart stack.
-class ProfilerDartExitStackWalker : public ProfilerStackWalker {
- public:
-  ProfilerDartExitStackWalker(Thread* thread,
-                              Isolate* isolate,
-                              Sample* sample,
-                              SampleBuffer* sample_buffer)
-      : ProfilerStackWalker(isolate, sample, sample_buffer),
-        frame_iterator_(thread) {}
-
-  void walk() {
-    // Mark that this sample was collected from an exit frame.
-    sample_->set_exit_frame_sample(true);
-
-    StackFrame* frame = frame_iterator_.NextFrame();
-    if (sample_ == NULL) {
-      // Only when we are dumping the stack trace for debug purposes.
-      Code& code = Code::Handle();
-      while (frame != NULL) {
-        code ^= frame->LookupDartCode();
-        if (!Append(frame->pc(), code)) {
-          return;
-        }
-        frame = frame_iterator_.NextFrame();
-      }
-    } else {
-      while (frame != NULL) {
-        if (!Append(frame->pc())) {
-          return;
-        }
-        frame = frame_iterator_.NextFrame();
-      }
-    }
-  }
-
- private:
-  DartFrameIterator frame_iterator_;
-};
-
-
 // Executing Dart code, walk the stack.
 class ProfilerDartStackWalker : public ProfilerStackWalker {
  public:
-  ProfilerDartStackWalker(Isolate* isolate,
+  ProfilerDartStackWalker(Thread* thread,
                           Sample* sample,
                           SampleBuffer* sample_buffer,
                           uword stack_lower,
                           uword stack_upper,
                           uword pc,
                           uword fp,
-                          uword sp)
-      : ProfilerStackWalker(isolate, sample, sample_buffer),
+                          uword sp,
+                          bool exited_dart_code,
+                          bool allocation_sample)
+      : ProfilerStackWalker(thread->isolate(), sample, sample_buffer),
+        pc_(reinterpret_cast<uword*>(pc)),
+        fp_(reinterpret_cast<uword*>(fp)),
+        sp_(reinterpret_cast<uword*>(sp)),
         stack_upper_(stack_upper),
-        stack_lower_(stack_lower) {
-    pc_ = reinterpret_cast<uword*>(pc);
-    fp_ = reinterpret_cast<uword*>(fp);
-    sp_ = reinterpret_cast<uword*>(sp);
+        stack_lower_(stack_lower),
+        has_exit_frame_(exited_dart_code) {
+    if (exited_dart_code) {
+      StackFrameIterator iterator(StackFrameIterator::kDontValidateFrames,
+                                  thread);
+      pc_ = NULL;
+      fp_ = NULL;
+      sp_ = NULL;
+      if (!iterator.HasNextFrame()) {
+        return;
+      }
+      // Ensure we are able to get to the exit frame.
+      StackFrame* frame = iterator.NextFrame();
+      if (!frame->IsExitFrame()) {
+        return;
+      }
+      // Skip the exit frame.
+      if (!iterator.HasNextFrame()) {
+        return;
+      }
+      frame = iterator.NextFrame();
+      // Record frame details of the first frame from which we start walking.
+      pc_ = reinterpret_cast<uword*>(frame->pc());
+      fp_ = reinterpret_cast<uword*>(frame->fp());
+      sp_ = reinterpret_cast<uword*>(frame->sp());
+    }
   }
 
   void walk() {
-    sample_->set_exit_frame_sample(false);
+    sample_->set_exit_frame_sample(has_exit_frame_);
     if (!ValidFramePointer()) {
       sample_->set_ignore_sample(true);
       return;
@@ -605,7 +592,8 @@ class ProfilerDartStackWalker : public ProfilerStackWalker {
   uword* fp_;
   uword* sp_;
   const uword stack_upper_;
-  uword stack_lower_;
+  const uword stack_lower_;
+  bool has_exit_frame_;
 };
 
 
@@ -758,7 +746,6 @@ static void CollectSample(Isolate* isolate,
                           bool in_dart_code,
                           Sample* sample,
                           ProfilerNativeStackWalker* native_stack_walker,
-                          ProfilerDartExitStackWalker* dart_exit_stack_walker,
                           ProfilerDartStackWalker* dart_stack_walker,
                           uword pc,
                           uword fp,
@@ -783,7 +770,7 @@ static void CollectSample(Isolate* isolate,
     } else if (StubCode::HasBeenInitialized() && exited_dart_code) {
       AtomicOperations::IncrementInt64By(&counters->stack_walker_dart_exit, 1);

[Cutch, 2016/11/18 17:59:08]

these two branches could be merged or maybe we want the counters to be distinct
still.

[siva, 2016/11/18 22:08:47]

We would still have to change the condition to
else if (StubCode::HasBeenInitialized() &&
         (exited_dart_code || in_dart_code))
so much of a merge, we could leave the counters as is.

       // We have a valid exit frame info, use the Dart stack walker.
-      dart_exit_stack_walker->walk();
+      dart_stack_walker->walk();
     } else if (StubCode::HasBeenInitialized() && in_dart_code) {
       AtomicOperations::IncrementInt64By(&counters->stack_walker_dart, 1);
       // We are executing Dart code. We have frame pointers.
@@ -964,8 +951,6 @@ void Profiler::DumpStackTrace(bool native_stack_trace) {
     return;
   }
 
-  const bool exited_dart_code = thread->HasExitedDartCode();
-
   OS::PrintErr("Dumping %s stack trace for thread %" Px "\n",
                native_stack_trace ? "native" : "dart-only",
                OSThread::ThreadIdToIntPtr(os_thread->trace_id()));
@@ -991,18 +976,10 @@ void Profiler::DumpStackTrace(bool native_stack_trace) {
     return;
   }
 
-  if (native_stack_trace) {
-    ProfilerNativeStackWalker native_stack_walker(
-        isolate, NULL, NULL, stack_lower, stack_upper, pc, fp, sp);
-    native_stack_walker.walk();
-  } else if (exited_dart_code) {
-    ProfilerDartExitStackWalker dart_exit_stack_walker(thread, isolate, NULL,
-                                                       NULL);
-    dart_exit_stack_walker.walk();
-  } else {
-    ProfilerDartStackWalker dart_stack_walker(isolate, NULL, NULL, stack_lower,
-                                              stack_upper, pc, fp, sp);
-  }
+  ASSERT(native_stack_trace);
+  ProfilerNativeStackWalker native_stack_walker(
+      isolate, NULL, NULL, stack_lower, stack_upper, pc, fp, sp);
+  native_stack_walker.walk();
   OS::PrintErr("-- End of DumpStackTrace\n");
 }
 
@@ -1024,36 +1001,36 @@ void Profiler::SampleAllocation(Thread* thread, intptr_t cid) {
     return;
   }
 
-  if (FLAG_profile_vm) {
-    uintptr_t sp = Thread::GetCurrentStackPointer();
-    uintptr_t fp = 0;
-    uintptr_t pc = GetProgramCounter();
+  uintptr_t sp = Thread::GetCurrentStackPointer();
+  uintptr_t fp = 0;
+  uintptr_t pc = GetProgramCounter();
 
-    COPY_FP_REGISTER(fp);
+  COPY_FP_REGISTER(fp);
 
-    uword stack_lower = 0;
-    uword stack_upper = 0;
+  uword stack_lower = 0;
+  uword stack_upper = 0;
 
-    if (!InitialRegisterCheck(pc, fp, sp)) {
-      return;
-    }
+  if (!InitialRegisterCheck(pc, fp, sp)) {
+    return;
+  }
 
-    if (!GetAndValidateIsolateStackBounds(thread, fp, sp, &stack_lower,
-                                          &stack_upper)) {
-      // Could not get stack boundary.
-      return;
-    }
+  if (!GetAndValidateIsolateStackBounds(thread, fp, sp, &stack_lower,
+                                        &stack_upper)) {
+    // Could not get stack boundary.
+    return;
+  }
 
-    Sample* sample = SetupSample(thread, sample_buffer, os_thread->trace_id());
-    sample->SetAllocationCid(cid);
+  Sample* sample = SetupSample(thread, sample_buffer, os_thread->trace_id());
+  sample->SetAllocationCid(cid);
+
+  if (FLAG_profile_vm) {
     ProfilerNativeStackWalker native_stack_walker(
         isolate, sample, sample_buffer, stack_lower, stack_upper, pc, fp, sp);
     native_stack_walker.walk();
   } else if (exited_dart_code) {
-    Sample* sample = SetupSample(thread, sample_buffer, os_thread->trace_id());
-    sample->SetAllocationCid(cid);
-    ProfilerDartExitStackWalker dart_exit_stack_walker(thread, isolate, sample,
-                                                       sample_buffer);
+    ProfilerDartStackWalker dart_exit_stack_walker(
+        thread, sample, sample_buffer, stack_lower, stack_upper, pc, fp, sp,
+        exited_dart_code, true);
     dart_exit_stack_walker.walk();
   } else {
     // Fall back.
@@ -1192,19 +1169,15 @@ void Profiler::SampleThread(Thread* thread,
 
   ProfilerNativeStackWalker native_stack_walker(
       isolate, sample, sample_buffer, stack_lower, stack_upper, pc, fp, sp);
-
-  ProfilerDartExitStackWalker dart_exit_stack_walker(thread, isolate, sample,
-                                                     sample_buffer);
-
-  ProfilerDartStackWalker dart_stack_walker(
-      isolate, sample, sample_buffer, stack_lower, stack_upper, pc, fp, sp);
-
   const bool exited_dart_code = thread->HasExitedDartCode();
+  ProfilerDartStackWalker dart_stack_walker(thread, sample, sample_buffer,
+                                            stack_lower, stack_upper, pc, fp,
+                                            sp, exited_dart_code, false);
 
   // All memory access is done inside CollectSample.
   CollectSample(isolate, exited_dart_code, in_dart_code, sample,
-                &native_stack_walker, &dart_exit_stack_walker,
-                &dart_stack_walker, pc, fp, sp, &counters_);
+                &native_stack_walker, &dart_stack_walker, pc, fp, sp,
+                &counters_);
 }