Skip to content

Commit

Permalink
[Tracing] Make AllocationContext constructor private
Browse files Browse the repository at this point in the history
This is to prevent unintentionally constructing an uninitialized
instance. It is still possible to construct an |AllocationContext|
(apart from copy constructing) by using an initializer list. This is
fine, because the instance will not be filled with garbage; it will
be zeroed out. By design, a zeroed instance represents a context with
unknown type ID and empty backtrace.

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

Cr-Commit-Position: refs/heads/master@{#359817}
  • Loading branch information
ruuda authored and Commit bot committed Nov 16, 2015
1 parent c958aa7 commit 02348bc
Show file tree
Hide file tree
Showing 3 changed files with 35 additions and 9 deletions.
16 changes: 16 additions & 0 deletions base/trace_event/heap_profiler_allocation_context.cc
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,26 @@
#include <cstring>

#include "base/hash.h"
#include "base/macros.h"

namespace base {
namespace trace_event {

// Constructor that does not initialize members.
AllocationContext::AllocationContext() {}

// static
AllocationContext AllocationContext::Empty() {
AllocationContext ctx;

for (size_t i = 0; i < arraysize(ctx.backtrace.frames); i++)
ctx.backtrace.frames[i] = nullptr;

ctx.type_id = 0;

return ctx;
}

bool operator==(const Backtrace& lhs, const Backtrace& rhs) {
// Pointer equality of the stack frames is assumed, so instead of doing a deep
// string comparison on all of the frames, a |memcmp| suffices.
Expand Down
16 changes: 13 additions & 3 deletions base/trace_event/heap_profiler_allocation_context.h
Original file line number Diff line number Diff line change
Expand Up @@ -50,18 +50,28 @@ bool BASE_EXPORT operator==(const Backtrace& lhs, const Backtrace& rhs);
// when heap profiling is enabled. To simplify memory management for book-
// keeping, this struct has a fixed size. All |const char*|s here must have
// static lifetime.
// TODO(ruuda): Make the default constructor private to avoid accidentally
// constructing an instance and forgetting to initialize it. Only
// |AllocationContextTracker| should be able to construct. (And tests.)
struct BASE_EXPORT AllocationContext {
public:
// A type ID is a number that is unique for every C++ type. A type ID is
// stored instead of the type name to avoid inflating the binary with type
// name strings. There is an out of band lookup table mapping IDs to the type
// names. A value of 0 means that the type is not known.
using TypeId = uint16_t;

// An allocation context with empty backtrace and type ID 0 (unknown type).
static AllocationContext Empty();

Backtrace backtrace;
TypeId type_id;

private:
friend class AllocationContextTracker;

// Don't allow uninitialized instances except inside the allocation context
// tracker. Except in tests, an |AllocationContext| should only be obtained
// from the tracker. In tests, paying the overhead of initializing the struct
// to |Empty| and then overwriting the members is not such a big deal.
AllocationContext();
};

bool BASE_EXPORT operator==(const AllocationContext& lhs,
Expand Down
12 changes: 6 additions & 6 deletions base/trace_event/heap_profiler_allocation_register_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ size_t SumAllSizes(const AllocationRegister& reg) {

TEST_F(AllocationRegisterTest, InsertRemove) {
AllocationRegister reg;
AllocationContext ctx = {};
AllocationContext ctx = AllocationContext::Empty();

EXPECT_EQ(0u, OrAllAddresses(reg));

Expand Down Expand Up @@ -82,7 +82,7 @@ TEST_F(AllocationRegisterTest, InsertRemove) {

TEST_F(AllocationRegisterTest, DoubleFreeIsAllowed) {
AllocationRegister reg;
AllocationContext ctx = {};
AllocationContext ctx = AllocationContext::Empty();

reg.Insert(reinterpret_cast<void*>(1), 0, ctx);
reg.Insert(reinterpret_cast<void*>(2), 0, ctx);
Expand All @@ -97,7 +97,7 @@ TEST_F(AllocationRegisterTest, DoubleInsertOverwrites) {
// TODO(ruuda): Although double insert happens in practice, it should not.
// Find out the cause and ban double insert if possible.
AllocationRegister reg;
AllocationContext ctx = {};
AllocationContext ctx = AllocationContext::Empty();
StackFrame frame1 = "Foo";
StackFrame frame2 = "Bar";

Expand Down Expand Up @@ -125,7 +125,7 @@ TEST_F(AllocationRegisterTest, DoubleInsertOverwrites) {
TEST_F(AllocationRegisterTest, InsertRemoveCollisions) {
size_t expected_sum = 0;
AllocationRegister reg;
AllocationContext ctx = {};
AllocationContext ctx = AllocationContext::Empty();

// By inserting 100 more entries than the number of buckets, there will be at
// least 100 collisions.
Expand Down Expand Up @@ -162,7 +162,7 @@ TEST_F(AllocationRegisterTest, InsertRemoveCollisions) {
TEST_F(AllocationRegisterTest, InsertRemoveRandomOrder) {
size_t expected_sum = 0;
AllocationRegister reg;
AllocationContext ctx = {};
AllocationContext ctx = AllocationContext::Empty();

uintptr_t generator = 3;
uintptr_t prime = 1013;
Expand Down Expand Up @@ -204,7 +204,7 @@ TEST_F(AllocationRegisterTest, InsertRemoveRandomOrder) {
#if GTEST_HAS_DEATH_TEST
TEST_F(AllocationRegisterTest, OverflowDeathTest) {
AllocationRegister reg;
AllocationContext ctx = {};
AllocationContext ctx = AllocationContext::Empty();
uintptr_t i;

// Fill up all of the memory allocated for the register. |kNumCells| minus 1
Expand Down

0 comments on commit 02348bc

Please sign in to comment.