Type change in StackSamplingProfiler from void* to uintptr_t.

base/profiler/stack_sampling_profiler.h

 // 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.
 
 #ifndef BASE_PROFILER_STACK_SAMPLING_PROFILER_H_
 #define BASE_PROFILER_STACK_SAMPLING_PROFILER_H_
 
 #include <string>
 #include <vector>
 
@@ skipping 46 matching lines @@
 // The results of the profiling are passed to the completed callback and consist
 // of a vector of CallStackProfiles. Each CallStackProfile corresponds to a
 // burst as specified in SamplingParams and contains a set of Samples and
 // Modules. One Sample corresponds to a single recorded stack, and the Modules
 // record those modules associated with the recorded stack frames.
 class BASE_EXPORT StackSamplingProfiler {
  public:
   // Module represents the module (DLL or exe) corresponding to a stack frame.
   struct BASE_EXPORT Module {
     Module();
     Module(const void* base_address, const std::string& id,

[Mike Wittman, 2015/08/31 21:42:00]

The |base_address| parameter should use the same type as is stored in the
struct.

[sydli, 2015/09/01 00:29:59]

Done.

            const FilePath& filename);
     ~Module();
 
     // Points to the base address of the module.
-    const void* base_address;
+    uintptr_t base_address;

[Mike Wittman, 2015/08/31 20:57:57]

I'd prefer to keep the const void* type for this and instruction_pointer. This
makes debugging the profiler a *lot* easier since the Visual Studio debugger
automatically symbolizes these when explicitly represented as pointer values.

I think this should be possible by creating an IPC::ParamTraits template
specialization for Module and Frame.

[erikchen, 2015/08/31 21:15:17]

The type of this member should not be dependent on Visual Studio capabilities.

Back when this class was only ever instantiated/used in a single process, const
void* was the correct type.

Since this class is going to be passed across Chrome IPC, const void* is no
longer the correct type (and is even dangerous, since one might be tempted to
dereference the pointer in the receiving process).

[Mike Wittman, 2015/08/31 21:42:00]

Debuggability of the code is always a relevant concern, and that code runs only
on Windows.
However, I agree that representing as pointers in a different process is
misleading at best.

Unfortunately I don't see a good way to preserve the pointer representation in
the browser process while supporting uintptr_t for IPC, so uintptr_t it is. I'll
provide additional comments accordingly.

[sydli, 2015/09/01 00:29:59]

Okay; I split this into a separate CL:
https://codereview.chromium.org/1319973011/
in case any functionality of the profiler breaks with the uintptr_t change
(Don't think it should, but just in case). I'll run the related tests on a
Windows machine.

[Mike Wittman, 2015/09/01 00:41:57]

The const void* => uintptr_t change should be pretty low risk. If it compiles
and there's no obvious issues in the code review, it should be fine.

Since we don't want pointers in the IPC representation, I think we should keep
that change in this CL.

 
     // An opaque binary string that uniquely identifies a particular program
     // version with high probability. This is parsed from headers of the loaded
     // module.
     // For binaries generated by GNU tools:
     //   Contents of the .note.gnu.build-id field.
     // On Windows:
     //   GUID + AGE in the debug image headers of a module.
     std::string id;
 
     // The filename of the module.
     FilePath filename;
   };
 
   // Frame represents an individual sampled stack frame with module information.
   struct BASE_EXPORT Frame {
     // Identifies an unknown module.
     static const size_t kUnknownModuleIndex = static_cast<size_t>(-1);
 
+    Frame();
     Frame(const void* instruction_pointer, size_t module_index);

[Mike Wittman, 2015/08/31 21:42:00]

Similarly, the |instruction_pointer| parameter should use the same type as is
stored in the struct.

[sydli, 2015/09/01 00:29:59]

Done.

     ~Frame();
 
     // The sampled instruction pointer within the function.
-    const void* instruction_pointer;
+    uintptr_t instruction_pointer;
 
     // Index of the module in CallStackProfile::modules. We don't represent
     // module state directly here to save space.
     size_t module_index;
   };
 
   // Sample represents a set of stack frames.
   using Sample = std::vector<Frame>;
 
   // CallStackProfile represents a set of samples.
@@ skipping 126 matching lines @@
 // The metrics provider code wants to put Samples in a map and compare them,
 // which requires us to define a few operators.
 BASE_EXPORT bool operator==(const StackSamplingProfiler::Frame& a,
                             const StackSamplingProfiler::Frame& b);
 BASE_EXPORT bool operator<(const StackSamplingProfiler::Frame& a,
                            const StackSamplingProfiler::Frame& b);
 
 }  // namespace base
 
 #endif  // BASE_PROFILER_STACK_SAMPLING_PROFILER_H_