Adds TCMalloc support for Android.

Adds TCMalloc support for Android.

This is part of the effort to bring TCMalloc to android.
The first goal is to get instrumentation to facilitate 
integration with DMP and memory profiling.
This is not yet intended for full production usage as the
default allocator.

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

[bulach, 2013-04-29 15:43:14]

warning! this is *nowhere* near ready. :)

however, adding android_use_tcmalloc=1 to GYP_DEFINES, it allows building
"allocator" (target from base/allocator.gyp) and content shell using TCMalloc..

I stress "building" :) it obviously doesn't run, dies with a SEGV super early
on.. but anyways, I thought you may want to take an initial look into it.

[digit1, 2013-04-29 16:11:27]

lgtm - seems a good start though :-)

For ucontext, see what I've done for Breakpad under breakpad/common/android/,
you should be able to do something similar to get the PC.

[Peter Beverloo, 2013-04-29 17:03:30]

A few minor drive-by nits.  Cool to see this being worked on!

https://codereview.chromium.org/14321006/diff/1/content/content_shell.gypi
File content/content_shell.gypi (right):

https://codereview.chromium.org/14321006/diff/1/content/content_shell.gypi#ne...
content/content_shell.gypi:196: ['OS!="android"', {
Why don't you add the block on 191-195 below line 292? That allows you to keep
the else.

https://codereview.chromium.org/14321006/diff/1/third_party/tcmalloc/README.c...
File third_party/tcmalloc/README.chromium (right):

https://codereview.chromium.org/14321006/diff/1/third_party/tcmalloc/README.c...
third_party/tcmalloc/README.chromium:89: - Add support for android.
nit: s/Add/Added/. s/android/Android/.

https://codereview.chromium.org/14321006/diff/1/third_party/tcmalloc/chromium...
File third_party/tcmalloc/chromium/src/base/sysinfo.cc (right):

https://codereview.chromium.org/14321006/diff/1/third_party/tcmalloc/chromium...
third_party/tcmalloc/chromium/src/base/sysinfo.cc:499: #elif defined(__linux__)
It defaults to true by default.  We can avoid this extra branch by changing the
original define to read "#if defined(__linux__) and !defined(__ANDROID__)".

https://codereview.chromium.org/14321006/diff/1/third_party/tcmalloc/chromium...
File third_party/tcmalloc/chromium/src/libc_override_gcc_and_weak_bionic.h
(right):

https://codereview.chromium.org/14321006/diff/1/third_party/tcmalloc/chromium...
third_party/tcmalloc/chromium/src/libc_override_gcc_and_weak_bionic.h:56: void*
operator new(size_t size) throw (/* std::bad_alloc */)
It would be helpful to add a comment as to why these have to be commented out (I
suspect Bionic doesn't throw?)

[Dai Mikurube (NOT FULLTIME), 2013-04-29 17:28:00]

almost looks good, but not sure of the crash.

added a comment about debugallocation in the bug 162208.

[bulach, 2013-04-30 18:43:07]

forgot to mention that comments so far have been addressed..
it's still WIP, but anyways..

https://codereview.chromium.org/14321006/diff/1/content/content_shell.gypi
File content/content_shell.gypi (right):

https://codereview.chromium.org/14321006/diff/1/content/content_shell.gypi#ne...
content/content_shell.gypi:196: ['OS!="android"', {
On 2013/04/29 17:03:31, Peter Beverloo wrote:
> Why don't you add the block on 191-195 below line 292? That allows you to keep
> the else.

Done.

https://codereview.chromium.org/14321006/diff/1/third_party/tcmalloc/README.c...
File third_party/tcmalloc/README.chromium (right):

https://codereview.chromium.org/14321006/diff/1/third_party/tcmalloc/README.c...
third_party/tcmalloc/README.chromium:89: - Add support for android.
On 2013/04/29 17:03:31, Peter Beverloo wrote:
> nit: s/Add/Added/. s/android/Android/.

Done.

https://codereview.chromium.org/14321006/diff/1/third_party/tcmalloc/chromium...
File third_party/tcmalloc/chromium/src/base/sysinfo.cc (right):

https://codereview.chromium.org/14321006/diff/1/third_party/tcmalloc/chromium...
third_party/tcmalloc/chromium/src/base/sysinfo.cc:499: #elif defined(__linux__)
On 2013/04/29 17:03:31, Peter Beverloo wrote:
> It defaults to true by default.  We can avoid this extra branch by changing
the
> original define to read "#if defined(__linux__) and !defined(__ANDROID__)".

Done.

https://codereview.chromium.org/14321006/diff/1/third_party/tcmalloc/chromium...
File third_party/tcmalloc/chromium/src/libc_override_gcc_and_weak_bionic.h
(right):

https://codereview.chromium.org/14321006/diff/1/third_party/tcmalloc/chromium...
third_party/tcmalloc/chromium/src/libc_override_gcc_and_weak_bionic.h:56: void*
operator new(size_t size) throw (/* std::bad_alloc */)
On 2013/04/29 17:03:31, Peter Beverloo wrote:
> It would be helpful to add a comment as to why these have to be commented out
(I
> suspect Bionic doesn't throw?)

Done.

[bulach, 2013-05-02 17:42:04]

dai: patchset #5 contains just my patch.

locally I have it on top of all your patches, so it should be trivial for you to
cherry-pick it as well...

once we have all of these up and running, we can then work out on how to best
split it. thanks again for all your help!

[Dai Mikurube (NOT FULLTIME), 2013-05-03 08:52:59]

Thanks, bulach@  Some nit comments.

https://codereview.chromium.org/14321006/diff/26001/third_party/tcmalloc/chro...
File third_party/tcmalloc/chromium/src/base/commandlineflags.h (right):

https://codereview.chromium.org/14321006/diff/26001/third_party/tcmalloc/chro...
third_party/tcmalloc/chromium/src/base/commandlineflags.h:3: //
nit: unnecessary deletion of whitespace

https://codereview.chromium.org/14321006/diff/26001/third_party/tcmalloc/chro...
third_party/tcmalloc/chromium/src/base/commandlineflags.h:124: static char
g_system_property_value[PROP_VALUE_MAX];
It's not required with my recent patch.

https://codereview.chromium.org/14321006/diff/26001/third_party/tcmalloc/chro...
File third_party/tcmalloc/chromium/src/base/spinlock_internal.cc (right):

https://codereview.chromium.org/14321006/diff/26001/third_party/tcmalloc/chro...
third_party/tcmalloc/chromium/src/base/spinlock_internal.cc:50: #elif
defined(__linux__) && !defined(__ANDROID__)
spinlock_linux-inl doesn't work?  How about adding a TODO since we may finally
want to use linux version.

https://codereview.chromium.org/14321006/diff/26001/third_party/tcmalloc/chro...
File third_party/tcmalloc/chromium/src/base/sysinfo.cc (right):

https://codereview.chromium.org/14321006/diff/26001/third_party/tcmalloc/chro...
third_party/tcmalloc/chromium/src/base/sysinfo.cc:3: //
nit: unnecessary whitespace deletion

https://codereview.chromium.org/14321006/diff/26001/third_party/tcmalloc/chro...
third_party/tcmalloc/chromium/src/base/sysinfo.cc:506: #if defined(__linux__) &&
!defined(__ANDROID__)
just curious.  what happens without this?

https://codereview.chromium.org/14321006/diff/26001/third_party/tcmalloc/chro...
File third_party/tcmalloc/chromium/src/config_android.h (right):

https://codereview.chromium.org/14321006/diff/26001/third_party/tcmalloc/chro...
third_party/tcmalloc/chromium/src/config_android.h:143: #undef
HAVE_SYS_SYSCALL_H
Actually we have sys/syscall.h.

https://codereview.chromium.org/14321006/diff/26001/third_party/tcmalloc/chro...
File third_party/tcmalloc/chromium/src/deep-heap-profile.cc (right):

https://codereview.chromium.org/14321006/diff/26001/third_party/tcmalloc/chro...
third_party/tcmalloc/chromium/src/deep-heap-profile.cc:643: 
nit: empty line

https://codereview.chromium.org/14321006/diff/26001/third_party/tcmalloc/chro...
File third_party/tcmalloc/chromium/src/getpc.h (right):

https://codereview.chromium.org/14321006/diff/26001/third_party/tcmalloc/chro...
third_party/tcmalloc/chromium/src/getpc.h:114: #if defined(__linux) &&
defined(__i386) && defined(__GNUC__)
I'm not sure if we have x86 Android, but we may want !defined(__ANDROID__) here?

https://codereview.chromium.org/14321006/diff/26001/third_party/tcmalloc/chro...
third_party/tcmalloc/chromium/src/getpc.h:144: for (int i = 0; i <
sizeof(CallUnrollInfo)/sizeof(*callunrollinfo); ++i) {
It's necessary?  (Since tcmalloc is a third_party library, I'd like to avoid
unnecessary changes.)

https://codereview.chromium.org/14321006/diff/26001/third_party/tcmalloc/chro...
File third_party/tcmalloc/chromium/src/malloc_hook.cc (right):

https://codereview.chromium.org/14321006/diff/26001/third_party/tcmalloc/chro...
third_party/tcmalloc/chromium/src/malloc_hook.cc:3: //
nit: unnecessary deletion

https://codereview.chromium.org/14321006/diff/26001/third_party/tcmalloc/chro...
File third_party/tcmalloc/chromium/src/maybe_threads.cc (right):

https://codereview.chromium.org/14321006/diff/26001/third_party/tcmalloc/chro...
third_party/tcmalloc/chromium/src/maybe_threads.cc:3: //
nit: unnecessary deletion

https://codereview.chromium.org/14321006/diff/26001/third_party/tcmalloc/chro...
third_party/tcmalloc/chromium/src/maybe_threads.cc:129: sizeof(*ctl)) == 0) {
Is it necessary for Android?

https://codereview.chromium.org/14321006/diff/26001/third_party/tcmalloc/chro...
File third_party/tcmalloc/chromium/src/memory_region_map.cc (right):

https://codereview.chromium.org/14321006/diff/26001/third_party/tcmalloc/chro...
third_party/tcmalloc/chromium/src/memory_region_map.cc:3: *
nit: unnecessary deletion

https://codereview.chromium.org/14321006/diff/26001/third_party/tcmalloc/chro...
File third_party/tcmalloc/chromium/src/memory_region_map.h (right):

https://codereview.chromium.org/14321006/diff/26001/third_party/tcmalloc/chro...
third_party/tcmalloc/chromium/src/memory_region_map.h:3: *
nit: unnecessary deletion

https://codereview.chromium.org/14321006/diff/26001/third_party/tcmalloc/chro...
File third_party/tcmalloc/chromium/src/profiler.cc (right):

https://codereview.chromium.org/14321006/diff/26001/third_party/tcmalloc/chro...
third_party/tcmalloc/chromium/src/profiler.cc:53: #elif !defined(__ANDROID__)
We may want a comment that describes nothing required in Android.  (Or, can we
keep it as is?)

https://codereview.chromium.org/14321006/diff/26001/third_party/tcmalloc/chro...
File third_party/tcmalloc/chromium/src/stacktrace_config.h (right):

https://codereview.chromium.org/14321006/diff/26001/third_party/tcmalloc/chro...
third_party/tcmalloc/chromium/src/stacktrace_config.h:3: //
nit: unnecessary deletion

https://codereview.chromium.org/14321006/diff/26001/third_party/tcmalloc/chro...
third_party/tcmalloc/chromium/src/stacktrace_config.h:73: 
nit: how about removing the empty line (line 73)?  (not mandatory)

[Dai Mikurube (NOT FULLTIME), 2013-05-03 11:02:35]

Some more comments on stacktrace_android-inl.h.

https://codereview.chromium.org/14321006/diff/26001/third_party/tcmalloc/chro...
File third_party/tcmalloc/chromium/src/stacktrace_android-inl.h (right):

https://codereview.chromium.org/14321006/diff/26001/third_party/tcmalloc/chro...
third_party/tcmalloc/chromium/src/stacktrace_android-inl.h:41: #include
<unwind.h>
Import comments mentioning http://crbug.com/236855.

https://codereview.chromium.org/14321006/diff/26001/third_party/tcmalloc/chro...
third_party/tcmalloc/chromium/src/stacktrace_android-inl.h:64: uintptr_t ip =
_Unwind_GetIP(context);
Need to import the hack for Clang.  (See the latest patch
https://codereview.chromium.org/14646012/.  Some portions are changed.)

https://codereview.chromium.org/14321006/diff/26001/third_party/tcmalloc/chro...
third_party/tcmalloc/chromium/src/stacktrace_android-inl.h:102:
reinterpret_cast<uintptr_t*>(result), max_depth, skip_count);
nit: indent by 4

https://codereview.chromium.org/14321006/diff/26001/third_party/tcmalloc/chro...
third_party/tcmalloc/chromium/src/stacktrace_android-inl.h:103:
_Unwind_Backtrace(tracer, &state);
We need
return state.frame_count;
here.  (It's the reason why the lengths of backtraces were always 5.)

[bulach, 2013-05-07 14:55:51]

thanks dai! all comments addressed, ptal.

https://codereview.chromium.org/14321006/diff/26001/third_party/tcmalloc/chro...
File third_party/tcmalloc/chromium/src/base/commandlineflags.h (right):

https://codereview.chromium.org/14321006/diff/26001/third_party/tcmalloc/chro...
third_party/tcmalloc/chromium/src/base/commandlineflags.h:3: //
On 2013/05/03 08:53:00, Dai Mikurube wrote:
> nit: unnecessary deletion of whitespace

my editor is configured to trim whitespaces automatically... :(
however, with your latest edits this file has been reverted anwyays..

https://codereview.chromium.org/14321006/diff/26001/third_party/tcmalloc/chro...
third_party/tcmalloc/chromium/src/base/commandlineflags.h:124: static char
g_system_property_value[PROP_VALUE_MAX];
On 2013/05/03 08:53:00, Dai Mikurube wrote:
> It's not required with my recent patch.

Done.

https://codereview.chromium.org/14321006/diff/26001/third_party/tcmalloc/chro...
File third_party/tcmalloc/chromium/src/base/spinlock_internal.cc (right):

https://codereview.chromium.org/14321006/diff/26001/third_party/tcmalloc/chro...
third_party/tcmalloc/chromium/src/base/spinlock_internal.cc:50: #elif
defined(__linux__) && !defined(__ANDROID__)
On 2013/05/03 08:53:00, Dai Mikurube wrote:
> spinlock_linux-inl doesn't work?  How about adding a TODO since we may finally
> want to use linux version.

good point! I mistakenly "un"defined HAVE_SYS_SYSCALL, and broke this. since
android does have sys/syscall, removed this here.

https://codereview.chromium.org/14321006/diff/26001/third_party/tcmalloc/chro...
File third_party/tcmalloc/chromium/src/base/sysinfo.cc (right):

https://codereview.chromium.org/14321006/diff/26001/third_party/tcmalloc/chro...
third_party/tcmalloc/chromium/src/base/sysinfo.cc:3: //
On 2013/05/03 08:53:00, Dai Mikurube wrote:
> nit: unnecessary whitespace deletion

Done.

https://codereview.chromium.org/14321006/diff/26001/third_party/tcmalloc/chro...
third_party/tcmalloc/chromium/src/base/sysinfo.cc:506: #if defined(__linux__) &&
!defined(__ANDROID__)
On 2013/05/03 08:53:00, Dai Mikurube wrote:
> just curious.  what happens without this?

'confstr' was not declared in this scope.
I suppose that pthread.h is what it's meant by "POSIX thread" here?

https://codereview.chromium.org/14321006/diff/26001/third_party/tcmalloc/chro...
File third_party/tcmalloc/chromium/src/config_android.h (right):

https://codereview.chromium.org/14321006/diff/26001/third_party/tcmalloc/chro...
third_party/tcmalloc/chromium/src/config_android.h:143: #undef
HAVE_SYS_SYSCALL_H
On 2013/05/03 08:53:00, Dai Mikurube wrote:
> Actually we have sys/syscall.h.

good point, done.

https://codereview.chromium.org/14321006/diff/26001/third_party/tcmalloc/chro...
File third_party/tcmalloc/chromium/src/deep-heap-profile.cc (right):

https://codereview.chromium.org/14321006/diff/26001/third_party/tcmalloc/chro...
third_party/tcmalloc/chromium/src/deep-heap-profile.cc:643: 
On 2013/05/03 08:53:00, Dai Mikurube wrote:
> nit: empty line

Done.

https://codereview.chromium.org/14321006/diff/26001/third_party/tcmalloc/chro...
File third_party/tcmalloc/chromium/src/getpc.h (right):

https://codereview.chromium.org/14321006/diff/26001/third_party/tcmalloc/chro...
third_party/tcmalloc/chromium/src/getpc.h:114: #if defined(__linux) &&
defined(__i386) && defined(__GNUC__)
On 2013/05/03 08:53:00, Dai Mikurube wrote:
> I'm not sure if we have x86 Android, but we may want !defined(__ANDROID__)
here?

we do have x86, added the guard here.

https://codereview.chromium.org/14321006/diff/26001/third_party/tcmalloc/chro...
third_party/tcmalloc/chromium/src/getpc.h:144: for (int i = 0; i <
sizeof(CallUnrollInfo)/sizeof(*callunrollinfo); ++i) {
On 2013/05/03 08:53:00, Dai Mikurube wrote:
> It's necessary?  (Since tcmalloc is a third_party library, I'd like to avoid
> unnecessary changes.)

Done.

https://codereview.chromium.org/14321006/diff/26001/third_party/tcmalloc/chro...
File third_party/tcmalloc/chromium/src/malloc_hook.cc (right):

https://codereview.chromium.org/14321006/diff/26001/third_party/tcmalloc/chro...
third_party/tcmalloc/chromium/src/malloc_hook.cc:3: //
On 2013/05/03 08:53:00, Dai Mikurube wrote:
> nit: unnecessary deletion

Done.

https://codereview.chromium.org/14321006/diff/26001/third_party/tcmalloc/chro...
File third_party/tcmalloc/chromium/src/maybe_threads.cc (right):

https://codereview.chromium.org/14321006/diff/26001/third_party/tcmalloc/chro...
third_party/tcmalloc/chromium/src/maybe_threads.cc:3: //
On 2013/05/03 08:53:00, Dai Mikurube wrote:
> nit: unnecessary deletion

Done.

https://codereview.chromium.org/14321006/diff/26001/third_party/tcmalloc/chro...
third_party/tcmalloc/chromium/src/maybe_threads.cc:129: sizeof(*ctl)) == 0) {
On 2013/05/03 08:53:00, Dai Mikurube wrote:
> Is it necessary for Android?

yep.. memcmp takes "const void*", the arguments here are "void volatile*" and
fails with:
maybe_threads.cc:129:28: error: invalid conversion from 'void volatile*' to
'void const*' [-fpermissive]
/usr/local/google/code/clankium-master/src/third_party/android_tools/ndk//platforms/android-14/arch-arm/usr/include/../include/string.h:40:15:
error:   initializing argument 2 of 'int memcmp(void const*, void const*,
size_t)' [-fpermissive]

https://codereview.chromium.org/14321006/diff/26001/third_party/tcmalloc/chro...
File third_party/tcmalloc/chromium/src/profiler.cc (right):

https://codereview.chromium.org/14321006/diff/26001/third_party/tcmalloc/chro...
third_party/tcmalloc/chromium/src/profiler.cc:53: #elif !defined(__ANDROID__)
On 2013/05/03 08:53:00, Dai Mikurube wrote:
> We may want a comment that describes nothing required in Android.  (Or, can we
> keep it as is?)

added a #include <unwind.h> similar to the cases above.

https://codereview.chromium.org/14321006/diff/26001/third_party/tcmalloc/chro...
File third_party/tcmalloc/chromium/src/stacktrace_android-inl.h (right):

https://codereview.chromium.org/14321006/diff/26001/third_party/tcmalloc/chro...
third_party/tcmalloc/chromium/src/stacktrace_android-inl.h:41: #include
<unwind.h>
On 2013/05/03 11:02:35, Dai Mikurube wrote:
> Import comments mentioning http://crbug.com/236855.

Done.

https://codereview.chromium.org/14321006/diff/26001/third_party/tcmalloc/chro...
third_party/tcmalloc/chromium/src/stacktrace_android-inl.h:64: uintptr_t ip =
_Unwind_GetIP(context);
On 2013/05/03 11:02:35, Dai Mikurube wrote:
> Need to import the hack for Clang.  (See the latest patch
> https://codereview.chromium.org/14646012/.  Some portions are changed.)

Done.

https://codereview.chromium.org/14321006/diff/26001/third_party/tcmalloc/chro...
third_party/tcmalloc/chromium/src/stacktrace_android-inl.h:102:
reinterpret_cast<uintptr_t*>(result), max_depth, skip_count);
On 2013/05/03 11:02:35, Dai Mikurube wrote:
> nit: indent by 4

Done.

https://codereview.chromium.org/14321006/diff/26001/third_party/tcmalloc/chro...
third_party/tcmalloc/chromium/src/stacktrace_android-inl.h:103:
_Unwind_Backtrace(tracer, &state);
On 2013/05/03 11:02:35, Dai Mikurube wrote:
> We need
> return state.frame_count;
> here.  (It's the reason why the lengths of backtraces were always 5.)

ahn, great!!! done.

https://codereview.chromium.org/14321006/diff/26001/third_party/tcmalloc/chro...
File third_party/tcmalloc/chromium/src/stacktrace_config.h (right):

https://codereview.chromium.org/14321006/diff/26001/third_party/tcmalloc/chro...
third_party/tcmalloc/chromium/src/stacktrace_config.h:3: //
On 2013/05/03 08:53:00, Dai Mikurube wrote:
> nit: unnecessary deletion

Done.

https://codereview.chromium.org/14321006/diff/26001/third_party/tcmalloc/chro...
third_party/tcmalloc/chromium/src/stacktrace_config.h:73: 
On 2013/05/03 08:53:00, Dai Mikurube wrote:
> nit: how about removing the empty line (line 73)?  (not mandatory)

Done.

[Dai Mikurube (NOT FULLTIME), 2013-05-08 14:53:25]

Sorry for adding some more comments that I didn't in the last look.  Mostly
nits.

https://codereview.chromium.org/14321006/diff/26001/third_party/tcmalloc/chro...
File third_party/tcmalloc/chromium/src/base/sysinfo.cc (right):

https://codereview.chromium.org/14321006/diff/26001/third_party/tcmalloc/chro...
third_party/tcmalloc/chromium/src/base/sysinfo.cc:506: #if defined(__linux__) &&
!defined(__ANDROID__)
On 2013/05/07 14:55:51, bulach wrote:
> On 2013/05/03 08:53:00, Dai Mikurube wrote:
> > just curious.  what happens without this?
> 
> 'confstr' was not declared in this scope.
> I suppose that pthread.h is what it's meant by "POSIX thread" here?

I think so.  Then, yes, we should return true on Android like this.

+ a comment on the patch set 6.

https://codereview.chromium.org/14321006/diff/40001/third_party/tcmalloc/chro...
File third_party/tcmalloc/chromium/src/base/sysinfo.cc (right):

https://codereview.chromium.org/14321006/diff/40001/third_party/tcmalloc/chro...
third_party/tcmalloc/chromium/src/base/sysinfo.cc:506: #if defined(__linux__) &&
!defined(__ANDROID__)
Let's add a comment that describes we can't do it in Android since Android
doesn't expose confstr() and we assume using POSIX threads in Android.

https://codereview.chromium.org/14321006/diff/40001/third_party/tcmalloc/chro...
File third_party/tcmalloc/chromium/src/config_android.h (right):

https://codereview.chromium.org/14321006/diff/40001/third_party/tcmalloc/chro...
third_party/tcmalloc/chromium/src/config_android.h:270: /* Do not use
std::set_new_handler */
We may want a description that "Android's NDK doesn't have (expose?)
std::set_new_handler".

https://codereview.chromium.org/14321006/diff/40001/third_party/tcmalloc/chro...
File third_party/tcmalloc/chromium/src/getpc.h (right):

https://codereview.chromium.org/14321006/diff/40001/third_party/tcmalloc/chro...
third_party/tcmalloc/chromium/src/getpc.h:178: 
nit: no empty line?  (not mandatory)

https://codereview.chromium.org/14321006/diff/40001/third_party/tcmalloc/chro...
third_party/tcmalloc/chromium/src/getpc.h:187: #else
Why did you move this #else?

https://codereview.chromium.org/14321006/diff/40001/third_party/tcmalloc/chro...
File third_party/tcmalloc/chromium/src/libc_override.h (right):

https://codereview.chromium.org/14321006/diff/40001/third_party/tcmalloc/chro...
third_party/tcmalloc/chromium/src/libc_override.h:88: #include
"libc_override_gcc_and_weak_bionic.h"
(a comment in libc_override_gcc_and_weak_bionic.h.

https://codereview.chromium.org/14321006/diff/40001/third_party/tcmalloc/chro...
File third_party/tcmalloc/chromium/src/libc_override_gcc_and_weak_bionic.h
(right):

https://codereview.chromium.org/14321006/diff/40001/third_party/tcmalloc/chro...
third_party/tcmalloc/chromium/src/libc_override_gcc_and_weak_bionic.h:57: void*
operator new(size_t size) throw (/* std::bad_alloc */)
How about simply using #ifdef instead of a new file?

https://codereview.chromium.org/14321006/diff/40001/third_party/tcmalloc/chro...
File third_party/tcmalloc/chromium/src/maybe_threads.cc (right):

https://codereview.chromium.org/14321006/diff/40001/third_party/tcmalloc/chro...
third_party/tcmalloc/chromium/src/maybe_threads.cc:129: sizeof(*ctl)) == 0) {
Ok, then let's use C++ style cast.

https://codereview.chromium.org/14321006/diff/40001/third_party/tcmalloc/chro...
File third_party/tcmalloc/chromium/src/profiler.cc (right):

https://codereview.chromium.org/14321006/diff/40001/third_party/tcmalloc/chro...
third_party/tcmalloc/chromium/src/profiler.cc:54: #include <unwind.h>
Is <unwind.h> really used?

[bulach, 2013-05-08 16:09:10]

thanks dai! all addressed, ptal

https://codereview.chromium.org/14321006/diff/40001/third_party/tcmalloc/chro...
File third_party/tcmalloc/chromium/src/base/sysinfo.cc (right):

https://codereview.chromium.org/14321006/diff/40001/third_party/tcmalloc/chro...
third_party/tcmalloc/chromium/src/base/sysinfo.cc:506: #if defined(__linux__) &&
!defined(__ANDROID__)
On 2013/05/08 14:53:25, Dai Mikurube wrote:
> Let's add a comment that describes we can't do it in Android since Android
> doesn't expose confstr() and we assume using POSIX threads in Android.

Done.

https://codereview.chromium.org/14321006/diff/40001/third_party/tcmalloc/chro...
File third_party/tcmalloc/chromium/src/config_android.h (right):

https://codereview.chromium.org/14321006/diff/40001/third_party/tcmalloc/chro...
third_party/tcmalloc/chromium/src/config_android.h:270: /* Do not use
std::set_new_handler */
On 2013/05/08 14:53:25, Dai Mikurube wrote:
> We may want a description that "Android's NDK doesn't have (expose?)
> std::set_new_handler".

Done.

https://codereview.chromium.org/14321006/diff/40001/third_party/tcmalloc/chro...
File third_party/tcmalloc/chromium/src/getpc.h (right):

https://codereview.chromium.org/14321006/diff/40001/third_party/tcmalloc/chro...
third_party/tcmalloc/chromium/src/getpc.h:178: 
On 2013/05/08 14:53:25, Dai Mikurube wrote:
> nit: no empty line?  (not mandatory)

Done.

https://codereview.chromium.org/14321006/diff/40001/third_party/tcmalloc/chro...
third_party/tcmalloc/chromium/src/getpc.h:187: #else
On 2013/05/08 14:53:25, Dai Mikurube wrote:
> Why did you move this #else?

no reason :) moved back.

https://codereview.chromium.org/14321006/diff/40001/third_party/tcmalloc/chro...
File third_party/tcmalloc/chromium/src/libc_override.h (right):

https://codereview.chromium.org/14321006/diff/40001/third_party/tcmalloc/chro...
third_party/tcmalloc/chromium/src/libc_override.h:88: #include
"libc_override_gcc_and_weak_bionic.h"
On 2013/05/08 14:53:25, Dai Mikurube wrote:
> (a comment in libc_override_gcc_and_weak_bionic.h.

Done.

https://codereview.chromium.org/14321006/diff/40001/third_party/tcmalloc/chro...
File third_party/tcmalloc/chromium/src/libc_override_gcc_and_weak_bionic.h
(right):

https://codereview.chromium.org/14321006/diff/40001/third_party/tcmalloc/chro...
third_party/tcmalloc/chromium/src/libc_override_gcc_and_weak_bionic.h:57: void*
operator new(size_t size) throw (/* std::bad_alloc */)
On 2013/05/08 14:53:25, Dai Mikurube wrote:
> How about simply using #ifdef instead of a new file?

good idea! I originally thought we'd have more diffs, but seems simpler to reuse
the linux version.

https://codereview.chromium.org/14321006/diff/40001/third_party/tcmalloc/chro...
File third_party/tcmalloc/chromium/src/maybe_threads.cc (right):

https://codereview.chromium.org/14321006/diff/40001/third_party/tcmalloc/chro...
third_party/tcmalloc/chromium/src/maybe_threads.cc:129: sizeof(*ctl)) == 0) {
On 2013/05/08 14:53:25, Dai Mikurube wrote:
> Ok, then let's use C++ style cast.

:)
it's a double-cast, and not entirely sure it'd work on all platforms if they
have different typedef's for pthread_once.
using c++ style but with an ifdef for android to be safe.

https://codereview.chromium.org/14321006/diff/40001/third_party/tcmalloc/chro...
File third_party/tcmalloc/chromium/src/profiler.cc (right):

https://codereview.chromium.org/14321006/diff/40001/third_party/tcmalloc/chro...
third_party/tcmalloc/chromium/src/profiler.cc:54: #include <unwind.h>
On 2013/05/08 14:53:25, Dai Mikurube wrote:
> Is <unwind.h> really used?

Done.

[Peter Beverloo, 2013-05-08 16:17:13]

Two minor nits :-)

https://codereview.chromium.org/14321006/diff/58001/third_party/tcmalloc/chro...
File third_party/tcmalloc/chromium/src/libc_override_gcc_and_weak.h (right):

https://codereview.chromium.org/14321006/diff/58001/third_party/tcmalloc/chro...
third_party/tcmalloc/chromium/src/libc_override_gcc_and_weak.h:58: #define
STD_BACK_ALLOC
Should this read STD_BAD_ALLOC?

https://codereview.chromium.org/14321006/diff/58001/third_party/tcmalloc/chro...
File third_party/tcmalloc/chromium/src/stacktrace_android-inl.h (right):

https://codereview.chromium.org/14321006/diff/58001/third_party/tcmalloc/chro...
third_party/tcmalloc/chromium/src/stacktrace_android-inl.h:46: /* depends how
the system includes define this */
This command is hard to read -- what exactly do you mean?  Maybe rephrase it?

[bulach, 2013-05-08 18:22:48]

thanks peter! addressed.

https://codereview.chromium.org/14321006/diff/58001/third_party/tcmalloc/chro...
File third_party/tcmalloc/chromium/src/libc_override_gcc_and_weak.h (right):

https://codereview.chromium.org/14321006/diff/58001/third_party/tcmalloc/chro...
third_party/tcmalloc/chromium/src/libc_override_gcc_and_weak.h:58: #define
STD_BACK_ALLOC
On 2013/05/08 16:17:13, Peter Beverloo wrote:
> Should this read STD_BAD_ALLOC?

duh :) done.

https://codereview.chromium.org/14321006/diff/58001/third_party/tcmalloc/chro...
File third_party/tcmalloc/chromium/src/stacktrace_android-inl.h (right):

https://codereview.chromium.org/14321006/diff/58001/third_party/tcmalloc/chro...
third_party/tcmalloc/chromium/src/stacktrace_android-inl.h:46: /* depends how
the system includes define this */
On 2013/05/08 16:17:13, Peter Beverloo wrote:
> This command is hard to read -- what exactly do you mean?  Maybe rephrase it?

Done.

[Dai Mikurube (NOT FULLTIME), 2013-05-09 16:29:28]

LGTM with a very small nit. :)

Let's show it to appropriate OWNERs (maybe willchan@ for tcmalloc and jochen@
for content_shell.gypi?) when it gets available.

(We may want to have a different subject for this change.)

https://codereview.chromium.org/14321006/diff/40001/third_party/tcmalloc/chro...
File third_party/tcmalloc/chromium/src/maybe_threads.cc (right):

https://codereview.chromium.org/14321006/diff/40001/third_party/tcmalloc/chro...
third_party/tcmalloc/chromium/src/maybe_threads.cc:129: sizeof(*ctl)) == 0) {
On 2013/05/08 16:09:10, bulach wrote:
> On 2013/05/08 14:53:25, Dai Mikurube wrote:
> > Ok, then let's use C++ style cast.
> 
> :)
> it's a double-cast, and not entirely sure it'd work on all platforms if they
> have different typedef's for pthread_once.
> using c++ style but with an ifdef for android to be safe.

wow, it's long, but I believe it's the better way... ;)

https://codereview.chromium.org/14321006/diff/69001/third_party/tcmalloc/chro...
File third_party/tcmalloc/chromium/src/libc_override_gcc_and_weak.h (right):

https://codereview.chromium.org/14321006/diff/69001/third_party/tcmalloc/chro...
third_party/tcmalloc/chromium/src/libc_override_gcc_and_weak.h:57: // Android's
bionic doesn't have std::bad_alloc
nit: a period at last.

[Dai Mikurube (NOT FULLTIME), 2013-05-09 17:04:50]

https://codereview.chromium.org/14321006/diff/69001/content/content_shell.gypi
File content/content_shell.gypi (right):

https://codereview.chromium.org/14321006/diff/69001/content/content_shell.gyp...
content/content_shell.gypi:201: }],
Ah, but finally, I'm not sure it should be added here.  Do we have a better
place common for all Android-related things?  (e.g. base/base.gyp for Linux)

[bulach, 2013-05-09 17:33:58]

+willchan: base/ and third_party/tcmalloc
+jochen: content/

thanks dai! remaining nits addressed, changed the description, sending it out
for full review.

https://codereview.chromium.org/14321006/diff/69001/content/content_shell.gypi
File content/content_shell.gypi (right):

https://codereview.chromium.org/14321006/diff/69001/content/content_shell.gyp...
content/content_shell.gypi:201: }],
On 2013/05/09 17:04:51, Dai Mikurube wrote:
> Ah, but finally, I'm not sure it should be added here.  Do we have a better
> place common for all Android-related things?  (e.g. base/base.gyp for Linux)

this is exactly the same as the block below :)
each target decides whether or not to link against allocator. I collapsed into a
single condition below.

https://codereview.chromium.org/14321006/diff/69001/third_party/tcmalloc/chro...
File third_party/tcmalloc/chromium/src/libc_override_gcc_and_weak.h (right):

https://codereview.chromium.org/14321006/diff/69001/third_party/tcmalloc/chro...
third_party/tcmalloc/chromium/src/libc_override_gcc_and_weak.h:57: // Android's
bionic doesn't have std::bad_alloc
On 2013/05/09 16:29:29, Dai Mikurube wrote:
> nit: a period at last.

Done.

[jochen (gone - plz use gerrit), 2013-05-09 18:14:12]

content lgtm

[Dai Mikurube (NOT FULLTIME), 2013-05-15 15:38:29]

common.gypi might need rebasing. :)

[Jeffrey Yasskin, 2013-05-21 17:09:43]

https://codereview.chromium.org/14321006/diff/87001/third_party/tcmalloc/chro...
File third_party/tcmalloc/chromium/src/maybe_threads.cc (right):

https://codereview.chromium.org/14321006/diff/87001/third_party/tcmalloc/chro...
third_party/tcmalloc/chromium/src/maybe_threads.cc:128: // Android's
pthread_once_t is typedef volatile and memcmp only takes
Are there any situations where Android is missing pthread_once? That is, can you
replace this branch with a CHECK on Android?

https://codereview.chromium.org/14321006/diff/87001/third_party/tcmalloc/chro...
third_party/tcmalloc/chromium/src/maybe_threads.cc:130: if
(memcmp(const_cast<const void*>(reinterpret_cast<const volatile void*>(
http://goto.google.com/c++03pdf [dcl.type.cv]p7 says, "If an attempt is made to
refer to an object defined with a volatile-qualified type through the use of an
lvalue with a non-volatile-qualified type, the program behaviour is undefined.",
so you should write the char-comparing loop instead of using memcpy if you
really need to compare by bytes.

In future changes, you shouldn't use reinterpret_cast to go to void*. Either use
a c-style cast directly to (const void*) or implicit-cast to const volatile
void* ("const volatile void* void_ctl = ctl;"), and then const_cast to const
void*. If you really like the 1-line option, and tcmalloc doesn't define the
implicit_cast<> template
(https://code.google.com/p/chromium/codesearch/#chromium/src/base/basictypes.h...),
use a static_cast.

[Dai Mikurube (NOT FULLTIME), 2013-05-21 17:21:46]

https://codereview.chromium.org/14321006/diff/87001/third_party/tcmalloc/chro...
File third_party/tcmalloc/chromium/src/maybe_threads.cc (right):

https://codereview.chromium.org/14321006/diff/87001/third_party/tcmalloc/chro...
third_party/tcmalloc/chromium/src/maybe_threads.cc:130: if
(memcmp(const_cast<const void*>(reinterpret_cast<const volatile void*>(
On 2013/05/21 17:09:43, Jeffrey Yasskin wrote:
> http://goto.google.com/c++03pdf [dcl.type.cv]p7 says, "If an attempt is made
to
> refer to an object defined with a volatile-qualified type through the use of
an
> lvalue with a non-volatile-qualified type, the program behaviour is
undefined.",
> so you should write the char-comparing loop instead of using memcpy if you
> really need to compare by bytes.
> 
> In future changes, you shouldn't use reinterpret_cast to go to void*. Either
use
> a c-style cast directly to (const void*) or implicit-cast to const volatile
> void* ("const volatile void* void_ctl = ctl;"), and then const_cast to const
> void*. If you really like the 1-line option, and tcmalloc doesn't define the
> implicit_cast<> template
>
(https://code.google.com/p/chromium/codesearch/#chromium/src/base/basictypes.h...),
> use a static_cast.

A comment from a side:

It's just a local change to the third party library gperftools (tcmalloc).  I
believe that we should follow the original's code convention in such a case and
we shouldn't have large changes from the original code in Chromium.

We may upstream this change to the original.  We may want to discuss it at that
time.

[Dai Mikurube (NOT FULLTIME), 2013-05-21 17:26:12]

https://codereview.chromium.org/14321006/diff/87001/third_party/tcmalloc/chro...
File third_party/tcmalloc/chromium/src/maybe_threads.cc (right):

https://codereview.chromium.org/14321006/diff/87001/third_party/tcmalloc/chro...
third_party/tcmalloc/chromium/src/maybe_threads.cc:130: if
(memcmp(const_cast<const void*>(reinterpret_cast<const volatile void*>(
On 2013/05/21 17:21:46, Dai Mikurube wrote:
> On 2013/05/21 17:09:43, Jeffrey Yasskin wrote:
> > http://goto.google.com/c++03pdf [dcl.type.cv]p7 says, "If an attempt is made
> to
> > refer to an object defined with a volatile-qualified type through the use of
> an
> > lvalue with a non-volatile-qualified type, the program behaviour is
> undefined.",
> > so you should write the char-comparing loop instead of using memcpy if you
> > really need to compare by bytes.
> > 
> > In future changes, you shouldn't use reinterpret_cast to go to void*. Either
> use
> > a c-style cast directly to (const void*) or implicit-cast to const volatile
> > void* ("const volatile void* void_ctl = ctl;"), and then const_cast to const
> > void*. If you really like the 1-line option, and tcmalloc doesn't define the
> > implicit_cast<> template
> >
>
(https://code.google.com/p/chromium/codesearch/#chromium/src/base/basictypes.h...),
> > use a static_cast.
> 
> A comment from a side:
> 
> It's just a local change to the third party library gperftools (tcmalloc).  I
> believe that we should follow the original's code convention in such a case
and
> we shouldn't have large changes from the original code in Chromium.
> 
> We may upstream this change to the original.  We may want to discuss it at
that
> time.

Hmm, sorry.  I might make a misunderstanding of your comments.  Please ignore
it.

[willchan no longer on Chromium, 2013-05-21 18:06:28]

I asked Jeffrey for his help here since he understands that way better than I do
:) Everything else LGTM (I'm deferring to digit@ for the bionic specific stuff
with stack unwinding, and since he's already reviewed this, it sounds good to
me).

So yeah, it all looks good except for Jeffrey's comment. Feel free to commit
when he's happy with your change.

Jeffrey, thanks for the help.

[digit1, 2013-05-21 18:16:24]

https://codereview.chromium.org/14321006/diff/87001/third_party/tcmalloc/chro...
File third_party/tcmalloc/chromium/src/maybe_threads.cc (right):

https://codereview.chromium.org/14321006/diff/87001/third_party/tcmalloc/chro...
third_party/tcmalloc/chromium/src/maybe_threads.cc:128: // Android's
pthread_once_t is typedef volatile and memcmp only takes
No, pthread_once() and pthread_once_t are definitely provided by the Android C
library when targetting Gingerbread (2.3) or higher. I'm not sure why this block
is needed. Without memory barriers, it won't work properly anyway on multi-core
devices.

[bulach, 2013-05-22 07:41:41]

thanks everyone! I think I removed the contentious issue, ptal.

https://codereview.chromium.org/14321006/diff/87001/third_party/tcmalloc/chro...
File third_party/tcmalloc/chromium/src/maybe_threads.cc (right):

https://codereview.chromium.org/14321006/diff/87001/third_party/tcmalloc/chro...
third_party/tcmalloc/chromium/src/maybe_threads.cc:128: // Android's
pthread_once_t is typedef volatile and memcmp only takes
On 2013/05/21 18:16:24, digit1 wrote:
> No, pthread_once() and pthread_once_t are definitely provided by the Android C
> library when targetting Gingerbread (2.3) or higher. I'm not sure why this
block
> is needed. Without memory barriers, it won't work properly anyway on
multi-core
> devices.

thanks both! so it seems pthread_once is guaranteed to exist, the "else" branch
and that crazy cast is not necessary on android. removed it.

https://codereview.chromium.org/14321006/diff/87001/third_party/tcmalloc/chro...
third_party/tcmalloc/chromium/src/maybe_threads.cc:130: if
(memcmp(const_cast<const void*>(reinterpret_cast<const volatile void*>(
On 2013/05/21 17:26:12, Dai Mikurube wrote:
> On 2013/05/21 17:21:46, Dai Mikurube wrote:
> > On 2013/05/21 17:09:43, Jeffrey Yasskin wrote:
> > > http://goto.google.com/c++03pdf [dcl.type.cv]p7 says, "If an attempt is
made
> > to
> > > refer to an object defined with a volatile-qualified type through the use
of
> > an
> > > lvalue with a non-volatile-qualified type, the program behaviour is
> > undefined.",
> > > so you should write the char-comparing loop instead of using memcpy if you
> > > really need to compare by bytes.
> > > 
> > > In future changes, you shouldn't use reinterpret_cast to go to void*.
Either
> > use
> > > a c-style cast directly to (const void*) or implicit-cast to const
volatile
> > > void* ("const volatile void* void_ctl = ctl;"), and then const_cast to
const
> > > void*. If you really like the 1-line option, and tcmalloc doesn't define
the
> > > implicit_cast<> template
> > >
> >
>
(https://code.google.com/p/chromium/codesearch/#chromium/src/base/basictypes.h...),
> > > use a static_cast.
> > 
> > A comment from a side:
> > 
> > It's just a local change to the third party library gperftools (tcmalloc). 
I
> > believe that we should follow the original's code convention in such a case
> and
> > we shouldn't have large changes from the original code in Chromium.
> > 
> > We may upstream this change to the original.  We may want to discuss it at
> that
> > time.
> 
> Hmm, sorry.  I might make a misunderstanding of your comments.  Please ignore
> it.

thanks jeffrey! yeah, one of the previous patches here used the c-style cast,
but reviewers preferred to stick c++ style...
thankfully this is all removed anyways. :)

[Jeffrey Yasskin, 2013-05-22 07:55:30]

maybe_threads.cc lgtm

[Dai Mikurube (NOT FULLTIME), 2013-05-22 08:16:50]

Still lgtm.  Thanks!

[digit1, 2013-05-22 09:30:49]

lgtm, thanks :)

[bulach, 2013-05-22 09:31:41]

thanks everyone! :) CQing..

[commit-bot: I haz the power, 2013-05-22 09:33:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bulach@chromium.org/14321006/99001

[commit-bot: I haz the power, 2013-05-22 15:17:17]

Change committed as 201524