StackSamplingProfiler clean up

base/profiler/stack_sampling_profiler_win.cc

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // 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 <dbghelp.h>
 #include <map>
 #include <utility>
 #include <windows.h>

[Peter Kasting, 2015/03/26 04:29:08]

I would place this with dbghelp.h and then put a blank between it and the C++
system headers, since it seems like a C system header; see
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Names_and_Ord...
.

[Mike Wittman, 2015/03/27 22:42:04]

Done.

 
 #include "base/logging.h"
 #include "base/time/time.h"
 #include "base/win/pe_image.h"
 #include "base/win/scoped_handle.h"
 
 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,

[Peter Kasting, 2015/03/26 04:29:09]

These functions need descriptive comments.

[Mike Wittman, 2015/03/27 22:42:04]

Done.

                             StackSamplingProfiler::Module* module_info);
 
   void CopyToSample(const void* const instruction_pointers[],

[Peter Kasting, 2015/03/26 04:29:08]

You use a lot of arrays in this file, which is a bit unusual.  Is there a
particular reason to use these over vectors in most places?  It seems like using
a vector would be safer (harder to read/write off the end accidentally) and
typically use slightly less memory (if you didn't need the full maximum array
sizes in all cases).

Using a container instead of an array would also allow for more use of
range-based for and the like in places where you currently have an integral loop
index.

[Mike Wittman, 2015/03/27 22:42:05]

Yes, the reason is that we can't do any memory allocation between SuspendThread
and ResumeThread in SuspendThreadAndRecordStack, otherwise we risk deadlocking
on heap locks acquired by the target thread before SuspendThread. We could make
do with vector with a reserve beforehand, but I don't see that as an advantage
-- we just trade the familiar concern of reading/writing off the end for the
obscure concern of whether a given call will result in a memory allocation.

I realized this isn't documented anywhere, so I added a note to
SuspendThreadAndRecordStack.

[Peter Kasting, 2015/03/27 23:44:46]

Ahhh... that's a good reason (and a scary one; there's no easy way to prevent
someone in the future from adding code that allocates memory).

You might want to mention |modules| in your comment as well, and possibly add a
note on FindModulesForAddresses() that readers should see the note on
uspendThreadAndRecordStack() for why the args for it and FreeModules() are
arrays.

[Mike Wittman, 2015/03/30 21:01:13]

Done.

                     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_;

[Peter Kasting, 2015/03/26 04:29:08]

Note that if you change Frame::module_index to a size_t as I suggest you'll also
need to change this map.

[Mike Wittman, 2015/03/27 22:42:04]

Done.

 
   DISALLOW_COPY_AND_ASSIGN(NativeStackSamplerWin);
 };

[Peter Kasting, 2015/03/26 04:29:08]

If possible, try to put the declaration of this class next to its function
definitions.

Maybe this could be done by moving both of these to the bottom of the anonymous
namespace?

[Mike Wittman, 2015/03/27 22:42:05]

Done.

 
 // Walk 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,
                 const void* instruction_pointers[],
                 bool* last_frame_is_unknown_function) {
 #ifdef _WIN64
   *last_frame_is_unknown_function = false;
 
   IMAGEHLP_SYMBOL64 sym;
   sym.SizeOfStruct = sizeof(sym);

[Peter Kasting, 2015/03/26 04:29:09]

Nit: While not required, I tend to initialize Windows structs as follows, since
all the ones with a cbSize (or similar) member seem to put it at the beginning:

  IMAGEHLP_SYMBOL64 sym = { sizeof(sym) };

This is shorter and, at least for most WIndows programmers, not less intuitive
(I hope).

I believe this should also ensure your MaxNameLength field gets initialized to
0.

[Mike Wittman, 2015/03/27 22:42:05]

This struct is actually unused, so I just removed it (and the associated
dbghelp.h header). Thanks for the tip though.

   sym.MaxNameLength = 0;
 
   for (int i = 0; i < max_stack_size; ++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);

[Peter Kasting, 2015/03/26 04:29:09]

It's not obvious to me whether it's correct that this happens before the "else"
arm below adjusts this value.

[Mike Wittman, 2015/03/27 22:42:05]

It is correct based on observation of collected stacks in the debugger.
Clarified the comment in the else clause.

 
     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);

[Peter Kasting, 2015/03/26 04:29:09]

Subsequent lines of args must be aligned with the first line's args; see
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Calls .

[Mike Wittman, 2015/03/27 22:42:05]

Done.

     } else {
       // If we don't have a RUNTIME_FUNCTION, then we've encountered
       // a leaf function.  Adjust the stack appropriately.
       context->Rip = *reinterpret_cast<PDWORD64>(context->Rsp);
       context->Rsp += 8;
       *last_frame_is_unknown_function = true;
     }
 
     if (!context->Rip)

[Peter Kasting, 2015/03/26 04:29:08]

We always do one iteration of the loop before checking this.  What if it's null
on entry?  Is that a problem?  Seems like that would likely go into the "else"
arm above which will dereference Rsp.  Do we need to add any DCHECKs atop the
function that one or both of these are non-null to make it clear what's safe for
callers to do?

Nit: Depending on the answer to this, you might be able to change the loop to
this slightly simpler form:

  int i = 0;
  for (; (i < max_stack_size) && context->Rip; ++i) {
    ...
    // No conditional here
  }
  return i;

[Mike Wittman, 2015/03/27 22:42:04]

I'm inclined to check it beforehand and use your suggested approach. That way
we're making fewer assumptions about the result of the prior GetThreadContext
call.

       return i;
   }
   return max_stack_size;
 #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.
 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;

[Peter Kasting, 2015/03/26 04:29:08]

Nit: This wrapping is a bit unusual; consider something like:

  const int module_frames =
      last_frame_is_unknown_function ? stack_depth - 1 : stack_depth;

(clang-format would have its own ideas about how to format this that would also
be acceptable, though I personally dislike them :) )

[Mike Wittman, 2015/03/27 22:42:05]

Done.

   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.

[Peter Kasting, 2015/03/26 04:29:08]

I'm not actually sure what this comment is adding.  I think HMODULE is meant to
be |module|, at which point the comment seems kind of vacuous, but maybe I'm
misunderstanding.

[Mike Wittman, 2015/03/27 22:42:05]

It was intended to document that the value of an HMODULE is the same as the base
address of the module it represents. I've updated the comment to hopefully be
more clear.

[Peter Kasting, 2015/03/27 23:44:46]

Yes, that makes more sense.

       DCHECK_LT(reinterpret_cast<const void*>(module), addresses[i]);

[Peter Kasting, 2015/03/26 04:29:09]

Should this be LE instead of LT?  Are we guaranteed that addresses[i] != module?

[Mike Wittman, 2015/03/27 22:42:04]

It should be LE. Fixed.

       modules[i] = module;
     }
   }
 }
 
 // Free the modules returned by FindModulesForAddresses.
 void FreeModules(int stack_depth, HMODULE modules[]) {
   for (int i = 0; i < stack_depth; ++i) {
     if (modules[i])
       ::FreeLibrary(modules[i]);
@@ skipping 13 matching lines @@
 
   DISALLOW_COPY_AND_ASSIGN(ScopedDisablePriorityBoost);
 };
 
 ScopedDisablePriorityBoost::ScopedDisablePriorityBoost(HANDLE thread_handle)
     : thread_handle_(thread_handle),
       got_previous_boost_state_(false),
       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_) {

[Peter Kasting, 2015/03/26 04:29:08]

Nit: Technically, the second part of the conditional doesn't matter, unless
calling the kernel function here might be expensive even when it no-ops.

(This expense concern should be considered in the context of my next comment as
well.)

[Mike Wittman, 2015/03/27 22:42:04]

I don't have reason to believe this function is expensive. Also as far as I
could tell, priority boost is always enabled in Chrome so we would always run
this anyway. Removed the second part of the conditional.

     // 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.

[Peter Kasting, 2015/03/26 04:29:08]

Nit: This comment would be unnecessary (because the code would be more obvious)
if you did this:

  if (got_previous_boost_state_)
    ::SetThreadPriorityBoost(thread_handle_, boost_state_was_disabled_);

[Mike Wittman, 2015/03/27 22:42:05]

Done.

     ::SetThreadPriorityBoost(thread_handle_, FALSE);
   }
 }
 
 // Suspends the thread with |thread_handle|, records the stack into
 // |instruction_pointers|, then resumes the thread. Returns the size of the
 // stack.
 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)

[Peter Kasting, 2015/03/26 04:29:08]

Tiny nit: Personally I prefer the brevity of "!foo" over "foo == nullptr", but
up to you.  (Multiple places)

[Mike Wittman, 2015/03/27 22:42:04]

Done.

     return 0;
 #endif
 
   if (::SuspendThread(thread_handle) == -1) {
     LOG(ERROR) << "SuspendThread failed: " << GetLastError();
     return 0;
   }
 
   CONTEXT thread_context = {0};
   thread_context.ContextFlags = CONTEXT_FULL;
   if (!::GetThreadContext(thread_handle, &thread_context)) {
     LOG(ERROR) << "GetThreadContext failed: " << GetLastError();

[Peter Kasting, 2015/03/26 04:29:08]

Do we need to return 0 here, or is calling RecordStack correct?

[Mike Wittman, 2015/03/27 22:42:05]

Presumably we don't have a valid CONTEXT at this point, so we probably should
just resume the thread and return 0.

   }
 
   int stack_depth = RecordStack(&thread_context, max_stack_size,
                                 instruction_pointers,
                                 last_frame_is_unknown_function);
 
   {
     ScopedDisablePriorityBoost disable_priority_boost(thread_handle);

[Peter Kasting, 2015/03/26 04:29:08]

While I like scoping objects, if this is the only use of
ScopedDisablePriorityBoost, I think it might be better to go ahead and just
inline its contents into this block -- it's basically just two function calls --
unless you intend to use it at other places in this file in the future.

It's also not clear why you need to disable the priority boost here; add
comments explaining this.

[Mike Wittman, 2015/03/27 22:42:05]

Done.

     if (::ResumeThread(thread_handle) == -1)
       LOG(ERROR) << "ResumeThread failed: " << GetLastError();
   }
 
   return stack_depth;
 }
 
 }  // namespace
 
 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";

[Peter Kasting, 2015/03/26 04:29:09]

Can the call ever actually fail?

If so, a DCHECK isn't appropriate here.  DCHECK means "can't ever fail even in
exceptional conditions".  We would want to handle this somehow, e.g. by
returning null.

[Mike Wittman, 2015/03/27 22:42:05]

Presumably this can fail if the thread exits before this function is invoked.
Updated to return null in that case.

 
   return scoped_ptr<NativeStackSampler>(new NativeStackSamplerWin(
       win::ScopedHandle(thread_handle)));
 #else
   return scoped_ptr<NativeStackSampler>();
 #endif
 }
 
 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) =

[Peter Kasting, 2015/03/26 04:29:08]

I'm confused.  Where is this declared?  I thought you were calling a kernel
function, but here you're treating it as if you've declared a function pointer
variable locally somewhere that points to the proc address.  But there's no such
declaration I can see.

Without this being declared as a variable, I can't see how this would actually
do what you want it to do at runtime.

If the rest of the file compiles and links, I think it means this function is
known to the linker, and you should just nuke this entire block (and the shorter
one earlier).

[Mike Wittman, 2015/03/27 22:42:04]

To be honest, I'm not sure why these checks are here either... This part of the
code was inherited from someone else's prototype. I'll remove the blocks.

[Peter Kasting, 2015/03/27 23:44:46]

\o/

           ::GetProcAddress(nt_dll_handle, "RtlVirtualUnwind");
       reinterpret_cast<void*&>(RtlLookupFunctionEntry) =
           ::GetProcAddress(nt_dll_handle, "RtlLookupFunctionEntry");
     }
   }
 #endif
 }
 
 NativeStackSamplerWin::~NativeStackSamplerWin() {
 }
@@ skipping 37 matching lines @@
   if (result_length == 0)
     return false;
 
   module_info->filename = base::FilePath(module_name);
 
   module_info->base_address = reinterpret_cast<const void*>(module);
 
   GUID guid;
   DWORD age;
   win::PEImage(module).GetDebugId(&guid, &age);
   module_info->id.insert(module_info->id.end(),

[Peter Kasting, 2015/03/26 04:29:08]

How come you're appending to |id| here instead of resetting it?  No callers
currently call this with an existing string here, but if they did, it seems like
this would be wrong.

Plus, the way these calls use adds and casts is a bit hard-to-read and
error-prone, and insert() is unnecessarily verbose (and possibly inefficient). 
How about:

  module_info->id.assign(reinterpret_cast<char*>(&guid), sizeof(guid));
  module_info->id.append(reinterpret_cast<char*>(&age), sizeof(age));

[Mike Wittman, 2015/03/27 22:42:05]

Sure, that's much clearer.

                          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));
 
   return true;
 }
 
 void NativeStackSamplerWin::CopyToSample(
     const void* const instruction_pointers[],
     const HMODULE modules[],
     int stack_depth,
     StackSamplingProfiler::Sample* sample,
     std::vector<StackSamplingProfiler::Module>* module_infos) {
   sample->clear();
   sample->reserve(stack_depth);
 
   for (int i = 0; i < stack_depth; ++i) {
     sample->push_back(StackSamplingProfiler::Frame());

[Peter Kasting, 2015/03/26 04:29:08]

The reason we need a push_back() and then non-const ref to the back element here
is because of the continues below.

If instead you have a private member helper function like:

  int GetModuleIndex(HMODULE module,
                     std::vector<StackSamplingProfiler::Module>* module_infos) {
    if (!module)
      return -1;
    auto loc = ...
    ... rest of code to possibly add to |module_infos| and compute the index
  }

...then the function above needs no continues, and this loop becomes very
simple:

  for (...) {
    sample->push_back(StackSamplingProfiler::Frame(
        instruction_pointers[i], GetModuleIndex(modules[i], module_infos)));
  }

(Note that the above requires adding a 2-arg constructor for Frame, or better
yet, replacing the existing 0-arg constructor with a 2-arg form.)

[Mike Wittman, 2015/03/27 22:42:04]

Done.

     StackSamplingProfiler::Frame& frame = sample->back();
 
     frame.instruction_pointer = instruction_pointers[i];
 
     // Record an invalid module index if we don't have a valid module.
     if (!modules[i]) {
       frame.module_index = -1;
       continue;
     }
 
     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;
     }
 
     frame.module_index = loc->second;
   }
 }
 
 }  // namespace base
+