1. Enable large object pointer offset check in release build.

third_party/tcmalloc/chromium/src/tcmalloc.cc

Index: third_party/tcmalloc/chromium/src/tcmalloc.cc
===================================================================
--- third_party/tcmalloc/chromium/src/tcmalloc.cc	(revision 137587)
+++ third_party/tcmalloc/chromium/src/tcmalloc.cc	(working copy)
@@ -178,13 +178,13 @@
 using tcmalloc::Static;
 using tcmalloc::ThreadCache;
 
-// ---- Double free debug declarations
+// ---- Functions doing validation with an extra mark.
 static size_t ExcludeSpaceForMark(size_t size);
 static void AddRoomForMark(size_t* size);
 static void ExcludeMarkFromSize(size_t* new_size);
 static void MarkAllocatedRegion(void* ptr);
 static void ValidateAllocatedRegion(void* ptr, size_t cl);
-// ---- End Double free debug declarations
+// ---- End validation functions.
 
 DECLARE_int64(tcmalloc_sample_parameter);
 DECLARE_double(tcmalloc_release_rate);
@@ -1155,8 +1155,17 @@
     }
     cl = span->sizeclass;
     Static::pageheap()->CacheSizeClass(p, cl);
+
+    // The span is not in use! A double free?

[gpike, 2012/05/21 19:04:58]

At this point cl could be anything. Don't these checks need to be in the block
of code that is executed when you are sure you are about to call
Static::pageheap()->Delete(span)?

[kaiwang, 2012/05/21 22:21:42]

oops, sorry I accidentally removed a {} pair.

These checks should be before ValidateAllocatedRegion, so error message will be
the same in debug and release builds (while ValidateAllocatedRegion does some
extra check not available in release build).
Also moving chromium modified code together is good for later merging

+    CHECK_CONDITION_PRINT(span->location == Span::IN_USE,
+                          "Freeing a span not in use");
+
+    // Mimic debug assertion below to validate pointer of large objects.
+    CHECK_CONDITION_PRINT(span->start == p,

[gpike, 2012/05/21 19:04:58]

It seems safe to replace the last two checks with a check for span->start <<
kPageShift == ptr.

[kaiwang, 2012/05/21 22:21:42]

Done. Good idea

+                          "Pointer is not inside the first page of a span");
+    CHECK_CONDITION_PRINT(reinterpret_cast<uintptr_t>(ptr) % kPageSize == 0,
+                          "Pointer is not pointing to the start of a span");

[jar (doing other things), 2012/05/21 18:44:10]

I appreciate that the text of these messages is only here to assure that we have
a death-test with a matching log.... but I'm sad that "test code" causes bloat
in the release version (re: strings), beyond the if/else execution cost of the
CHECK.

Is there any way you can propose to not have the text appear in release
versions?  Maybe there should be yet another flavor of macro based on
OFFICIAL_BUILD (or such) that discards the text, and leaves it to us to check
stacks to determine the cause of a CHECK.  

Suggestions?

[kaiwang, 2012/05/21 22:21:42]

Actually for this specific case, the size will not increase, because
CHECK_CONDITION still have a string which is set to the condition parameter.
(You can see from above that the length is about the same..

If size of tcmalloc library (release version) is actually an issue, I think we
can make some code change to internal_logging.h and logging.h. But might be in
another CL :)

   }
-
   ValidateAllocatedRegion(ptr, cl);
 
   if (cl != 0) {
@@ -1276,7 +1285,7 @@
 void* do_memalign(size_t align, size_t size) {
   ASSERT((align & (align - 1)) == 0);
   ASSERT(align > 0);
-  // Marked in CheckMallocResult(), which is also inside SpanToMallocResult(). 
+  // Marked in CheckedMallocResult(), which is also inside SpanToMallocResult().
   AddRoomForMark(&size);
   if (size + align < size) return NULL;         // Overflow
 
@@ -1698,7 +1707,7 @@
 
 #endif  // TCMALLOC_USING_DEBUGALLOCATION
 
-// ---Double free() debugging implementation -----------------------------------
+// --- Validation implementation with an extra mark ----------------------------
 // We will put a mark at the extreme end of each allocation block.  We make
 // sure that we always allocate enough "extra memory" that we can fit in the
 // mark, and still provide the requested usable region.  If ever that mark is
@@ -1741,22 +1750,11 @@
 #else  // TCMALLOC_VALIDATION
 
 static void DieFromDoubleFree() {
-  char* p = NULL;
-  p++;
-  *p += 1;  // Segv.
+  Log(kCrash, __FILE__, __LINE__, "Attempt to double free");
 }
 
-static size_t DieFromBadFreePointer(const void* unused) {
-  char* p = NULL;
-  p += 2;
-  *p += 2;  // Segv.
-  return 0;
-}
-
 static void DieFromMemoryCorruption() {
-  char* p = NULL;
-  p += 3;
-  *p += 3;  // Segv.
+  Log(kCrash, __FILE__, __LINE__, "Memory corrupted");
 }
 
 // We can either do byte marking, or whole word marking based on the following
@@ -1770,7 +1768,7 @@
 typedef char MarkType;  // char saves memory... int is more complete.
 static const MarkType kAllocationMarkMask = static_cast<MarkType>(0x36);
 
-#else 
+#else
 
 typedef int MarkType;  // char saves memory... int is more complete.
 static const MarkType kAllocationMarkMask = static_cast<MarkType>(0xE1AB9536);
@@ -1793,9 +1791,9 @@
 }
 
 inline static MarkType* GetMarkLocation(void* ptr) {
-  size_t class_size = GetSizeWithCallback(ptr, DieFromBadFreePointer);
-  ASSERT(class_size % sizeof(kAllocationMarkMask) == 0);
-  size_t last_index = (class_size / sizeof(kAllocationMarkMask)) - 1;
+  size_t size = GetSizeWithCallback(ptr, &InvalidGetAllocatedSize);
+  ASSERT(size % sizeof(kAllocationMarkMask) == 0);
+  size_t last_index = (size / sizeof(kAllocationMarkMask)) - 1;
   return static_cast<MarkType*>(ptr) + last_index;
 }