Skip to content

Commit

Permalink
All instances of addresses as void* are changed to uintptr_t in
Browse files Browse the repository at this point in the history 
StackSamplingProfiler. These addresses should not be dereferenced, so
uintptr_t is a better type for recorded instruction pointers. Additionally,
this change will allow IPC macros to use these structures in the future.

BUG=517958

Review URL: https://codereview.chromium.org/1325653003

Cr-Commit-Position: refs/heads/master@{#349254}
  • Loading branch information
sydli authored and Commit bot committed Sep 16, 2015
1 parent 8eed5f5 commit f192d05
Show file tree
Hide file tree
Showing 6 changed files with 64 additions and 58 deletions.
4 changes: 2 additions & 2 deletions base/profiler/native_stack_sampler_win.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -266,7 +266,7 @@ bool NativeStackSamplerWin::GetModuleForHandle(

module->filename = base::FilePath(module_name);

module->base_address = reinterpret_cast<const void*>(module_handle);
module->base_address = reinterpret_cast<uintptr_t>(module_handle);

module->id = GetBuildIDForModule(module_handle);
if (module->id.empty())
Expand Down Expand Up @@ -305,7 +305,7 @@ void NativeStackSamplerWin::CopyToSample(

for (int i = 0; i < stack_depth; ++i) {
sample->push_back(StackSamplingProfiler::Frame(
instruction_pointers[i],
reinterpret_cast<uintptr_t>(instruction_pointers[i]),
GetModuleIndex(module_handles[i], module)));
}
}
Expand Down
11 changes: 6 additions & 5 deletions base/profiler/stack_sampling_profiler.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -83,8 +83,8 @@ void AsyncRunner::RunCallbackAndDeleteInstance(

// StackSamplingProfiler::Module ----------------------------------------------

StackSamplingProfiler::Module::Module() : base_address(nullptr) {}
StackSamplingProfiler::Module::Module(const void* base_address,
StackSamplingProfiler::Module::Module() : base_address(0u) {}
StackSamplingProfiler::Module::Module(uintptr_t base_address,
const std::string& id,
const FilePath& filename)
: base_address(base_address), id(id), filename(filename) {}
Expand All @@ -93,13 +93,14 @@ StackSamplingProfiler::Module::~Module() {}

// StackSamplingProfiler::Frame -----------------------------------------------

StackSamplingProfiler::Frame::Frame(const void* instruction_pointer,
StackSamplingProfiler::Frame::Frame(uintptr_t instruction_pointer,
size_t module_index)
: instruction_pointer(instruction_pointer),
module_index(module_index) {}
: instruction_pointer(instruction_pointer), module_index(module_index) {}

StackSamplingProfiler::Frame::~Frame() {}

StackSamplingProfiler::Frame::Frame() {}

// StackSamplingProfiler::CallStackProfile ------------------------------------

StackSamplingProfiler::CallStackProfile::CallStackProfile() {}
Expand Down
12 changes: 8 additions & 4 deletions base/profiler/stack_sampling_profiler.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -63,12 +63,13 @@ class BASE_EXPORT StackSamplingProfiler {
// 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,
Module(uintptr_t base_address,
const std::string& id,
const FilePath& filename);
~Module();

// Points to the base address of the module.
const void* base_address;
uintptr_t base_address;

// An opaque binary string that uniquely identifies a particular program
// version with high probability. This is parsed from headers of the loaded
Expand All @@ -88,11 +89,14 @@ class BASE_EXPORT StackSamplingProfiler {
// Identifies an unknown module.
static const size_t kUnknownModuleIndex = static_cast<size_t>(-1);

Frame(const void* instruction_pointer, size_t module_index);
Frame(uintptr_t instruction_pointer, size_t module_index);
~Frame();

// Default constructor to satisfy IPC macros. Do not use explicitly.
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.
Expand Down
7 changes: 4 additions & 3 deletions base/profiler/stack_sampling_profiler_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -183,8 +183,9 @@ Sample::const_iterator FindFirstFrameWithinFunction(
int function_size) {
function_address = MaybeFixupFunctionAddressForILT(function_address);
for (auto it = sample.begin(); it != sample.end(); ++it) {
if ((it->instruction_pointer >= function_address) &&
(it->instruction_pointer <
if ((reinterpret_cast<const void*>(it->instruction_pointer) >=
function_address) &&
(reinterpret_cast<const void*>(it->instruction_pointer) <
(static_cast<const unsigned char*>(function_address) + function_size)))
return it;
}
Expand All @@ -198,7 +199,7 @@ std::string FormatSampleForDiagnosticOutput(
std::string output;
for (const Frame& frame: sample) {
output += StringPrintf(
"0x%p %s\n", frame.instruction_pointer,
"0x%p %s\n", reinterpret_cast<const void*>(frame.instruction_pointer),
modules[frame.module_index].filename.AsUTF8Unsafe().c_str());
}
return output;
Expand Down
4 changes: 3 additions & 1 deletion components/metrics/call_stack_profile_metrics_provider.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,13 @@ class ChromeUserMetricsExtension;
class CallStackProfileMetricsProvider : public MetricsProvider {
public:
// The event that triggered the profile collection.
// This enum should be kept in sync with content/common/profiled_stack_state.h
enum Trigger {
UNKNOWN,
PROCESS_STARTUP,
JANKY_TASK,
THREAD_HUNG
THREAD_HUNG,
TRIGGER_LAST = THREAD_HUNG
};

// Parameters to pass back to the metrics provider.
Expand Down
84 changes: 41 additions & 43 deletions components/metrics/call_stack_profile_metrics_provider_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ TEST_F(CallStackProfileMetricsProviderTest, MultipleProfiles) {
const Module profile_modules[][2] = {
{
Module(
reinterpret_cast<const void*>(module1_base_address),
module1_base_address,
"ABCD",
#if defined(OS_WIN)
base::FilePath(L"c:\\some\\path\\to\\chrome.exe")
Expand All @@ -74,7 +74,7 @@ TEST_F(CallStackProfileMetricsProviderTest, MultipleProfiles) {
#endif
),
Module(
reinterpret_cast<const void*>(module2_base_address),
module2_base_address,
"EFGH",
#if defined(OS_WIN)
base::FilePath(L"c:\\some\\path\\to\\third_party.dll")
Expand All @@ -85,7 +85,7 @@ TEST_F(CallStackProfileMetricsProviderTest, MultipleProfiles) {
},
{
Module(
reinterpret_cast<const void*>(module3_base_address),
module3_base_address,
"MNOP",
#if defined(OS_WIN)
base::FilePath(L"c:\\some\\path\\to\\third_party2.dll")
Expand All @@ -94,7 +94,7 @@ TEST_F(CallStackProfileMetricsProviderTest, MultipleProfiles) {
#endif
),
Module( // Repeated from the first profile.
reinterpret_cast<const void*>(module1_base_address),
module1_base_address,
"ABCD",
#if defined(OS_WIN)
base::FilePath(L"c:\\some\\path\\to\\chrome.exe")
Expand Down Expand Up @@ -147,26 +147,26 @@ TEST_F(CallStackProfileMetricsProviderTest, MultipleProfiles) {
const Frame profile_sample_frames[][2][3] = {
{
{
Frame(reinterpret_cast<const void*>(module1_base_address + 0x10), 0),
Frame(reinterpret_cast<const void*>(module2_base_address + 0x20), 1),
Frame(reinterpret_cast<const void*>(module1_base_address + 0x30), 0)
Frame(module1_base_address + 0x10, 0),
Frame(module2_base_address + 0x20, 1),
Frame(module1_base_address + 0x30, 0)
},
{
Frame(reinterpret_cast<const void*>(module2_base_address + 0x10), 1),
Frame(reinterpret_cast<const void*>(module1_base_address + 0x20), 0),
Frame(reinterpret_cast<const void*>(module2_base_address + 0x30), 1)
Frame(module2_base_address + 0x10, 1),
Frame(module1_base_address + 0x20, 0),
Frame(module2_base_address + 0x30, 1)
}
},
{
{
Frame(reinterpret_cast<const void*>(module3_base_address + 0x10), 0),
Frame(reinterpret_cast<const void*>(module1_base_address + 0x20), 1),
Frame(reinterpret_cast<const void*>(module3_base_address + 0x30), 0)
Frame(module3_base_address + 0x10, 0),
Frame(module1_base_address + 0x20, 1),
Frame(module3_base_address + 0x30, 0)
},
{
Frame(reinterpret_cast<const void*>(module1_base_address + 0x10), 1),
Frame(reinterpret_cast<const void*>(module3_base_address + 0x20), 0),
Frame(reinterpret_cast<const void*>(module1_base_address + 0x30), 1)
Frame(module1_base_address + 0x10, 1),
Frame(module3_base_address + 0x20, 0),
Frame(module1_base_address + 0x30, 1)
}
}
};
Expand Down Expand Up @@ -277,7 +277,7 @@ TEST_F(CallStackProfileMetricsProviderTest, RepeatedStacksUnordered) {

const Module modules[] = {
Module(
reinterpret_cast<const void*>(module_base_address),
module_base_address,
"ABCD",
#if defined(OS_WIN)
base::FilePath(L"c:\\some\\path\\to\\chrome.exe")
Expand All @@ -289,10 +289,10 @@ TEST_F(CallStackProfileMetricsProviderTest, RepeatedStacksUnordered) {

// Duplicate samples in slots 0, 2, and 3.
const Frame sample_frames[][1] = {
{ Frame(reinterpret_cast<const void*>(module_base_address + 0x10), 0), },
{ Frame(reinterpret_cast<const void*>(module_base_address + 0x20), 0), },
{ Frame(reinterpret_cast<const void*>(module_base_address + 0x10), 0), },
{ Frame(reinterpret_cast<const void*>(module_base_address + 0x10), 0) }
{ Frame(module_base_address + 0x10, 0), },
{ Frame(module_base_address + 0x20, 0), },
{ Frame(module_base_address + 0x10, 0), },
{ Frame(module_base_address + 0x10, 0) }
};

Profile profile;
Expand Down Expand Up @@ -356,7 +356,7 @@ TEST_F(CallStackProfileMetricsProviderTest, RepeatedStacksOrdered) {

const Module modules[] = {
Module(
reinterpret_cast<const void*>(module_base_address),
module_base_address,
"ABCD",
#if defined(OS_WIN)
base::FilePath(L"c:\\some\\path\\to\\chrome.exe")
Expand All @@ -368,10 +368,10 @@ TEST_F(CallStackProfileMetricsProviderTest, RepeatedStacksOrdered) {

// Duplicate samples in slots 0, 2, and 3.
const Frame sample_frames[][1] = {
{ Frame(reinterpret_cast<const void*>(module_base_address + 0x10), 0), },
{ Frame(reinterpret_cast<const void*>(module_base_address + 0x20), 0), },
{ Frame(reinterpret_cast<const void*>(module_base_address + 0x10), 0), },
{ Frame(reinterpret_cast<const void*>(module_base_address + 0x10), 0) }
{ Frame(module_base_address + 0x10, 0), },
{ Frame(module_base_address + 0x20, 0), },
{ Frame(module_base_address + 0x10, 0), },
{ Frame(module_base_address + 0x10, 0) }
};

Profile profile;
Expand All @@ -390,9 +390,8 @@ TEST_F(CallStackProfileMetricsProviderTest, RepeatedStacksOrdered) {

CallStackProfileMetricsProvider provider;
provider.OnRecordingEnabled();
AppendProfiles(
Params(CallStackProfileMetricsProvider::PROCESS_STARTUP, true),
std::vector<Profile>(1, profile));
AppendProfiles(Params(CallStackProfileMetricsProvider::PROCESS_STARTUP, true),
std::vector<Profile>(1, profile));
ChromeUserMetricsExtension uma_proto;
provider.ProvideGeneralMetrics(&uma_proto);

Expand Down Expand Up @@ -430,8 +429,7 @@ TEST_F(CallStackProfileMetricsProviderTest, RepeatedStacksOrdered) {

// Checks that unknown modules produce an empty Entry.
TEST_F(CallStackProfileMetricsProviderTest, UnknownModule) {
const Frame frame(reinterpret_cast<const void*>(0x1000),
Frame::kUnknownModuleIndex);
const Frame frame(0x1000, Frame::kUnknownModuleIndex);

Profile profile;

Expand Down Expand Up @@ -471,8 +469,8 @@ TEST_F(CallStackProfileMetricsProviderTest, ProfilesProvidedOnlyOnce) {
CallStackProfileMetricsProvider provider;
for (int i = 0; i < 2; ++i) {
Profile profile;
profile.samples.push_back(Sample(1, Frame(
reinterpret_cast<const void*>(0x1000), Frame::kUnknownModuleIndex)));
profile.samples.push_back(
Sample(1, Frame(0x1000, Frame::kUnknownModuleIndex)));

profile.profile_duration = base::TimeDelta::FromMilliseconds(100);
// Use the sampling period to distinguish the two profiles.
Expand Down Expand Up @@ -500,8 +498,8 @@ TEST_F(CallStackProfileMetricsProviderTest, ProfilesProvidedOnlyOnce) {
TEST_F(CallStackProfileMetricsProviderTest,
ProfilesProvidedWhenCollectedBeforeInstantiation) {
Profile profile;
profile.samples.push_back(Sample(1, Frame(
reinterpret_cast<const void*>(0x1000), Frame::kUnknownModuleIndex)));
profile.samples.push_back(
Sample(1, Frame(0x1000, Frame::kUnknownModuleIndex)));

profile.profile_duration = base::TimeDelta::FromMilliseconds(100);
profile.sampling_period = base::TimeDelta::FromMilliseconds(10);
Expand All @@ -522,8 +520,8 @@ TEST_F(CallStackProfileMetricsProviderTest,
// while recording is disabled.
TEST_F(CallStackProfileMetricsProviderTest, ProfilesNotProvidedWhileDisabled) {
Profile profile;
profile.samples.push_back(Sample(1, Frame(
reinterpret_cast<const void*>(0x1000), Frame::kUnknownModuleIndex)));
profile.samples.push_back(
Sample(1, Frame(0x1000, Frame::kUnknownModuleIndex)));

profile.profile_duration = base::TimeDelta::FromMilliseconds(100);
profile.sampling_period = base::TimeDelta::FromMilliseconds(10);
Expand All @@ -544,8 +542,8 @@ TEST_F(CallStackProfileMetricsProviderTest, ProfilesNotProvidedWhileDisabled) {
TEST_F(CallStackProfileMetricsProviderTest,
ProfilesNotProvidedAfterChangeToDisabled) {
Profile profile;
profile.samples.push_back(Sample(1, Frame(
reinterpret_cast<const void*>(0x1000), Frame::kUnknownModuleIndex)));
profile.samples.push_back(
Sample(1, Frame(0x1000, Frame::kUnknownModuleIndex)));

profile.profile_duration = base::TimeDelta::FromMilliseconds(100);
profile.sampling_period = base::TimeDelta::FromMilliseconds(10);
Expand All @@ -569,8 +567,8 @@ TEST_F(CallStackProfileMetricsProviderTest,
TEST_F(CallStackProfileMetricsProviderTest,
ProfilesNotProvidedAfterChangeToDisabledThenEnabled) {
Profile profile;
profile.samples.push_back(Sample(1, Frame(
reinterpret_cast<const void*>(0x1000), Frame::kUnknownModuleIndex)));
profile.samples.push_back(
Sample(1, Frame(0x1000, Frame::kUnknownModuleIndex)));

profile.profile_duration = base::TimeDelta::FromMilliseconds(100);
profile.sampling_period = base::TimeDelta::FromMilliseconds(10);
Expand All @@ -595,8 +593,8 @@ TEST_F(CallStackProfileMetricsProviderTest,
TEST_F(CallStackProfileMetricsProviderTest,
ProfilesNotProvidedAfterChangeFromDisabled) {
Profile profile;
profile.samples.push_back(Sample(1, Frame(
reinterpret_cast<const void*>(0x1000), Frame::kUnknownModuleIndex)));
profile.samples.push_back(
Sample(1, Frame(0x1000, Frame::kUnknownModuleIndex)));

profile.profile_duration = base::TimeDelta::FromMilliseconds(100);
profile.sampling_period = base::TimeDelta::FromMilliseconds(10);
Expand Down

0 comments on commit f192d05

Please sign in to comment.