Update the tcmalloc chromium branch to r144 (gperftools 2.0), and merge chromium-specific changes.

Update the tcmalloc chromium branch to r144 (gperftools 2.0), and merge chromium-specific changes.

The major reason for us is to enable a fix for HEAP_PROFILE_MMAP.

This change merges tcmalloc r144 (gperftools 2.0 f.k.a. google-perftools) with the tcmalloc/chromium branch, which is the forked Chromium version of tcmalloc.

The change is created by 3-way merge from
1) the original google-perftools r109 ( = vendor base),
2) the original gperftools 2.0  r144 ( = branch), and
3) the chromium branch ( = another branch)

with the following additional changes :
* base/allocator/allocator.gyp is modified.
* Many [#include "third_party/tcmalloc/chromium/src/google/..."] are replaced with "gperftools/".  (Many files in Chromium)
* gperftools/tcmalloc.h (formerly google/tcmalloc.h) is replaced with the original (generated) one.
* windows/gperftools/tcmalloc.h (formerly windows/google/tcmalloc.h) is replaced with the original (generated) one.
* malloc_hook-like functions are moved to libc_override*.h in gperftools 2.0.  Some changes due to it.
* MALLOC_HOOK_MAYBE_VOLATILE is redefined using __MALLOC_HOOK_VOLATILE.  (config.h, tcmalloc.cc and libc_override_glibc.h)
* The macro "CRASH(...)" is replaced with "Log(kCrash, __FILE__, __LINE__, ...)".  (Many files)
* LARGE_PAGE-related parameters (which may affect performance?) are merged.  (common.h)
* RAW_VLOG() calls are removed.  (base/googleinit.h)
* sys_{mmap|munmap|mremap}(...) calls are tentatively replaced with syscall(SYS_{mmap|munmap|mremap}, ...).  (malloc_hook_mmap_linux.h)
* tc_mallinfo is declared only when HAVE_STRUCT_MALLINFO is defined.  (gperftools/tcmalloc.h)
* "libc_override_redefine.h" is not included in Windows.  (libc_override.h)
* Chromium-original "sys_alloc" is not declared.  (windows/port.cc)
* base/spinlock_win32-inl.h is reverted from r144 because 64-bit atomicops are not implemented on Windows.  (base/atomicops-internals-windows.h)

The vendor branch is updated in another change.

BUG=114302
TEST=run all existing tests.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=124832

[Dai Mikurube (NOT FULLTIME), 2012-02-06 06:42:58]

The change is generated by 1) taking a diff between the original r109
(checked-out separately) and the current tcmalloc/chromium branch, and 2)
applying the diff to a clean r136 tree.

The tcmalloc/chromium and tcmalloc/vendor branches are now based on different
versions :
* /chromium: r111 @ http://crrev.com/93000
* /vendor: r109 @ http://crrev.com/87286 > http://crrev.com/89452


There is an unapplied diff in tcmalloc.cc.  A similar part is now in
libc_override*.h, but they don't work since they call tc_malloc and other tc_*
functions.

Calling tc_* functions from libc_override*.h is available in the original
g(oogle-)perftools since it is including google/tcmalloc.h which is generated
from google/tcmalloc.h.in.

But, in the tcmalloc/chromium branch, src/tcmalloc.h is moved into
src/google/tcmalloc.h.  No tcmalloc.h is generated from tcmalloc.h.in.  It's the
reason why the Chromium version cannot call tc_* from libc_override*.h.

src/tcmalloc.h is moved into src/google/ in a very old change
http://crrev.com/33078.

Does someone know why src/tcmalloc.h is moved into src/google?  I looked at
http://crbug.com/27911, but it doesn't describe a good reason for this moving...

[Dai Mikurube (NOT FULLTIME), 2012-02-07 10:47:03]

Updated to use the latest gperftools 2.0 (r144).

Please read the comment #1 above.  This change doesn't include tcmalloc.cc.diff,
but it still have a problem about tc_* functions called from libc_override*.h.

Note that the directory for header files is changed from google/ into
gperftools/.  It's a change from gperftools 2.0.

[Dai Mikurube (NOT FULLTIME), 2012-02-07 12:27:08]

Please read the comments #1 and #2 above.

Successfully built by
* replacing gperftools/tcmalloc.h with the original (generated) one, and
* redefined MALLOC_HOOK_MAYBE_VOLATILE with using __MALLOC_HOOK_VOLATILE, and
* fixed all #include "third_party/tcmalloc/chromium/src/google/..." to use
gperftools/.

It works for test_shell_tests, but the Chrome binary doesn't work.  The Debug
build successfully starts with a green window, but no progress from the green
window.

[Dai Mikurube (NOT FULLTIME), 2012-02-08 07:06:11]

An interim update: the Chrome binary with tcmalloc-r144 worked with
--no-sandbox.  It looks like a sandbox-related issue.

[Dai Mikurube (NOT FULLTIME), 2012-02-08 11:21:08]

On 2012/02/08 07:06:11, Dai Mikurube wrote:
> An interim update: the Chrome binary with tcmalloc-r144 worked with
> --no-sandbox.  It looks like a sandbox-related issue.

Another interim report:  it works with sandbox in Release builds... I'm
narrowing down the cause.

[Dai Mikurube (NOT FULLTIME), 2012-02-09 10:57:04]

The linux_rel bot got happy.

Could you take a *brief* look, Jim and Geoff? though it still stops in Debug
builds with sandbox...

The change is created by 3-way merging from the original r109 ( = base), the
original r144 ( = branch-1) and the chromium branch ( = branch-2).

Changes to be noticed are:
* google/ is renamed into gperftools/ in gperftools 2.0.
* malloc_hooks are moved to libc_override*.h.  Volatile malloc_hooks are handled
in libc_override*.h.
* replaced gperftools/tcmalloc.h with the original generated tcmalloc.h.
* LARGE_PAGE-related parameters (maybe performance issue??) are merged.  Look at
common.h.
* RAW_VLOGs are removed in base/googleinit.h since
net/tools/flip_server/balsa_headers.cc includes
third_party/tcmalloc/chromium/src/base/googleinit.h.  googleinit.h includes
"base/logging.h".  TCMalloc thinks it's "base/logging.h" in gperftools, but
actually it is recognized as base/logging.h in Chromium.

Other parts are naively merged.

[Dai Mikurube (NOT FULLTIME), 2012-02-10 07:33:07]

It looks like working by replacing sys_{mmap|munmap|mremap}(...) calls with
syscall(SYS_{mmap|munmap|mremap}, ...) calls.  This change is tentative.

http://codereview.chromium.org/9311003/diff/25001/third_party/tcmalloc/chromi...
File third_party/tcmalloc/chromium/src/base/basictypes.h (right):

http://codereview.chromium.org/9311003/diff/25001/third_party/tcmalloc/chromi...
third_party/tcmalloc/chromium/src/base/basictypes.h:215: memcpy(&dest, &source,
sizeof(dest));
memcpy

http://codereview.chromium.org/9311003/diff/25001/third_party/tcmalloc/chromi...
File third_party/tcmalloc/chromium/src/base/cycleclock.h (right):

http://codereview.chromium.org/9311003/diff/25001/third_party/tcmalloc/chromi...
third_party/tcmalloc/chromium/src/base/cycleclock.h:147: * CyclesPerSecond());
gettimeofday, CyclesPerSecond

http://codereview.chromium.org/9311003/diff/25001/third_party/tcmalloc/chromi...
File third_party/tcmalloc/chromium/src/base/linux_syscall_support.h (right):

http://codereview.chromium.org/9311003/diff/25001/third_party/tcmalloc/chromi...
third_party/tcmalloc/chromium/src/base/linux_syscall_support.h:128: *     
sys_write
syscalls

http://codereview.chromium.org/9311003/diff/25001/third_party/tcmalloc/chromi...
File third_party/tcmalloc/chromium/src/base/sysinfo.cc (right):

http://codereview.chromium.org/9311003/diff/25001/third_party/tcmalloc/chromi...
third_party/tcmalloc/chromium/src/base/sysinfo.cc:100: #ifdef SYS_close
syscall

http://codereview.chromium.org/9311003/diff/25001/third_party/tcmalloc/chromi...
File third_party/tcmalloc/chromium/src/debugallocation.cc (right):

http://codereview.chromium.org/9311003/diff/25001/third_party/tcmalloc/chromi...
third_party/tcmalloc/chromium/src/debugallocation.cc:1: // Copyright (c) 2000,
Google Inc.
debug? -- it's not.  it's not used.

http://codereview.chromium.org/9311003/diff/25001/third_party/tcmalloc/chromi...
File third_party/tcmalloc/chromium/src/malloc_hook_mmap_linux.h (right):

http://codereview.chromium.org/9311003/diff/25001/third_party/tcmalloc/chromi...
third_party/tcmalloc/chromium/src/malloc_hook_mmap_linux.h:59: return
sys_mmap(start, length, prot, flags, fd, offset);
not syscall(SYS_MMAP, ...) but sys_mmap().

[Dai Mikurube (NOT FULLTIME), 2012-02-10 07:34:51]

On 2012/02/10 07:33:07, Dai Mikurube wrote:
> It looks like working by replacing sys_{mmap|munmap|mremap}(...) calls with
> syscall(SYS_{mmap|munmap|mremap}, ...) calls.  This change is tentative.
> 
>
http://codereview.chromium.org/9311003/diff/25001/third_party/tcmalloc/chromi...
> File third_party/tcmalloc/chromium/src/base/basictypes.h (right):
> 
>
http://codereview.chromium.org/9311003/diff/25001/third_party/tcmalloc/chromi...
> third_party/tcmalloc/chromium/src/base/basictypes.h:215: memcpy(&dest,
&source,
> sizeof(dest));
> memcpy
> 
>
http://codereview.chromium.org/9311003/diff/25001/third_party/tcmalloc/chromi...
> File third_party/tcmalloc/chromium/src/base/cycleclock.h (right):
> 
>
http://codereview.chromium.org/9311003/diff/25001/third_party/tcmalloc/chromi...
> third_party/tcmalloc/chromium/src/base/cycleclock.h:147: * CyclesPerSecond());
> gettimeofday, CyclesPerSecond
> 
>
http://codereview.chromium.org/9311003/diff/25001/third_party/tcmalloc/chromi...
> File third_party/tcmalloc/chromium/src/base/linux_syscall_support.h (right):
> 
>
http://codereview.chromium.org/9311003/diff/25001/third_party/tcmalloc/chromi...
> third_party/tcmalloc/chromium/src/base/linux_syscall_support.h:128: *     
> sys_write
> syscalls
> 
>
http://codereview.chromium.org/9311003/diff/25001/third_party/tcmalloc/chromi...
> File third_party/tcmalloc/chromium/src/base/sysinfo.cc (right):
> 
>
http://codereview.chromium.org/9311003/diff/25001/third_party/tcmalloc/chromi...
> third_party/tcmalloc/chromium/src/base/sysinfo.cc:100: #ifdef SYS_close
> syscall
> 
>
http://codereview.chromium.org/9311003/diff/25001/third_party/tcmalloc/chromi...
> File third_party/tcmalloc/chromium/src/debugallocation.cc (right):
> 
>
http://codereview.chromium.org/9311003/diff/25001/third_party/tcmalloc/chromi...
> third_party/tcmalloc/chromium/src/debugallocation.cc:1: // Copyright (c) 2000,
> Google Inc.
> debug? -- it's not.  it's not used.
> 
>
http://codereview.chromium.org/9311003/diff/25001/third_party/tcmalloc/chromi...
> File third_party/tcmalloc/chromium/src/malloc_hook_mmap_linux.h (right):
> 
>
http://codereview.chromium.org/9311003/diff/25001/third_party/tcmalloc/chromi...
> third_party/tcmalloc/chromium/src/malloc_hook_mmap_linux.h:59: return
> sys_mmap(start, length, prot, flags, fd, offset);
> not syscall(SYS_MMAP, ...) but sys_mmap().

Inline comments above are just my personal notes, which are wrongly published. 
Please ignore them.

[Dai Mikurube (NOT FULLTIME), 2012-02-13 08:49:14]

It's ready for review, except for the win_rel bot.  Could you PTAL at it?

I wonder if someone knows a good fix for the issue in the try with win_rel...

[kinuko, 2012-02-13 11:27:33]

(Adding glider@)

[Alexander Potapenko, 2012-02-13 11:52:11]

On 2012/02/13 11:27:33, kinuko wrote:
> (Adding glider@)

Hi Dai,

you may want to take a look http://codereview.chromium.org/7430007/, which was
my previous attempt to update tcmalloc in Chromium.
First, the compilation problems may be fixed by this diff:
http://codereview.chromium.org/7430007/diff2/25007:18014/third_party/tcmalloc...
-- I remember spending some time on this, but the trybot runs are lost, so I
cannot tell for sure.
Second, consider looking at Jim's comments there before he reads your CL and
leaves the same comments here.

[Dai Mikurube (NOT FULLTIME), 2012-02-14 11:32:08]

Thanks for your comments, Alaxander, Kinuko.

I fixed the problems with the win_rel bot by your and jar's help.  win_rel looks
successfully compiling.

I added some comments inline related to http://codereview.chromium.org/7430007/.

http://codereview.chromium.org/9311003/diff/46004/third_party/tcmalloc/chromi...
File third_party/tcmalloc/chromium/src/base/spinlock_internal.cc (right):

http://codereview.chromium.org/9311003/diff/46004/third_party/tcmalloc/chromi...
third_party/tcmalloc/chromium/src/base/spinlock_internal.cc:50: #elif
defined(__linux__)
From
http://codereview.chromium.org/7430007/diff/18014/third_party/tcmalloc/chromi...

defined(__native_client__) is not checked in this change.

http://codereview.chromium.org/9311003/diff/46004/third_party/tcmalloc/chromi...
File third_party/tcmalloc/chromium/src/central_freelist.h (right):

http://codereview.chromium.org/9311003/diff/46004/third_party/tcmalloc/chromi...
third_party/tcmalloc/chromium/src/central_freelist.h:79: // page full of 5-byte
objects would have 2 bytes memory overhead).
From
http://codereview.chromium.org/7430007/diff/18014/third_party/tcmalloc/chromi...

This comment came from the latest gperftools.  Just ported.

http://codereview.chromium.org/9311003/diff/46004/third_party/tcmalloc/chromi...
File third_party/tcmalloc/chromium/src/common.cc (right):

http://codereview.chromium.org/9311003/diff/46004/third_party/tcmalloc/chromi...
third_party/tcmalloc/chromium/src/common.cc:131: // Continue to add pages until
there are at least as many objects in
From
http://codereview.chromium.org/7430007/diff/18014/third_party/tcmalloc/chromi...

Hmm, I know no background of this, too.  It's from the original gperftools.

http://codereview.chromium.org/9311003/diff/46004/third_party/tcmalloc/chromi...
File third_party/tcmalloc/chromium/src/common.h (right):

http://codereview.chromium.org/9311003/diff/46004/third_party/tcmalloc/chromi...
third_party/tcmalloc/chromium/src/common.h:83: #endif
From
http://codereview.chromium.org/7430007/diff/18014/third_party/tcmalloc/chromi...
and
http://codereview.chromium.org/7430007/diff/18014/third_party/tcmalloc/chromi...

These changes are manually merged from both the Chromium-original branch and the
latest original gperftools r144.

http://codereview.chromium.org/9311003/diff/46004/third_party/tcmalloc/chromi...
third_party/tcmalloc/chromium/src/common.h:87: static const size_t kMaxSize    =
256 * 1024;
From
http://codereview.chromium.org/7430007/diff/18014/third_party/tcmalloc/chromi...

This may affect performance in a large way.  We may have to tune it.

http://codereview.chromium.org/9311003/diff/46004/third_party/tcmalloc/chromi...
File third_party/tcmalloc/chromium/src/malloc_hook_mmap_linux.h (right):

http://codereview.chromium.org/9311003/diff/46004/third_party/tcmalloc/chromi...
third_party/tcmalloc/chromium/src/malloc_hook_mmap_linux.h:61: */
I know these comment-outs are redundant.

It may have some problems, so I'm contacting the author.

[Dai Mikurube (NOT FULLTIME), 2012-02-16 08:38:07]

win_rel looks like failing very flakily.  My guess is that these errors
(terminate in an unusual way) are sandbox-related since they don't happen in
unit tests.

Could someone imagine the root cause of it?  I don't have an active Windows
machine.

[Dai Mikurube (NOT FULLTIME), 2012-02-23 08:14:28]

This call looks like the cause of the crash in Windows Release builds.  I'll
contact the original author...

http://codereview.chromium.org/9311003/diff/46004/third_party/tcmalloc/chromi...
File third_party/tcmalloc/chromium/src/base/spinlock_internal.cc (right):

http://codereview.chromium.org/9311003/diff/46004/third_party/tcmalloc/chromi...
third_party/tcmalloc/chromium/src/base/spinlock_internal.cc:84: uint64 r =
base::subtle::NoBarrier_Load(&rand);
It's calling 64-bit NoBarrier_Load().  It's not implemented in Windows!  (See
base/atomicops-internals-windows.h.)

[Dai Mikurube (NOT FULLTIME), 2012-02-24 03:30:54]

Hi,

Could you PTAL at it, or forward it to right persons?

It looks working except for some existing flaky failures.  (I'll submit a try
again.)

[Dai Mikurube (NOT FULLTIME), 2012-02-24 06:13:00]

Added comments on Chromium-original changes to describe clearly.  I'll submit
tries when try bots calm down.

I'm looking forward to be reviewed.

[jar (doing other things), 2012-02-28 22:15:49]

Comments below per our marathon (in person) review.

Thanks for spending the time!

http://codereview.chromium.org/9311003/diff/73001/third_party/tcmalloc/chromi...
File third_party/tcmalloc/chromium/src/base/googleinit.h (right):

http://codereview.chromium.org/9311003/diff/73001/third_party/tcmalloc/chromi...
third_party/tcmalloc/chromium/src/base/googleinit.h:47: // when this
googleinit.h is included from out of third_party/tcmalloc.
Suggest you have comment, and then show exactly the code that you chose to
remove as "commented out code."  In this case, it might be:

// We commented out the following lines, since chromium does // not have the
proper includes to log using these macros.
// RAW_VLOG(......)

http://codereview.chromium.org/9311003/diff/73001/third_party/tcmalloc/chromi...
File third_party/tcmalloc/chromium/src/base/spinlock_win32-inl.h (right):

http://codereview.chromium.org/9311003/diff/73001/third_party/tcmalloc/chromi...
third_party/tcmalloc/chromium/src/base/spinlock_win32-inl.h:47: //
base/atomicops-internals-windows.h doesn't support 64-bit operations.
Please supply commented out version of original code.

http://codereview.chromium.org/9311003/diff/73001/third_party/tcmalloc/chromi...
File third_party/tcmalloc/chromium/src/common.cc (right):

http://codereview.chromium.org/9311003/diff/73001/third_party/tcmalloc/chromi...
third_party/tcmalloc/chromium/src/common.cc:74: alignment = kPageSize;
It is interesting this this "kPageSize" is now a more fake size, as we may use
8K or as large as 32K pages.

http://codereview.chromium.org/9311003/diff/73001/third_party/tcmalloc/chromi...
File third_party/tcmalloc/chromium/src/common.h (right):

http://codereview.chromium.org/9311003/diff/73001/third_party/tcmalloc/chromi...
third_party/tcmalloc/chromium/src/common.h:89: static const size_t kMaxSize    =
256 * 1024;
All these size changes have a potentially large impact on performance.

kMaxSize went from 32K to 256K
PageSize went from 4 k to 8K or 32K

http://codereview.chromium.org/9311003/diff/73001/third_party/tcmalloc/chromi...
File third_party/tcmalloc/chromium/src/tcmalloc.cc (right):

http://codereview.chromium.org/9311003/diff/73001/third_party/tcmalloc/chromi...
third_party/tcmalloc/chromium/src/tcmalloc.cc:150: // Some windows file
somewhere (at least on cygwin) #define's small (!)
Comment that <windows.h> appears to #define small char

http://codereview.chromium.org/9311003/diff/73001/third_party/tcmalloc/chromi...
File third_party/tcmalloc/chromium/src/thread_cache.h (right):

http://codereview.chromium.org/9311003/diff/73001/third_party/tcmalloc/chromi...
third_party/tcmalloc/chromium/src/thread_cache.h:213: return SLL_Next(&list_);
Probably add FL_Next() instead of SLL_next.

[Dai Mikurube (NOT FULLTIME), 2012-02-28 23:05:42]

Hi John and Mike,

I wonder if you could PTAL at changes in content/* and webkit/extensions/v8/* as
you're OWNERs.

It changes only include directories from google/ to gperftools/.  (The latest
tcmalloc library moves its header files into gperftools/.  See
http://code.google.com/p/gperftools/ .)


Jim,

Thank you for spending your time to this tough review.  Updated the patch.

http://codereview.chromium.org/9311003/diff/73001/third_party/tcmalloc/chromi...
File third_party/tcmalloc/chromium/src/base/googleinit.h (right):

http://codereview.chromium.org/9311003/diff/73001/third_party/tcmalloc/chromi...
third_party/tcmalloc/chromium/src/base/googleinit.h:47: // when this
googleinit.h is included from out of third_party/tcmalloc.
On 2012/02/28 22:15:49, jar wrote:
> Suggest you have comment, and then show exactly the code that you chose to
> remove as "commented out code."  In this case, it might be:
> 
> // We commented out the following lines, since chromium does // not have the
> proper includes to log using these macros.
> // RAW_VLOG(......)

Done.

http://codereview.chromium.org/9311003/diff/73001/third_party/tcmalloc/chromi...
File third_party/tcmalloc/chromium/src/base/spinlock_win32-inl.h (right):

http://codereview.chromium.org/9311003/diff/73001/third_party/tcmalloc/chromi...
third_party/tcmalloc/chromium/src/base/spinlock_win32-inl.h:47: //
base/atomicops-internals-windows.h doesn't support 64-bit operations.
On 2012/02/28 22:15:49, jar wrote:
> Please supply commented out version of original code.

Done.

http://codereview.chromium.org/9311003/diff/73001/third_party/tcmalloc/chromi...
File third_party/tcmalloc/chromium/src/common.h (right):

http://codereview.chromium.org/9311003/diff/73001/third_party/tcmalloc/chromi...
third_party/tcmalloc/chromium/src/common.h:89: static const size_t kMaxSize    =
256 * 1024;
On 2012/02/28 22:15:49, jar wrote:
> All these size changes have a potentially large impact on performance.
> 
> kMaxSize went from 32K to 256K
> PageSize went from 4 k to 8K or 32K

Will check impact on performance.

http://codereview.chromium.org/9311003/diff/73001/third_party/tcmalloc/chromi...
File third_party/tcmalloc/chromium/src/tcmalloc.cc (right):

http://codereview.chromium.org/9311003/diff/73001/third_party/tcmalloc/chromi...
third_party/tcmalloc/chromium/src/tcmalloc.cc:150: // Some windows file
somewhere (at least on cygwin) #define's small (!)
On 2012/02/28 22:15:49, jar wrote:
> Comment that <windows.h> appears to #define small char

Done.

http://codereview.chromium.org/9311003/diff/73001/third_party/tcmalloc/chromi...
File third_party/tcmalloc/chromium/src/thread_cache.h (right):

http://codereview.chromium.org/9311003/diff/73001/third_party/tcmalloc/chromi...
third_party/tcmalloc/chromium/src/thread_cache.h:213: return SLL_Next(&list_);
On 2012/02/28 22:15:49, jar wrote:
> Probably add FL_Next() instead of SLL_next.

Done.

[jar (doing other things), 2012-02-28 23:24:08]

LGTM for base and TCMalloc code.

As per discussion, please carefully monitor perf waterfalls when you land.  I'd
even suggest landing and reverting at a "quiet time" to get a clean demo that
there is no apparent regression.  Be sure to look at both time, and memory usage
in the perf bots.

Thanks.

[Dai Mikurube (NOT FULLTIME), 2012-02-28 23:56:39]

+tony@

Mike (mbelshe@) may not looking at it...  Could you PTAL at changes in
webkit/extensions/v8/*?

[tony, 2012-02-29 00:13:04]

webkit/ LGTM

[jam, 2012-02-29 00:32:14]

content lgtm

[Dai Mikurube (NOT FULLTIME), 2012-02-29 01:04:32]

Ahh, some _tests in Debug builds report memory corruption in FL_Next...  It's
strange corruption in _tests, but no corruption in Chrome binaries.

I'll investigate it.

[Dai Mikurube (NOT FULLTIME), 2012-02-29 01:12:51]

On 2012/02/29 01:04:32, Dai Mikurube wrote:
> Ahh, some _tests in Debug builds report memory corruption in FL_Next...  It's
> strange corruption in _tests, but no corruption in Chrome binaries.
> 
> I'll investigate it.

It comes from gtest.cc.

...
#3  0x00000000029cb9d8 in tcmalloc::FL_Next (t=0x7ffff7f3b098) at
third_party/tcmalloc/chromium/src/free_list.cc:117
#4  0x00000000029c1a78 in tcmalloc::ThreadCache::FreeList::Next
(this=0x7ffff7f3b098)
    at third_party/tcmalloc/chromium/src/thread_cache.h:213
#5  0x00000000029c1d61 in tcmalloc::ThreadCache::Deallocate
(this=0x7ffff7f3b000, ptr=0x7ffff7e43440, cl=4)
    at third_party/tcmalloc/chromium/src/thread_cache.h:372
#6  0x00000000029bff27 in (anonymous namespace)::do_free_with_callback
(ptr=0x7ffff7e43440, 
    invalid_free_fn=0x29be01c <(anonymous namespace)::InvalidFree(void*)>) at
third_party/tcmalloc/chromium/src/tcmalloc.cc:1166
#7  0x00000000029c01bd in (anonymous namespace)::do_free (ptr=0x7ffff7e43440) at
third_party/tcmalloc/chromium/src/tcmalloc.cc:1188
#8  0x000000000572cc6b in tc_delete (p=0x7ffff7e43440) at
third_party/tcmalloc/chromium/src/tcmalloc.cc:1589
#9  0x00007ffff119f129 in _M_dispose (__a=<optimized out>, this=<optimized out>)
    at
/build/buildd/gcc-4.4-4.4.3/build/x86_64-linux-gnu/libstdc++-v3/include/bits/basic_string.h:231
#10 std::basic_string<char, std::char_traits<char>, std::allocator<char>
>::~basic_string (this=<optimized out>, 
    __in_chrg=<optimized out>) at
/build/buildd/gcc-4.4-4.4.3/build/x86_64-linux-gnu/libstdc++-v3/include/bits/basic_string.h:498
#11 0x00000000024c0af1 in testing::internal::StringStreamToString
(ss=0x7ffff7e8be00) at testing/gtest/src/gtest.cc:1751
#12 0x00000000004415d6 in testing::Message::GetString (this=0x7fffffffdf00) at
testing/gtest/include/gtest/gtest-message.h:191
#13 0x00000000024ba363 in testing::internal::FlagToEnvVar (flag=0x5b068ba
"also_run_disabled_tests")
    at testing/gtest/src/gtest-port.cc:652
#14 0x00000000024ba664 in testing::internal::BoolFromGTestEnv (flag=0x5b068ba
"also_run_disabled_tests", default_value=false)
    at testing/gtest/src/gtest-port.cc:708
#15 0x00000000024c9038 in __static_initialization_and_destruction_0
(__initialize_p=1, __priority=65535)
    at testing/gtest/src/gtest.cc:185
#16 0x00000000024c923c in global constructors keyed to gtest.cc(void) () at
testing/gtest/src/gtest.cc:4898
...

[Dai Mikurube (NOT FULLTIME), 2012-02-29 02:07:56]

Ah, looks like not coming from the latest tcmalloc, but from gtest itself.

Just adding the same check in ThreadCache::Deallocate causes the same memory
corruption error:
* http://codereview.chromium.org/9528002/
* http://crbug.com/116201

It means... memory was actually corrupted in some _tests for long time? ;)

[Dai Mikurube (NOT FULLTIME), 2012-02-29 02:22:02]

Jim and reviewers,

I wonder if you could give me some advice on the gtest issue.

This tcmalloc update is desired quickly, but updating gtest upstream will take
long time.

I'd like to choose the option below.  What do you think?

1) Comment-out the newly-added ASSERT in thread_cache.h.
2) Add TODO to revive it.
3) Commit this tcmalloc update (after checking performance).
4) Try investigating gtest's issue in background.
5) Fix it and re-enable the ASSERT and later.

It doesn't make the code more vulnerable since it's only in tests (gtest) and
the current Chromium doesn't contain the ASSERT.

[jar (doing other things), 2012-02-29 17:45:33]

If your analysis is correct, and gtest is corrupting our memory, then it is
causing other tests to fail (probably in a flaky way).

Can you explain why the current TCMalloc code is not detecting the
corruption?  I *thought* the assertion in question was already implemented
at a lower level in our branch (but I may be confused about the assertion).

thanks,

Jim


On Tue, Feb 28, 2012 at 6:22 PM, <dmikurube@chromium.org> wrote:

> Jim and reviewers,
>
> I wonder if you could give me some advice on the gtest issue.
>
> This tcmalloc update is desired quickly, but updating gtest upstream will
> take
> long time.
>
> I'd like to choose the option below.  What do you think?
>
> 1) Comment-out the newly-added ASSERT in thread_cache.h.
> 2) Add TODO to revive it.
> 3) Commit this tcmalloc update (after checking performance).
> 4) Try investigating gtest's issue in background.
> 5) Fix it and re-enable the ASSERT and later.
>
> It doesn't make the code more vulnerable since it's only in tests (gtest)
> and
> the current Chromium doesn't contain the ASSERT.
>
>
http://codereview.chromium.**org/9311003/<http://codereview.chromium.org/9311...
>

[Dai Mikurube (NOT FULLTIME), 2012-02-29 18:40:43]

I'm still not sure why it happens and why the current code doesn't detect it...


I'm guessing that this corruption happens when declaring & destructing
(allocating and deallocating) variables from *static scope* though I haven't
made sure yet.

For example, the following code snippet in global scope causes the memory
corruption when exiting (destructing) foo():

--- code snippet ---
int foo() {
  std::string s = std::string("foobar");
  return s.length();
}

int l = foo();
--- code snippet ---


On 2012/02/29 17:45:33, jar wrote:
> If your analysis is correct, and gtest is corrupting our memory, then it is
> causing other tests to fail (probably in a flaky way).
> 
> Can you explain why the current TCMalloc code is not detecting the
> corruption?  I *thought* the assertion in question was already implemented
> at a lower level in our branch (but I may be confused about the assertion).
> 
> thanks,
> 
> Jim
> 
> 
> On Tue, Feb 28, 2012 at 6:22 PM, <mailto:dmikurube@chromium.org> wrote:
> 
> > Jim and reviewers,
> >
> > I wonder if you could give me some advice on the gtest issue.
> >
> > This tcmalloc update is desired quickly, but updating gtest upstream will
> > take
> > long time.
> >
> > I'd like to choose the option below.  What do you think?
> >
> > 1) Comment-out the newly-added ASSERT in thread_cache.h.
> > 2) Add TODO to revive it.
> > 3) Commit this tcmalloc update (after checking performance).
> > 4) Try investigating gtest's issue in background.
> > 5) Fix it and re-enable the ASSERT and later.
> >
> > It doesn't make the code more vulnerable since it's only in tests (gtest)
> > and
> > the current Chromium doesn't contain the ASSERT.
> >
> >
>
http://codereview.chromium.**org/9311003/%3Chttp://codereview.chromium.org/93...>
> >

[Dai Mikurube (NOT FULLTIME), 2012-02-29 21:29:43]

Ah, no.  This run
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux&numb...
(with only adding ASSERT, not updating tcmalloc) looks like having some green
tests, but actually they all are failing.

For example, test_shell_tests is green, but it has many "Memory corruption
detected."  The chrome binary is corrupting, too.

[Dai Mikurube (NOT FULLTIME), 2012-02-29 22:52:03]

It's failing not at the ASSERT, but at MEMORY_CHECK in FL_Next() before ASSERT. 
It means ' list->Next()->Previous() != list '.

I suspect that this new ASSERT doesn't assume this condition...  What do you
think?  I'll be looking into free_list.{cc|h}.


On 2012/02/29 21:29:43, Dai Mikurube wrote:
> Ah, no.  This run
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux&numb...
> (with only adding ASSERT, not updating tcmalloc) looks like having some green
> tests, but actually they all are failing.
> 
> For example, test_shell_tests is green, but it has many "Memory corruption
> detected."  The chrome binary is corrupting, too.

[jar (doing other things), 2012-03-01 01:35:05]

http://codereview.chromium.org/9311003/diff/73001/third_party/tcmalloc/chromi...
File third_party/tcmalloc/chromium/src/thread_cache.h (right):

http://codereview.chromium.org/9311003/diff/73001/third_party/tcmalloc/chromi...
third_party/tcmalloc/chromium/src/thread_cache.h:372: ASSERT(ptr !=
list->Next());
Since you're having trouble with this assert... I looked closer... and it looks
to be correct to use FL_Next() in the  implementation of Next().

The list that we got is a FreeList found in line 365.  That list should be
completely well formed.  (i.e., there is no chance this is just a partially
formed list mis-firing an assertion).

The assertion in the implementation of FL_Next() (which we forward Next() into)
first checks (without asserting) if there is a next item, and if so, then it
traps if the follower does not point back to the current item via a prev link. 
That should definitely be valid in the heap (where we found the free list) at
all times.  It should be a nice clean invariant.

[Dai Mikurube (NOT FULLTIME), 2012-03-01 01:54:04]

http://codereview.chromium.org/9311003/diff/73001/third_party/tcmalloc/chromi...
File third_party/tcmalloc/chromium/src/thread_cache.h (right):

http://codereview.chromium.org/9311003/diff/73001/third_party/tcmalloc/chromi...
third_party/tcmalloc/chromium/src/thread_cache.h:372: ASSERT(ptr !=
list->Next());
Thanks, Jim.

I agree.  It looks that the list SHOULD be well formed at all times (at least at
the ASSERT).  But, I observed a case that :

FL_Previous_No_Check(FL_Next_No_Check(t)) != t
(i.e. FL_Previous_No_Check(FL_Next_No_Check(t)) == NULL
   && FL_Next_No_Check(t) != NULL)

at FL_Next() while I was diving with gdb into the code ONLY with the ASSERT
(http://codereview.chromium.org/9528002/).

If my observation is correct, it may be a real corruption... I'm looking at this
deeper.


On 2012/03/01 01:35:05, jar wrote:
> Since you're having trouble with this assert... I looked closer... and it
looks
> to be correct to use FL_Next() in the  implementation of Next().
> 
> The list that we got is a FreeList found in line 365.  That list should be
> completely well formed.  (i.e., there is no chance this is just a partially
> formed list mis-firing an assertion).
> 
> The assertion in the implementation of FL_Next() (which we forward Next()
into)
> first checks (without asserting) if there is a next item, and if so, then it
> traps if the follower does not point back to the current item via a prev link.

> That should definitely be valid in the heap (where we found the free list) at
> all times.  It should be a nice clean invariant.

[Dai Mikurube (NOT FULLTIME), 2012-03-01 02:29:17]

I think the FL_Next() issue is independent from this TCMalloc update change.   I
created a bug and a test code at :

* http://crbug.com/116201
* http://codereview.chromium.org/9528002/

Let's discuss there.

[jar (doing other things), 2012-03-01 02:33:41]

I think it is a separable issue... but IMO, you should not land while that issue
is pending.

Also, please look at your merge.  There are a number of places where (as I
recall) an abort() was replaced by an exit(1).  Apparently, we need the abort to
trigger our crash dumps, so you'll need to adjust that part of the integration
in this CL (before landing).

Thanks!

On 2012/03/01 02:29:17, Dai Mikurube wrote:
> I think the FL_Next() issue is independent from this TCMalloc update change.  
I
> created a bug and a test code at :
> 
> * http://crbug.com/116201
> * http://codereview.chromium.org/9528002/
> 
> Let's discuss there.

[Dai Mikurube (NOT FULLTIME), 2012-03-01 04:45:06]

Thank you for the comment.

Agreed.  I don't land it till this issue is fixed.

And, ok, I'll change abort()s back to exit(1) in our Chromium branch.


On 2012/03/01 02:33:41, jar wrote:
> I think it is a separable issue... but IMO, you should not land while that
issue
> is pending.
> 
> Also, please look at your merge.  There are a number of places where (as I
> recall) an abort() was replaced by an exit(1).  Apparently, we need the abort
to
> trigger our crash dumps, so you'll need to adjust that part of the integration
> in this CL (before landing).
> 
> Thanks!
> 
> On 2012/03/01 02:29:17, Dai Mikurube wrote:
> > I think the FL_Next() issue is independent from this TCMalloc update change.
 
> I
> > created a bug and a test code at :
> > 
> > * http://crbug.com/116201
> > * http://codereview.chromium.org/9528002/
> > 
> > Let's discuss there.

[Dai Mikurube (NOT FULLTIME), 2012-03-01 23:48:09]

Fixed the issue based on knowledge from http://crbug.com/116201 and
http://codereview.chromium.org/9528002/.

[Dai Mikurube (NOT FULLTIME), 2012-03-03 21:01:52]

Ah, I did the updated TCMalloc performance experiments, but I forgot to commit
from another issue...  This issue is closed.

Please look at the new issue http://codereview.chromium.org/9584046/ for the
TCMalloc update.