StackSamplingProfiler clean up

base/profiler/stack_sampling_profiler_unittest.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 <sstream>
 
 #include "base/bind.h"
 #include "base/compiler_specific.h"
 #include "base/path_service.h"
 #include "base/profiler/stack_sampling_profiler.h"
 #include "base/synchronization/waitable_event.h"
 #include "base/threading/platform_thread.h"
 #include "base/time/time.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace base {
 
 using Frame = StackSamplingProfiler::Frame;
 using Module = StackSamplingProfiler::Module;
 using Sample = StackSamplingProfiler::Sample;
 using Profile = StackSamplingProfiler::Profile;
 
 namespace {
 // A thread to target for profiling, whose stack is guaranteed to contain

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

Nit: Blank line above

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

Done.

 // SignalAndWaitUntilSignaled() when coordinated with the main thread.
 class TargetThread : public PlatformThread::Delegate {
  public:
   TargetThread();
 
   // Implementation of PlatformThread::Delegate:

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

Nit: "Implementation of" is not really necessary

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

Done.

   void ThreadMain() override;
 
   // Wait for the thread to have started and be executing in
   // SignalAndWaitUntilSignaled().
   void WaitForThreadStart();
   // Allow the thread to return from SignalAndWaitUntilSignaled() and finish

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

Nit: Blank line above

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

Done.

   // execution.
   void SignalThreadToFinish();
 
   // This function is guaranteed to be executing between calls to
   // WaitForThreadStart() and SignalThreadToFinish().

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

You should probably add to this comment the reason why it's a static instead of
a normal member (presumably, so we can more easily compare its address in one of
the tests below).

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

Done.

   static void SignalAndWaitUntilSignaled(WaitableEvent* thread_started_event,
                                          WaitableEvent* finish_event);
 
   PlatformThreadId id() const { return id_; }
 
  private:
   WaitableEvent thread_started_event_;
   WaitableEvent finish_event_;
   PlatformThreadId id_;
 
@@ skipping 68 matching lines @@
   target_thread.SignalThreadToFinish();
 
   PlatformThread::Join(target_thread_handle);
 }
 
 // If this executable was linked with /INCREMENTAL (the default for non-official
 // debug and release builds on Windows), function addresses do not correspond to
 // function code itself, but instead to instructions in the Incremental Link
 // Table that jump to the functions. Check for a jump instruction and if present
 // do a little decompilation to find the function's actual starting address.
 const void* MaybeFixupFunctionAddressForILT(const void* function_address) {

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

Nit: If you take and return char*s instead of void*s, you can avoid a cast in
FindFirstFrameWithinFunction().

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

I'd somewhat prefer to keep these interfaces using const void*, for two reasons:
 1. it's consistent with the StackSamplingProfiler interfaces that use const
void* for memory addresses
 2. the need to interpret the memory addresses as char/int32 pointers in order
to do pointer arithmetic is essentially an implementation detail of the
functions

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

Fair enough.

 #if defined(_WIN64)
   const unsigned char* opcode =
       reinterpret_cast<const unsigned char*>(function_address);
   if (*opcode == 0xe9) {
     // This is a relative jump instruction. Assume we're in the ILT and compute
     // the function start address from the instruction offset.
     const unsigned char* offset = opcode + 1;

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

Nit: Why not declare this as type const int32* and cast here?  Then you can do
"next_instruction = offset + 1;" in the next line, which is a little less magic
than "5".

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

Done.

     const unsigned char* next_instruction = opcode + 5;
     return next_instruction +
         static_cast<int64>(*reinterpret_cast<const int32*>(offset));

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

Why do we need the int64 cast?  AFAICT it does nothing.

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

We don't. I was following the instruction set specification which calls for
sign-extending the 32 bit offset to a 64 bit offset, but that's done implicitly
by the compiler of course.

   }
 #endif
   return function_address;
 }
 
 // Searches through the frames in |sample|, returning an iterator to the first
 // frame that has an instruction pointer between |function_address| and
 // |function_address| + |size|. Returns sample.end() if no such frames are
 // found.
 Sample::const_iterator FindFirstFrameWithinFunction(
     const Sample& sample,
     const void* function_address,
     int function_size) {
   function_address = MaybeFixupFunctionAddressForILT(function_address);
   for (auto it = sample.begin(); it != sample.end(); ++it) {
     if ((reinterpret_cast<const unsigned char*>(it->instruction_pointer) >=

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

I don't think casting the instruction_pointer to char* is necessary in either
comparison.  Even if it is, you can static_cast it.

You can also avoid casting the function_address in the first comparison; in the
second one you can avoid the cast if you follow my suggestion above to use char*
instead of void* in MaybeFixupFunctionAddressForILT() (and, transitively, here.)

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

That's correct, we only need to cast to support the pointer arithmetic. Why
static_cast though? As far as I can tell the behavior of static_cast and
reinterpret_cast are equally undefined in this case. :)
See my response above.

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

My rule-of-thumb is that if static_cast does the right thing, prefer it to
reinterpret_cast.  Its functionality is generally weaker -- you are less able to
accidentally do bad things with it -- and before C++11, it was the only legal
choice when casting to/from void*.  In C++11, both casts are allowed, but
reinterpret_cast is defined in terms of static_cast, which suggests to me that
static_cast maps better to the "underlying transformation" than reinterpret_cast
does.

It's a super-nitpicky detail, though.

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

Sounds reasonable. Changed to static_cast.

          reinterpret_cast<const unsigned char*>(function_address)) &&
         (reinterpret_cast<const unsigned char*>(it->instruction_pointer) <
          (reinterpret_cast<const unsigned char*>(function_address) +
           function_size)))
       return it;
   }
   return sample.end();
 }
 
 // Formats a sample into a string that can be output for test diagnostics.
 std::string FormatSampleForDiagnosticOutput(
     const Sample& sample,
     const std::vector<Module>& modules) {
   std::ostringstream stream;

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

ostringstream seems like overkill here.

Something like this would probably work:

  std::string output;
  for (const Frame& frame : sample) {
    output += StringPrintf("0x%p %s\n", frame.instruction_pointer,
                            modules[frame.module_index].filename.AsUTF8Unsafe())
  }
  return output;

Streams are also discouraged in general ("Do not use streams, except where
required by a logging interface," says
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Streams ), and
since their use isn't "required" here, I would avoid them.

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

Done.

   for (const Frame& frame: sample) {
     stream << frame.instruction_pointer << " "
            << modules[frame.module_index].filename.value() << std::endl;
   }
   return stream.str();
 }
 
 // Returns a duration that is longer than the test timeout. We would use
 // TimeDelta::Max() but https://crbug.com/465948.
 TimeDelta AVeryLongTimeDelta() { return TimeDelta::FromDays(1); }
 }  // namespace

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

Nit: Blank line above

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

Done.

 
 
 // The tests below are enabled for Win x64 only, pending implementation of the
 // tested functionality on other platforms/architectures.

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

It might be more readable if you simply bracketed this whole file in a single
#if, then, instead of disabling each test.

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

Probably. But I'd prefer to keep these tests compiling on all platforms since we
hope to eventually support this functionality everywhere.

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

OK.  I hope "eventually" is soon :)

 
 // Checks that the basic expected information is present in a sampled profile.
 #if defined(_WIN64)
 #define MAYBE_Basic Basic
 #else
 #define MAYBE_Basic DISABLED_Basic
 #endif
 TEST(StackSamplingProfilerTest, MAYBE_Basic) {
   StackSamplingProfiler::SamplingParams params;

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

Enough of these tests want to set most/all of the params that I would consider
adding one or more n-arg constructors to SamplingParams.  (You can use delegated
constructors to minimize the code footprint.)

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

I'm setting most of the params intentionally (even ones with the same values as
the defaults) to try to make the code more self-documenting and ensure the
relevant state is clearly specified in the tests. This won't scale to a larger
number of params, but I'd prefer to keep it this way until it's no longer
sustainable.

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

Hmm.

That isn't strictly against the style guide, so you're welcome to do what you
like.  That said, the idea of re-setting members to their default values in
particular makes me a little nervous; having default values for things means
code that changes other members can just change the members that need to be
non-default, which calls those to the reader's attention.  When all values are
set, those values are buried in the noise of the other values also being set, so
everything looks equally important, which may not be the case.

Worse, as soon as you add another member to the struct, all the above places no
longer set it explicitly, and you have to manually find and update them with no
help from the compiler, which makes it very likely you'll miss one.  Then in
some sense you have the worst of both worlds, because you're neither setting
everything nor setting only the non-default things.

If you want to be explicit, I might add a single constructor that requires
arguments for all members, and have no default constructor.  Then your callsites
still have to pass all params, but if you add a new member and thus change the
constructor signature, the compiler will point out every caller.  Admittedly
this does not give you member names at every callsite, but you can either add
them as EOL comments to the constructor call args, or (what most Chromium code
does) simply rely on the reader to glance at the constructor definition if they
need to know arg order, much as people do with other functions.

Up to you.

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

Looking through this more closely, many of the values that are set don't
actually affect their tests, so I've just removed them.

   params.initial_delay = params.burst_interval = params.sampling_interval =
       TimeDelta::FromMilliseconds(0);
   params.bursts = 1;
   params.samples_per_burst = 1;
 
   std::vector<Profile> profiles;
   CaptureProfiles(params, &profiles, AVeryLongTimeDelta());
 
   // Check that the profile and samples sizes are correct, and the module
   // indices are in range.
 

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

Nit: I'd remove this blank line and the one on line 233, and put an empty "  //"
on line 219.  This makes sure each block comment directly abuts the code it
describes.

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

Done.

   ASSERT_EQ(1u, profiles.size());
   const Profile& profile = profiles[0];
   ASSERT_EQ(1u, profile.samples.size());
   EXPECT_EQ(params.sampling_interval, profile.sampling_period);
   const Sample& sample = profile.samples[0];
   for (const auto& frame : sample) {
     ASSERT_GE(frame.module_index, 0);
     ASSERT_LT(frame.module_index, static_cast<int>(profile.modules.size()));
   }
 
   // Check that the stack contains a frame for
   // TargetThread::SignalAndWaitUntilSignaled() and that the frame has this
   // executable's module.
 
   // Since we don't have a good way to know the function size, use 100 bytes as
   // a reasonable window to locate the instruction pointer.
   Sample::const_iterator loc = FindFirstFrameWithinFunction(
       sample,
       reinterpret_cast<const void*>(&TargetThread::SignalAndWaitUntilSignaled),
       100);
   ASSERT_TRUE(loc != sample.end())
       << "Function at "
       << MaybeFixupFunctionAddressForILT(
           reinterpret_cast<const void*>(
               &TargetThread::SignalAndWaitUntilSignaled))
       << " was not found in stack:" << std::endl

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

Nit: I'd normally use \n over std::endl, since you don't actually need the
"flush stream" behavior endl gives.

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

Done.

       << FormatSampleForDiagnosticOutput(sample, profile.modules);
 
   FilePath executable_path;
   bool got_executable_path = PathService::Get(FILE_EXE, &executable_path);

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

Nit: Just inline this into the EXPECT_TRUE below.

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

Done.

   EXPECT_TRUE(got_executable_path);
   EXPECT_EQ(executable_path, profile.modules[loc->module_index].filename);
 }
 
 // Checks that the expected number of profiles and samples are present in the
 // profiles produced.
 #if defined(_WIN64)
 #define MAYBE_MultipleProfilesAndSamples MultipleProfilesAndSamples
 #else
 #define MAYBE_MultipleProfilesAndSamples DISABLED_MultipleProfilesAndSamples
@@ skipping 69 matching lines @@
   params.bursts = 1;
   params.samples_per_burst = 2;
 
   std::vector<Profile> profiles;
   CaptureProfiles(params, &profiles, TimeDelta::FromMilliseconds(50));
 
   EXPECT_TRUE(profiles.empty());
 }
 
 }  // namespace tracked_objects
+