[Sampling profiler] Record frame from context before unwinding (Win)

The frame that's currently executing can be immediately recorded from
the values in the register context, independent of any unwinding.
This change separates the initial frame recording from the recording
within the native unwinding, which will be required to support restarting
native unwinding after v8 unwinding.

Note: this change alters the stack walking behavior in the case of
code that isn't associated with a module. The WalkNativeFrames now
appends a frame with the instruction pointer and a null module,
whereas before it omitted the frame entirely.

Bug: 909957
Change-Id: Id14284004cb69a39cf6a9e7d7b3e86269ab45011
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1540266
Commit-Queue: Mike Wittman <wittman@chromium.org>
Reviewed-by: Leonard Grey <lgrey@chromium.org>
Cr-Commit-Position: refs/heads/master@{#645138}

base/profiler/stack_sampling_profiler_unittest.cc

@@ -562,7 +562,8 @@ std::string FormatSampleForDiagnosticOutput(const Frames& frames) {
   for (const auto& frame : frames) {
     output += StringPrintf(
         "0x%p %s\n", reinterpret_cast<const void*>(frame.instruction_pointer),
-        frame.module->GetDebugBasename().AsUTF8Unsafe().c_str());
+        frame.module ? frame.module->GetDebugBasename().AsUTF8Unsafe().c_str()
+                     : "null module");
   }
   return output;
 }
@@ -676,15 +677,27 @@ void TestLibraryUnload(bool wait_until_unloaded, ModuleCache* module_cache) {
 
   if (wait_until_unloaded) {
     // The stack should look like this, resulting one frame after
-    // SignalAndWaitUntilSignaled. The frame in the now-unloaded library is
-    // not recorded since we can't get module information.
+    // SignalAndWaitUntilSignaled. The frame in the now-unloaded library should
+    // have a null module.
     //
     // ... WaitableEvent and system frames ...
     // TargetThread::SignalAndWaitUntilSignaled
     // TargetThread::OtherLibraryCallback
+    // <frame in unloaded library>
+#if !defined(OS_MACOSX)
+    EXPECT_EQ(3, frames.end() - end_frame)
+        << "Stack:\n"
+        << FormatSampleForDiagnosticOutput(frames);
+    EXPECT_EQ(nullptr, frames.back().module)
+        << "Stack:\n"
+        << FormatSampleForDiagnosticOutput(frames);
+#else
+    // TODO(wittman): Fix Mac unwinding to match the expected behavior
+    // above. Currently it doesn't produce a frame for the unloaded library.
     EXPECT_EQ(2, frames.end() - end_frame)
         << "Stack:\n"
         << FormatSampleForDiagnosticOutput(frames);
+#endif
   } else {
     // We didn't wait for the asynchronous unloading to complete, so the results
     // are non-deterministic: if the library finished unloading we should have

base/profiler/thread_delegate_win.cc

@@ -205,19 +205,21 @@ UnwindResult ThreadDelegateWin::WalkNativeFrames(
     uintptr_t stack_top,
     ModuleCache* module_cache,
     std::vector<ProfileBuilder::Frame>* stack) {
-  Win32StackFrameUnwinder frame_unwinder;
-  while (ContextPC(thread_context)) {
-    const ModuleCache::Module* const module =
-        module_cache->GetModuleForAddress(ContextPC(thread_context));
+  // Record the first frame from the context values.
+  const ModuleCache::Module* module =
+      module_cache->GetModuleForAddress(ContextPC(thread_context));
+  stack->emplace_back(ContextPC(thread_context), module);
 
+  Win32StackFrameUnwinder frame_unwinder;
+  for (;;) {
     if (!module) {
       // There's no loaded module containing the instruction pointer. This can
-      // be due to executing code that is not in a module. For example, V8
-      // generated code or runtime-generated code associated with third-party
-      // injected DLLs. It can also be due to the the module having been
-      // unloaded since we recorded the stack.  In the latter case the function
-      // unwind information was part of the unloaded module, so it's not
-      // possible to unwind further.
+      // be due to executing code that is not in a module (e.g. V8 generated
+      // code or runtime-generated code associated with third-party injected
+      // DLLs). It can also be due to the the module having been unloaded since
+      // we recorded the stack.  In the latter case the function unwind
+      // information was part of the unloaded module, so it's not possible to
+      // unwind further.
       //
       // If a module was found, it's still theoretically possible for the
       // detected module module to be different than the one that was loaded
@@ -231,13 +233,18 @@ UnwindResult ThreadDelegateWin::WalkNativeFrames(
       return UnwindResult::UNRECOGNIZED_FRAME;
     }
 
-    // Record the current frame.
-    stack->emplace_back(ContextPC(thread_context), module);
-
-    if (!frame_unwinder.TryUnwind(thread_context, module))
+    if (!frame_unwinder.TryUnwind(stack->size() == 1u, thread_context, module))
       return UnwindResult::ABORTED;
+
+    if (ContextPC(thread_context) == 0)
+      return UnwindResult::COMPLETED;
+
+    // Record the frame to which we just unwound.
+    module = module_cache->GetModuleForAddress(ContextPC(thread_context));
+    stack->emplace_back(ContextPC(thread_context), module);
   }
 
+  NOTREACHED();
   return UnwindResult::COMPLETED;
 }
 

base/profiler/win32_stack_frame_unwinder.cc

@@ -78,17 +78,7 @@ Win32StackFrameUnwinder::Win32StackFrameUnwinder()
 
 Win32StackFrameUnwinder::~Win32StackFrameUnwinder() {}
 
-bool Win32StackFrameUnwinder::TryUnwind(CONTEXT* context,
-                                        const ModuleCache::Module* module) {
-  bool result =
-      TryUnwindImpl(unwind_functions_.get(), at_top_frame_, context, module);
-  at_top_frame_ = false;
-  return result;
-}
-
-// static
-bool Win32StackFrameUnwinder::TryUnwindImpl(
-    UnwindFunctions* unwind_functions,
+bool Win32StackFrameUnwinder::TryUnwind(
     bool at_top_frame,
     CONTEXT* context,
     // The module parameter, while not directly used, is still passed because it
@@ -105,12 +95,12 @@ bool Win32StackFrameUnwinder::TryUnwindImpl(
   ULONG64 image_base;
   // Try to look up unwind metadata for the current function.
   PRUNTIME_FUNCTION runtime_function =
-      unwind_functions->LookupFunctionEntry(ContextPC(context), &image_base);
+      unwind_functions_->LookupFunctionEntry(ContextPC(context), &image_base);
   DCHECK_EQ(module->GetBaseAddress(), image_base);
 
   if (runtime_function) {
-    unwind_functions->VirtualUnwind(image_base, ContextPC(context),
-                                    runtime_function, context);
+    unwind_functions_->VirtualUnwind(image_base, ContextPC(context),
+                                     runtime_function, context);
     return true;
   }
 

base/profiler/win32_stack_frame_unwinder.h

@@ -38,9 +38,8 @@ inline ULONG64 ContextPC(CONTEXT* context) {
 #endif
 }
 
-// Instances of this class are expected to be created and destroyed for each
-// stack unwinding. This class is not used while the target thread is suspended,
-// so may allocate from the default heap.
+// This class is not used while the target thread is suspended, so may allocate
+// from the default heap.
 class BASE_EXPORT Win32StackFrameUnwinder {
  public:
   // Interface for Win32 unwind-related functionality this class depends
@@ -69,21 +68,15 @@ class BASE_EXPORT Win32StackFrameUnwinder {
   // Attempts to unwind the frame represented by |context|, where the
   // instruction pointer is known to be in |module|. Updates |context| if
   // successful.
-  bool TryUnwind(CONTEXT* context, const ModuleCache::Module* module);
+  bool TryUnwind(bool at_top_frame,
+                 CONTEXT* context,
+                 const ModuleCache::Module* module);
 
  private:
-  static bool TryUnwindImpl(UnwindFunctions* unwind_functions,
-                            bool at_top_frame,
-                            CONTEXT* context,
-                            const ModuleCache::Module* module);
-
   // This function is for internal and test purposes only.
   Win32StackFrameUnwinder(std::unique_ptr<UnwindFunctions> unwind_functions);
   friend class Win32StackFrameUnwinderTest;
 
-  // State associated with each stack unwinding.
-  bool at_top_frame_ = true;
-
   std::unique_ptr<UnwindFunctions> unwind_functions_;
 
   DISALLOW_COPY_AND_ASSIGN(Win32StackFrameUnwinder);

base/profiler/win32_stack_frame_unwinder_unittest.cc

@@ -147,15 +147,15 @@ TEST_F(Win32StackFrameUnwinderTest, FramesWithUnwindInfo) {
 
   TestModule stub_module1(kImageBaseIncrement);
   unwind_functions_->SetHasRuntimeFunction(&context);
-  EXPECT_TRUE(unwinder->TryUnwind(&context, &stub_module1));
+  EXPECT_TRUE(unwinder->TryUnwind(true, &context, &stub_module1));
 
   TestModule stub_module2(kImageBaseIncrement * 2);
   unwind_functions_->SetHasRuntimeFunction(&context);
-  EXPECT_TRUE(unwinder->TryUnwind(&context, &stub_module2));
+  EXPECT_TRUE(unwinder->TryUnwind(false, &context, &stub_module2));
 
   TestModule stub_module3(kImageBaseIncrement * 3);
   unwind_functions_->SetHasRuntimeFunction(&context);
-  EXPECT_TRUE(unwinder->TryUnwind(&context, &stub_module3));
+  EXPECT_TRUE(unwinder->TryUnwind(false, &context, &stub_module3));
 }
 
 // Checks that the CONTEXT's stack pointer gets popped when the top frame has no
@@ -167,19 +167,11 @@ TEST_F(Win32StackFrameUnwinderTest, FrameAtTopWithoutUnwindInfo) {
   DWORD64 original_rsp = reinterpret_cast<DWORD64>(&next_ip);
   context.Rsp = original_rsp;
 
-  TestModule stub_module1(kImageBaseIncrement);
+  TestModule stub_module(kImageBaseIncrement);
   unwind_functions_->SetNoRuntimeFunction(&context);
-  EXPECT_TRUE(unwinder->TryUnwind(&context, &stub_module1));
+  EXPECT_TRUE(unwinder->TryUnwind(true, &context, &stub_module));
   EXPECT_EQ(next_ip, context.Rip);
   EXPECT_EQ(original_rsp + 8, context.Rsp);
-
-  TestModule stub_module2(kImageBaseIncrement * 2);
-  unwind_functions_->SetHasRuntimeFunction(&context);
-  EXPECT_TRUE(unwinder->TryUnwind(&context, &stub_module2));
-
-  TestModule stub_module3(kImageBaseIncrement * 3);
-  unwind_functions_->SetHasRuntimeFunction(&context);
-  EXPECT_TRUE(unwinder->TryUnwind(&context, &stub_module3));
 }
 
 // Checks that a frame below the top of the stack with missing unwind info
@@ -190,13 +182,9 @@ TEST_F(Win32StackFrameUnwinderTest, FrameBelowTopWithoutUnwindInfo) {
     std::unique_ptr<Win32StackFrameUnwinder> unwinder = CreateUnwinder();
     CONTEXT context = {0};
 
-    TestModule stub_module1(kImageBaseIncrement);
-    unwind_functions_->SetHasRuntimeFunction(&context);
-    EXPECT_TRUE(unwinder->TryUnwind(&context, &stub_module1));
-
-    TestModule stub_module2(kImageBaseIncrement * 2);
+    TestModule stub_module(kImageBaseIncrement);
     unwind_functions_->SetNoRuntimeFunction(&context);
-    EXPECT_FALSE(unwinder->TryUnwind(&context, &stub_module2));
+    EXPECT_FALSE(unwinder->TryUnwind(false, &context, &stub_module));
   }
 }