[Sampling profiler] Add Unwinder::OnStackCapture

Adds a function to the Unwinder interface that is invoked at the time the
stack is captured, while the target thread is suspended, and wires up the
StackCopiers and StackSamplerImpl to invoke it at the appropriate time.

This function will be overridden in the new V8 unwinder to save off the
active V8 code pages while the thread is suspended.

Bug: 1035855
Change-Id: Ide287443faed211e2e6d205ebe78b8a536caa729
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2074806
Commit-Queue: Mike Wittman <wittman@chromium.org>
Reviewed-by: Etienne Pierre-Doray <etiennep@chromium.org>
Cr-Commit-Position: refs/heads/master@{#747393}

base/profiler/stack_copier.h

@@ -23,17 +23,33 @@ class ProfileBuilder;
 // is performed by the OS through signals (Android).
 class BASE_EXPORT StackCopier {
  public:
+  // Interface that may be implemented by the caller of CopyStack() to receive a
+  // callback when the stack is copied, while the target thread is suspended.
+  class BASE_EXPORT Delegate {
+   public:
+    virtual ~Delegate() {}
+
+    // IMPORTANT NOTE: to avoid deadlock implementations of this interface must
+    // not invoke any non-reentrant code that is also invoked by the target
+    // thread. In particular, it may not perform any heap allocation or
+    // deallocation, including indirectly via use of DCHECK/CHECK or other
+    // logging statements.
+    virtual void OnStackCopy() = 0;
+  };
+
   virtual ~StackCopier();
 
   // Copies the thread's register context into |thread_context|, the stack into
   // |stack_buffer|, and the top of stack address into |stack_top|. Records
   // metadata while the thread is suspended via |profile_builder|. Records
-  // |timestamp| at the time the stack was copied. Returns true if successful.
+  // |timestamp| at the time the stack was copied. delegate->OnStackCopy() will
+  // be invoked while the thread is suspended. Returns true if successful.
   virtual bool CopyStack(StackBuffer* stack_buffer,
                          uintptr_t* stack_top,
                          ProfileBuilder* profile_builder,
                          TimeTicks* timestamp,
-                         RegisterContext* thread_context) = 0;
+                         RegisterContext* thread_context,
+                         Delegate* delegate) = 0;
 
  protected:
   // If the value at |pointer| points to the original stack, rewrite it to point

base/profiler/stack_copier_signal.cc

@@ -95,6 +95,9 @@ struct HandlerParams {
 
   // The timestamp when the stack was copied.
   TimeTicks* timestamp;
+
+  // The delegate provided to the StackCopier.
+  StackCopier::Delegate* stack_copier_delegate;
 };
 
 // Pointer to the parameters to be "passed" to the CopyStackSignalHandler() from
@@ -127,13 +130,16 @@ void CopyStackSignalHandler(int n, siginfo_t* siginfo, void* sigcontext) {
     return;
   }
 
+  // TODO(https://crbug.com/988579): Record metadata while the thread is
+  // suspended.
+
+  params->stack_copier_delegate->OnStackCopy();
+
   *params->stack_copy_bottom =
       StackCopierSignal::CopyStackContentsAndRewritePointers(
           reinterpret_cast<uint8_t*>(bottom), reinterpret_cast<uintptr_t*>(top),
           StackBuffer::kPlatformStackAlignment, params->stack_buffer->buffer());
 
-  // TODO(https://crbug.com/988579): Record metadata while the thread is
-  // suspended.
   *params->success = true;
 }
 
@@ -188,14 +194,15 @@ bool StackCopierSignal::CopyStack(StackBuffer* stack_buffer,
                                   uintptr_t* stack_top,
                                   ProfileBuilder* profile_builder,
                                   TimeTicks* timestamp,
-                                  RegisterContext* thread_context) {
+                                  RegisterContext* thread_context,
+                                  Delegate* delegate) {
   AsyncSafeWaitableEvent wait_event;
   bool copied = false;
   const uint8_t* stack_copy_bottom = nullptr;
   const uintptr_t stack_base_address = thread_delegate_->GetStackBaseAddress();
   HandlerParams params = {stack_base_address, &wait_event,  &copied,
                           thread_context,     stack_buffer, &stack_copy_bottom,
-                          timestamp};
+                          timestamp,          delegate};
   {
     ScopedSetSignalHandlerParams scoped_handler_params(&params);
 

base/profiler/stack_copier_signal.h

@@ -26,7 +26,8 @@ class BASE_EXPORT StackCopierSignal : public StackCopier {
                  uintptr_t* stack_top,
                  ProfileBuilder* profile_builder,
                  TimeTicks* timestamp,
-                 RegisterContext* thread_context) override;
+                 RegisterContext* thread_context,
+                 Delegate* delegate) override;
 
   using StackCopier::CopyStackContentsAndRewritePointers;
 

base/profiler/stack_copier_signal_unittest.cc

@@ -82,6 +82,15 @@ class TargetThread : public SimpleThread {
   SamplingProfilerThreadToken thread_token_;
 };
 
+class TestStackCopierDelegate : public StackCopier::Delegate {
+ public:
+  void OnStackCopy() override { was_invoked_ = true; }
+  bool was_invoked() const { return was_invoked_; }
+
+ private:
+  bool was_invoked_ = false;
+};
+
 }  // namespace
 
 // ASAN moves local variables outside of the stack extents, which breaks the
@@ -104,6 +113,7 @@ TEST(StackCopierSignalTest, MAYBE_CopyStack) {
   TestProfileBuilder profiler_builder;
   TimeTicks timestamp;
   RegisterContext context;
+  TestStackCopierDelegate stack_copier_delegate;
 
   StackCopierSignal copier(std::make_unique<ThreadDelegatePosix>(
       GetSamplingProfilerCurrentThreadToken()));
@@ -115,7 +125,7 @@ TEST(StackCopierSignalTest, MAYBE_CopyStack) {
     sentinels[i] = kStackSentinels[i];
 
   bool result = copier.CopyStack(&stack_buffer, &stack_top, &profiler_builder,
-                                 &timestamp, &context);
+                                 &timestamp, &context, &stack_copier_delegate);
   ASSERT_TRUE(result);
 
   uint32_t* const end = reinterpret_cast<uint32_t*>(stack_top);
@@ -141,13 +151,14 @@ TEST(StackCopierSignalTest, MAYBE_CopyStackTimestamp) {
   TestProfileBuilder profiler_builder;
   TimeTicks timestamp;
   RegisterContext context;
+  TestStackCopierDelegate stack_copier_delegate;
 
   StackCopierSignal copier(std::make_unique<ThreadDelegatePosix>(
       GetSamplingProfilerCurrentThreadToken()));
 
   TimeTicks before = TimeTicks::Now();
   bool result = copier.CopyStack(&stack_buffer, &stack_top, &profiler_builder,
-                                 &timestamp, &context);
+                                 &timestamp, &context, &stack_copier_delegate);
   TimeTicks after = TimeTicks::Now();
   ASSERT_TRUE(result);
 
@@ -155,6 +166,31 @@ TEST(StackCopierSignalTest, MAYBE_CopyStackTimestamp) {
   EXPECT_LE(timestamp, after);
 }
 
+// TSAN hangs on the AsyncSafeWaitableEvent FUTEX_WAIT call.
+#if defined(THREAD_SANITIZER)
+#define MAYBE_CopyStackDelegateInvoked DISABLED_CopyStackDelegateInvoked
+#else
+#define MAYBE_CopyStackDelegateInvoked CopyStackDelegateInvoked
+#endif
+TEST(StackCopierSignalTest, MAYBE_CopyStackDelegateInvoked) {
+  StackBuffer stack_buffer(/* buffer_size = */ 1 << 20);
+  memset(stack_buffer.buffer(), 0, stack_buffer.size());
+  uintptr_t stack_top = 0;
+  TestProfileBuilder profiler_builder;
+  TimeTicks timestamp;
+  RegisterContext context;
+  TestStackCopierDelegate stack_copier_delegate;
+
+  StackCopierSignal copier(std::make_unique<ThreadDelegatePosix>(
+      GetSamplingProfilerCurrentThreadToken()));
+
+  bool result = copier.CopyStack(&stack_buffer, &stack_top, &profiler_builder,
+                                 &timestamp, &context, &stack_copier_delegate);
+  ASSERT_TRUE(result);
+
+  EXPECT_TRUE(stack_copier_delegate.was_invoked());
+}
+
 // Limit to 32-bit Android, which is the platform we care about for this
 // functionality. The test is broken on too many other varied platforms to try
 // to selectively disable.
@@ -170,6 +206,7 @@ TEST(StackCopierSignalTest, MAYBE_CopyStackFromOtherThread) {
   TestProfileBuilder profiler_builder;
   TimeTicks timestamp;
   RegisterContext context{};
+  TestStackCopierDelegate stack_copier_delegate;
 
   TargetThread target_thread;
   target_thread.Start();
@@ -179,7 +216,7 @@ TEST(StackCopierSignalTest, MAYBE_CopyStackFromOtherThread) {
   StackCopierSignal copier(std::make_unique<ThreadDelegatePosix>(thread_token));
 
   bool result = copier.CopyStack(&stack_buffer, &stack_top, &profiler_builder,
-                                 &timestamp, &context);
+                                 &timestamp, &context, &stack_copier_delegate);
   ASSERT_TRUE(result);
 
   target_thread.NotifyCopyFinished();

base/profiler/stack_copier_suspend.cc

@@ -27,7 +27,8 @@ bool StackCopierSuspend::CopyStack(StackBuffer* stack_buffer,
                                    uintptr_t* stack_top,
                                    ProfileBuilder* profile_builder,
                                    TimeTicks* timestamp,
-                                   RegisterContext* thread_context) {
+                                   RegisterContext* thread_context,
+                                   Delegate* delegate) {
   const uintptr_t top = thread_delegate_->GetStackBaseAddress();
   uintptr_t bottom = 0;
   const uint8_t* stack_copy_bottom = nullptr;
@@ -67,6 +68,8 @@ bool StackCopierSuspend::CopyStack(StackBuffer* stack_buffer,
 
     profile_builder->RecordMetadata(get_metadata_items.get());
 
+    delegate->OnStackCopy();
+
     stack_copy_bottom = CopyStackContentsAndRewritePointers(
         reinterpret_cast<uint8_t*>(bottom), reinterpret_cast<uintptr_t*>(top),
         StackBuffer::kPlatformStackAlignment, stack_buffer->buffer());

base/profiler/stack_copier_suspend.h

@@ -28,7 +28,8 @@ class BASE_EXPORT StackCopierSuspend : public StackCopier {
                  uintptr_t* stack_top,
                  ProfileBuilder* profile_builder,
                  TimeTicks* timestamp,
-                 RegisterContext* thread_context) override;
+                 RegisterContext* thread_context,
+                 Delegate* delegate) override;
 
  private:
   std::unique_ptr<SuspendableThreadDelegate> thread_delegate_;

base/profiler/stack_copier_suspend_unittest.cc

@@ -107,6 +107,15 @@ class TestProfileBuilder : public ProfileBuilder {
   bool recorded_metadata_ = false;
 };
 
+class TestStackCopierDelegate : public StackCopier::Delegate {
+ public:
+  void OnStackCopy() override { was_invoked_ = true; }
+  bool was_invoked() const { return was_invoked_; }
+
+ private:
+  bool was_invoked_ = false;
+};
+
 }  // namespace
 
 TEST(StackCopierSuspendTest, CopyStack) {
@@ -120,9 +129,10 @@ TEST(StackCopierSuspendTest, CopyStack) {
   TestProfileBuilder profile_builder;
   TimeTicks timestamp;
   RegisterContext register_context{};
+  TestStackCopierDelegate stack_copier_delegate;
   stack_copier_suspend.CopyStack(stack_buffer.get(), &stack_top,
                                  &profile_builder, &timestamp,
-                                 &register_context);
+                                 &register_context, &stack_copier_delegate);
 
   uintptr_t* stack_copy_bottom =
       reinterpret_cast<uintptr_t*>(stack_buffer.get()->buffer());
@@ -144,9 +154,10 @@ TEST(StackCopierSuspendTest, CopyStackBufferTooSmall) {
   TestProfileBuilder profile_builder;
   TimeTicks timestamp;
   RegisterContext register_context{};
+  TestStackCopierDelegate stack_copier_delegate;
   stack_copier_suspend.CopyStack(stack_buffer.get(), &stack_top,
                                  &profile_builder, &timestamp,
-                                 &register_context);
+                                 &register_context, &stack_copier_delegate);
 
   uintptr_t* stack_copy_bottom =
       reinterpret_cast<uintptr_t*>(stack_buffer.get()->buffer());
@@ -171,9 +182,10 @@ TEST(StackCopierSuspendTest, CopyStackAndRewritePointers) {
   TestProfileBuilder profile_builder;
   TimeTicks timestamp;
   RegisterContext register_context{};
+  TestStackCopierDelegate stack_copier_delegate;
   stack_copier_suspend.CopyStack(stack_buffer.get(), &stack_top,
                                  &profile_builder, &timestamp,
-                                 &register_context);
+                                 &register_context, &stack_copier_delegate);
 
   uintptr_t* stack_copy_bottom =
       reinterpret_cast<uintptr_t*>(stack_buffer.get()->buffer());
@@ -196,20 +208,42 @@ TEST(StackCopierSuspendTest, CopyStackTimeStamp) {
   TestProfileBuilder profile_builder;
   TimeTicks timestamp;
   RegisterContext register_context{};
+  TestStackCopierDelegate stack_copier_delegate;
 
   TimeTicks before = TimeTicks::Now();
   stack_copier_suspend.CopyStack(stack_buffer.get(), &stack_top,
                                  &profile_builder, &timestamp,
-                                 &register_context);
+                                 &register_context, &stack_copier_delegate);
   TimeTicks after = TimeTicks::Now();
 
   EXPECT_GE(timestamp, before);
   EXPECT_LE(timestamp, after);
 }
 
+TEST(StackCopierSuspendTest, CopyStackDelegateInvoked) {
+  const std::vector<uintptr_t> stack = {0};
+  StackCopierSuspend stack_copier_suspend(
+      std::make_unique<TestSuspendableThreadDelegate>(stack));
+
+  std::unique_ptr<StackBuffer> stack_buffer =
+      std::make_unique<StackBuffer>(stack.size() * sizeof(uintptr_t));
+  uintptr_t stack_top = 0;
+  TestProfileBuilder profile_builder;
+  TimeTicks timestamp;
+  RegisterContext register_context{};
+  TestStackCopierDelegate stack_copier_delegate;
+
+  stack_copier_suspend.CopyStack(stack_buffer.get(), &stack_top,
+                                 &profile_builder, &timestamp,
+                                 &register_context, &stack_copier_delegate);
+
+  EXPECT_TRUE(stack_copier_delegate.was_invoked());
+}
+
 TEST(StackCopierSuspendTest, RewriteRegisters) {
   std::vector<uintptr_t> stack = {0, 1, 2};
   RegisterContext register_context{};
+  TestStackCopierDelegate stack_copier_delegate;
   RegisterContextFramePointer(&register_context) =
       reinterpret_cast<uintptr_t>(&stack[1]);
   StackCopierSuspend stack_copier_suspend(
@@ -223,7 +257,7 @@ TEST(StackCopierSuspendTest, RewriteRegisters) {
   TestProfileBuilder profile_builder;
   stack_copier_suspend.CopyStack(stack_buffer.get(), &stack_top,
                                  &profile_builder, &timestamp,
-                                 &register_context);
+                                 &register_context, &stack_copier_delegate);
 
   uintptr_t stack_copy_bottom =
       reinterpret_cast<uintptr_t>(stack_buffer.get()->buffer());

base/profiler/stack_sampler_impl.cc

@@ -46,8 +46,9 @@ void StackSamplerImpl::RecordStackFrames(StackBuffer* stack_buffer,
   RegisterContext thread_context;
   uintptr_t stack_top;
   TimeTicks timestamp;
-  bool success = stack_copier_->CopyStack(
-      stack_buffer, &stack_top, profile_builder, &timestamp, &thread_context);
+  bool success =
+      stack_copier_->CopyStack(stack_buffer, &stack_top, profile_builder,
+                               &timestamp, &thread_context, this);
   if (!success)
     return;
 
@@ -59,8 +60,18 @@ void StackSamplerImpl::RecordStackFrames(StackBuffer* stack_buffer,
                 native_unwinder_.get(), aux_unwinder_.get()),
       timestamp);
 }
-// static
 
+// IMPORTANT NOTE: to avoid deadlock this function must not invoke any
+// non-reentrant code that is also invoked by the target thread. In particular,
+// it may not perform any heap allocation or deallocation, including indirectly
+// via use of DCHECK/CHECK or other logging statements.
+void StackSamplerImpl::OnStackCopy() {
+  native_unwinder_->OnStackCapture();
+  if (aux_unwinder_)
+    aux_unwinder_->OnStackCapture();
+}
+
+// static
 std::vector<Frame> StackSamplerImpl::WalkStackForTesting(
     ModuleCache* module_cache,
     RegisterContext* thread_context,

base/profiler/stack_sampler_impl.h

@@ -10,16 +10,17 @@
 #include "base/base_export.h"
 #include "base/profiler/frame.h"
 #include "base/profiler/register_context.h"
+#include "base/profiler/stack_copier.h"
 #include "base/profiler/stack_sampler.h"
 
 namespace base {
 
-class StackCopier;
 class Unwinder;
 
 // Cross-platform stack sampler implementation. Delegates to StackCopier for the
 // platform-specific stack copying implementation.
-class BASE_EXPORT StackSamplerImpl : public StackSampler {
+class BASE_EXPORT StackSamplerImpl : public StackSampler,
+                                     public StackCopier::Delegate {
  public:
   StackSamplerImpl(std::unique_ptr<StackCopier> stack_copier,
                    std::unique_ptr<Unwinder> native_unwinder,
@@ -35,6 +36,9 @@ class BASE_EXPORT StackSamplerImpl : public StackSampler {
   void RecordStackFrames(StackBuffer* stack_buffer,
                          ProfileBuilder* profile_builder) override;
 
+  // StackCopier::Delegate:
+  void OnStackCopy() override;
+
   // Exposes the internal function for unit testing.
   static std::vector<Frame> WalkStackForTesting(ModuleCache* module_cache,
                                                 RegisterContext* thread_context,

base/profiler/stack_sampler_impl_unittest.cc

@@ -66,7 +66,8 @@ class TestStackCopier : public StackCopier {
                  uintptr_t* stack_top,
                  ProfileBuilder* profile_builder,
                  TimeTicks* timestamp,
-                 RegisterContext* thread_context) override {
+                 RegisterContext* thread_context,
+                 Delegate* delegate) override {
     std::memcpy(stack_buffer->buffer(), &fake_stack_[0], fake_stack_.size());
     *stack_top =
         reinterpret_cast<uintptr_t>(&fake_stack_[0] + fake_stack_.size());

base/profiler/unwinder.h

@@ -31,8 +31,7 @@ enum class UnwindResult {
 // use with the StackSamplingProfiler. The profiler is expected to call
 // CanUnwind() to determine if the Unwinder thinks it can unwind from the frame
 // represented by the context values, then TryUnwind() to attempt the
-// unwind. Note that the stack samples for multiple collection scenarios are
-// interleaved on a single Unwinder instance.
+// unwind.
 class Unwinder {
  public:
   virtual ~Unwinder() = default;
@@ -41,6 +40,13 @@ class Unwinder {
   // ModuleCache.
   virtual void AddInitialModules(ModuleCache* module_cache) {}
 
+  // Invoked at the time the stack is captured. IMPORTANT NOTE: this function is
+  // invoked while the target thread is suspended. To avoid deadlock it must not
+  // invoke any non-reentrant code that is also invoked by the target thread. In
+  // particular, it may not perform any heap allocation or deallocation,
+  // including indirectly via use of DCHECK/CHECK or other logging statements.
+  virtual void OnStackCapture() {}
+
   // Returns true if the unwinder recognizes the code referenced by
   // |current_frame| as code from which it should be able to unwind. When
   // multiple unwinders are in use, each should return true for a disjoint set