StackSamplingProfiler clean up

base/profiler/stack_sampling_profiler_win.cc

Index: base/profiler/stack_sampling_profiler_win.cc
diff --git a/base/profiler/stack_sampling_profiler_win.cc b/base/profiler/stack_sampling_profiler_win.cc
index ba46cf076dbd01a26dcaf2d65df3e9c5830dd2dd..453a4a53499f5d03be21e5364b65454cd2bb461f 100644
--- a/base/profiler/stack_sampling_profiler_win.cc
+++ b/base/profiler/stack_sampling_profiler_win.cc
@@ -2,14 +2,13 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "base/profiler/stack_sampling_profiler.h"
+#include <windows.h>
 
-#include <dbghelp.h>
 #include <map>
 #include <utility>
-#include <windows.h>
 
 #include "base/logging.h"
+#include "base/profiler/native_stack_sampler.h"
 #include "base/time/time.h"
 #include "base/win/pe_image.h"
 #include "base/win/scoped_handle.h"
@@ -18,38 +17,7 @@ namespace base {
 
 namespace {
 
-class NativeStackSamplerWin : public StackSamplingProfiler::NativeStackSampler {
- public:
-  explicit NativeStackSamplerWin(win::ScopedHandle thread_handle);
-  ~NativeStackSamplerWin() override;
-
-  // StackSamplingProfiler::NativeStackSampler:
-  void ProfileRecordingStarting(
-      StackSamplingProfiler::Profile* profile) override;
-  void RecordStackSample(StackSamplingProfiler::Sample* sample) override;
-  void ProfileRecordingStopped() override;
-
- private:
-  static bool GetModuleInfo(HMODULE module,
-                            StackSamplingProfiler::Module* module_info);
-
-  void CopyToSample(const void* const instruction_pointers[],
-                    const HMODULE modules[],
-                    int stack_depth,
-                    StackSamplingProfiler::Sample* sample,
-                    std::vector<StackSamplingProfiler::Module>* module_infos);
-
-  win::ScopedHandle thread_handle_;
-  // Weak. Points to the profile being recorded between
-  // ProfileRecordingStarting() and ProfileRecordingStopped().
-  StackSamplingProfiler::Profile* current_profile_;
-  // Maps a module to the module's index within current_profile_->modules.
-  std::map<HMODULE, int> profile_module_index_;
-
-  DISALLOW_COPY_AND_ASSIGN(NativeStackSamplerWin);
-};
-
-// Walk the stack represented by |context| from the current frame downwards,
+// Walks the stack represented by |context| from the current frame downwards,
 // recording the instruction pointers for each frame in |instruction_pointers|.
 int RecordStack(CONTEXT* context,
                 int max_stack_size,
@@ -58,62 +26,60 @@ int RecordStack(CONTEXT* context,
 #ifdef _WIN64
   *last_frame_is_unknown_function = false;
 
-  IMAGEHLP_SYMBOL64 sym;
-  sym.SizeOfStruct = sizeof(sym);
-  sym.MaxNameLength = 0;
-
-  for (int i = 0; i < max_stack_size; ++i) {
+  int i = 0;
+  for (; (i < max_stack_size) && context->Rip; ++i) {
     // Try to look up unwind metadata for the current function.
     ULONG64 image_base;
     PRUNTIME_FUNCTION runtime_function =
         RtlLookupFunctionEntry(context->Rip, &image_base, nullptr);
 
-    instruction_pointers[i] = reinterpret_cast<void*>(context->Rip);
+    instruction_pointers[i] = reinterpret_cast<const void*>(context->Rip);
 
     if (runtime_function) {
       KNONVOLATILE_CONTEXT_POINTERS nvcontext = {0};
       void* handler_data;
       ULONG64 establisher_frame;
       RtlVirtualUnwind(0, image_base, context->Rip, runtime_function, context,
-          &handler_data, &establisher_frame, &nvcontext);
+                       &handler_data, &establisher_frame, &nvcontext);
     } else {
-      // If we don't have a RUNTIME_FUNCTION, then we've encountered
-      // a leaf function.  Adjust the stack appropriately.
+      // If we don't have a RUNTIME_FUNCTION, then we've encountered a leaf
+      // function.  Adjust the stack appropriately prior to the next function
+      // lookup.
       context->Rip = *reinterpret_cast<PDWORD64>(context->Rsp);
       context->Rsp += 8;
       *last_frame_is_unknown_function = true;
     }
-
-    if (!context->Rip)
-      return i;
   }
-  return max_stack_size;
+  return i;
 #else
   return 0;
 #endif
 }
 
 // Fills in |modules| corresponding to the pointers to code in |addresses|. The
-// modules are returned with reference counts incremented should be freed with
-// FreeModules.
+// modules are returned with reference counts incremented and should be freed
+// with FreeModules. See note in SuspendThreadAndRecordStack for why |addresses|
+// and |modules| are arrays.
 void FindModulesForAddresses(const void* const addresses[], HMODULE modules[],
                              int stack_depth,
                              bool last_frame_is_unknown_function) {
-  const int module_frames = last_frame_is_unknown_function ? stack_depth - 1 :
-      stack_depth;
+  const int module_frames =
+      last_frame_is_unknown_function ? stack_depth - 1 : stack_depth;
   for (int i = 0; i < module_frames; ++i) {
     HMODULE module = NULL;
     if (GetModuleHandleEx(GET_MODULE_HANDLE_EX_FLAG_FROM_ADDRESS,
                           reinterpret_cast<LPCTSTR>(addresses[i]),
                           &module)) {
-      // HMODULE is the base address of the module.
-      DCHECK_LT(reinterpret_cast<const void*>(module), addresses[i]);
+      // HMODULE actually represents the base address of the module, so we can
+      // use it directly as an address.
+      DCHECK_LE(reinterpret_cast<const void*>(module), addresses[i]);
       modules[i] = module;
     }
   }
 }
 
-// Free the modules returned by FindModulesForAddresses.
+// Frees the modules returned by FindModulesForAddresses. See note in
+// SuspendThreadAndRecordStack for why |modules| is an array.
 void FreeModules(int stack_depth, HMODULE modules[]) {
   for (int i = 0; i < stack_depth; ++i) {
     if (modules[i])
@@ -141,94 +107,102 @@ ScopedDisablePriorityBoost::ScopedDisablePriorityBoost(HANDLE thread_handle)
       boost_state_was_disabled_(false) {
   got_previous_boost_state_ =
       ::GetThreadPriorityBoost(thread_handle_, &boost_state_was_disabled_);
-  if (got_previous_boost_state_ && !boost_state_was_disabled_) {
-    // Confusingly, TRUE disables priority boost ...
+  if (got_previous_boost_state_) {
+    // Confusingly, TRUE disables priority boost.
     ::SetThreadPriorityBoost(thread_handle_, TRUE);
   }
 }
 
 ScopedDisablePriorityBoost::~ScopedDisablePriorityBoost() {
-  if (got_previous_boost_state_ && !boost_state_was_disabled_) {
-    // ... and FALSE enables priority boost.
-    ::SetThreadPriorityBoost(thread_handle_, FALSE);
-  }
+  if (got_previous_boost_state_)
+    ::SetThreadPriorityBoost(thread_handle_, boost_state_was_disabled_);
 }
 
 // Suspends the thread with |thread_handle|, records the stack into
 // |instruction_pointers|, then resumes the thread. Returns the size of the
 // stack.
+//
+// IMPORTANT NOTE: No heap allocations may occur between SuspendThread and
+// ResumeThread. Otherwise this code can deadlock on heap locks acquired by the
+// target thread before it was suspended. This is why we pass instruction
+// pointers and modules as preallocated arrays rather than vectors, since
+// vectors make it too easy to subtly allocate memory.
 int SuspendThreadAndRecordStack(HANDLE thread_handle, int max_stack_size,
                                 const void* instruction_pointers[],
                                 bool* last_frame_is_unknown_function) {
-#if defined(_WIN64)
-  if (RtlVirtualUnwind == nullptr || RtlLookupFunctionEntry == nullptr)
-    return 0;
-#endif
-
-  if (::SuspendThread(thread_handle) == -1) {
-    LOG(ERROR) << "SuspendThread failed: " << GetLastError();
+  if (::SuspendThread(thread_handle) == -1)
     return 0;
-  }
 
+  int stack_depth = 0;
   CONTEXT thread_context = {0};
   thread_context.ContextFlags = CONTEXT_FULL;
-  if (!::GetThreadContext(thread_handle, &thread_context)) {
-    LOG(ERROR) << "GetThreadContext failed: " << GetLastError();
+  if (::GetThreadContext(thread_handle, &thread_context)) {
+    stack_depth = RecordStack(&thread_context, max_stack_size,
+                              instruction_pointers,
+                              last_frame_is_unknown_function);
   }
 
-  int stack_depth = RecordStack(&thread_context, max_stack_size,
-                                instruction_pointers,
-                                last_frame_is_unknown_function);
-
-  {
-    ScopedDisablePriorityBoost disable_priority_boost(thread_handle);
-    if (::ResumeThread(thread_handle) == -1)
-      LOG(ERROR) << "ResumeThread failed: " << GetLastError();
-  }
+  // Disable the priority boost that the thread would otherwise receive on
+  // resume. We do this to avoid artificially altering the dynamics of the
+  // executing application any more than we already are by suspending and
+  // resuming the thread.
+  ScopedDisablePriorityBoost disable_priority_boost(thread_handle);
+  bool resume_thread_succeeded = ::ResumeThread(thread_handle) != -1;
+  CHECK(resume_thread_succeeded) << "ResumeThread failed: " << GetLastError();
 
   return stack_depth;
 }
 
-}  // namespace
+class NativeStackSamplerWin : public NativeStackSampler {
+ public:
+  explicit NativeStackSamplerWin(win::ScopedHandle thread_handle);
+  ~NativeStackSamplerWin() override;
 
-scoped_ptr<StackSamplingProfiler::NativeStackSampler>
-StackSamplingProfiler::NativeStackSampler::Create(PlatformThreadId thread_id) {
-#if _WIN64
-  // Get the thread's handle.
-  HANDLE thread_handle = ::OpenThread(
-      THREAD_GET_CONTEXT | THREAD_SUSPEND_RESUME | THREAD_QUERY_INFORMATION,
-      FALSE,
-      thread_id);
-  DCHECK(thread_handle) << "OpenThread failed";
+  // StackSamplingProfiler::NativeStackSampler:
+  void ProfileRecordingStarting(
+      StackSamplingProfiler::CallStackProfile* profile) override;
+  void RecordStackSample(StackSamplingProfiler::Sample* sample) override;
+  void ProfileRecordingStopped() override;
 
-  return scoped_ptr<NativeStackSampler>(new NativeStackSamplerWin(
-      win::ScopedHandle(thread_handle)));
-#else
-  return scoped_ptr<NativeStackSampler>();
-#endif
-}
+ private:
+  // Attempts to query the module filename, base address, and id and store them
+  // in |module_info|. Returns true if it succeeded.

[Peter Kasting, 2015/03/31 01:51:26]

Nit: You use "module_info" quite a bit in this file in place of "module", which
is used elsewhere.  It would be nice to be consistent.  In particular,
pluralizing this as "module_infos" seems a bit awkward compared to "modules".

I suspect this is all due to using "module[s]" for HMODULEs and arrays thereof. 
I would suggest calling the latter "module_handle[s]" instead.  In function
names (e.g. FreeModules()), you could either use ModuleHandles or just Handles.

[Mike Wittman, 2015/03/31 19:29:41]

Done.

+  static bool GetModuleInfo(HMODULE module,
+                            StackSamplingProfiler::Module* module_info);
+
+  // Gets the module index for |module| in |modules|, adding it if it's not
+  // already present. Returns StackSamplingProfiler::Frame::kUnknownModuleIndex
+  // if no Module can be determined for |module|.
+  size_t GetModuleIndex(HMODULE module,
+                        std::vector<StackSamplingProfiler::Module>* modules);
+
+  // Copies the stack information represented by |instruction_pointers| into
+  // |sample| and |module_infos|.
+  void CopyToSample(const void* const instruction_pointers[],
+                    const HMODULE modules[],
+                    int stack_depth,
+                    StackSamplingProfiler::Sample* sample,
+                    std::vector<StackSamplingProfiler::Module>* module_infos);
+
+  win::ScopedHandle thread_handle_;
+  // Weak. Points to the profile being recorded between
+  // ProfileRecordingStarting() and ProfileRecordingStopped().
+  StackSamplingProfiler::CallStackProfile* current_profile_;
+  // Maps a module to the module's index within current_profile_->modules.
+  std::map<HMODULE, size_t> profile_module_index_;
+
+  DISALLOW_COPY_AND_ASSIGN(NativeStackSamplerWin);
+};
 
 NativeStackSamplerWin::NativeStackSamplerWin(win::ScopedHandle thread_handle)
     : thread_handle_(thread_handle.Take()) {
-#ifdef _WIN64
-  if (RtlVirtualUnwind == nullptr && RtlLookupFunctionEntry == nullptr) {
-    const HMODULE nt_dll_handle = ::GetModuleHandle(L"ntdll.dll");
-    // This should always be non-null, but handle just in case.
-    if (nt_dll_handle) {
-      reinterpret_cast<void*&>(RtlVirtualUnwind) =
-          ::GetProcAddress(nt_dll_handle, "RtlVirtualUnwind");
-      reinterpret_cast<void*&>(RtlLookupFunctionEntry) =
-          ::GetProcAddress(nt_dll_handle, "RtlLookupFunctionEntry");
-    }
-  }
-#endif
 }
 
 NativeStackSamplerWin::~NativeStackSamplerWin() {
 }
 
 void NativeStackSamplerWin::ProfileRecordingStarting(
-    StackSamplingProfiler::Profile* profile) {
+    StackSamplingProfiler::CallStackProfile* profile) {
   current_profile_ = profile;
   profile_module_index_.clear();
 }
@@ -273,16 +247,31 @@ bool NativeStackSamplerWin::GetModuleInfo(
   GUID guid;
   DWORD age;
   win::PEImage(module).GetDebugId(&guid, &age);
-  module_info->id.insert(module_info->id.end(),
-                         reinterpret_cast<char*>(&guid),
-                         reinterpret_cast<char*>(&guid + 1));
-  module_info->id.insert(module_info->id.end(),
-                         reinterpret_cast<char*>(&age),
-                         reinterpret_cast<char*>(&age + 1));
+  module_info->id.assign(reinterpret_cast<char*>(&guid), sizeof(guid));
+  module_info->id.append(reinterpret_cast<char*>(&age), sizeof(age));
 
   return true;
 }
 
+size_t NativeStackSamplerWin::GetModuleIndex(
+    HMODULE module,
+    std::vector<StackSamplingProfiler::Module>* modules) {
+  if (!module)
+    return StackSamplingProfiler::Frame::kUnknownModuleIndex;
+
+  auto loc = profile_module_index_.find(module);
+  if (loc == profile_module_index_.end()) {
+    StackSamplingProfiler::Module module_info;
+    if (!GetModuleInfo(module, &module_info))
+      return StackSamplingProfiler::Frame::kUnknownModuleIndex;
+    modules->push_back(module_info);
+    loc = profile_module_index_.insert(std::make_pair(
+        module, modules->size() - 1)).first;
+  }
+
+  return loc->second;
+}
+
 void NativeStackSamplerWin::CopyToSample(
     const void* const instruction_pointers[],
     const HMODULE modules[],
@@ -293,33 +282,28 @@ void NativeStackSamplerWin::CopyToSample(
   sample->reserve(stack_depth);
 
   for (int i = 0; i < stack_depth; ++i) {
-    sample->push_back(StackSamplingProfiler::Frame());
-    StackSamplingProfiler::Frame& frame = sample->back();
-
-    frame.instruction_pointer = instruction_pointers[i];
+    sample->push_back(StackSamplingProfiler::Frame(
+        instruction_pointers[i], GetModuleIndex(modules[i], module_infos)));
+  }
+}
 
-    // Record an invalid module index if we don't have a valid module.
-    if (!modules[i]) {
-      frame.module_index = -1;
-      continue;
-    }
+}  // namespace
 
-    auto loc = profile_module_index_.find(modules[i]);
-    if (loc == profile_module_index_.end()) {
-      StackSamplingProfiler::Module module_info;
-      // Record an invalid module index if we have a module but can't find
-      // information on it.
-      if (!GetModuleInfo(modules[i], &module_info)) {
-        frame.module_index = -1;
-        continue;
-      }
-      module_infos->push_back(module_info);
-      loc = profile_module_index_.insert(std::make_pair(
-          modules[i], static_cast<int>(module_infos->size() - 1))).first;
-    }
+scoped_ptr<NativeStackSampler> NativeStackSampler::Create(
+    PlatformThreadId thread_id) {
+#if _WIN64
+  // Get the thread's handle.
+  HANDLE thread_handle = ::OpenThread(
+      THREAD_GET_CONTEXT | THREAD_SUSPEND_RESUME | THREAD_QUERY_INFORMATION,
+      FALSE,
+      thread_id);
 
-    frame.module_index = loc->second;
+  if (thread_handle) {
+    return scoped_ptr<NativeStackSampler>(new NativeStackSamplerWin(
+        win::ScopedHandle(thread_handle)));
   }
+#endif
+  return scoped_ptr<NativeStackSampler>();
 }
 
 }  // namespace base