Add a chromium version abort function for tcmalloc: Abort().

Add a chromium version abort function for tcmalloc: Abort().
This is because on some platform (e.g. Windows), the way to implement abort()
is different so chrome's crash service can not detect the crash but treat as
normal exit. See http://code.google.com/p/chromium/issues/detail?id=118665 for
some detail.
In this implementation, a segment fault will be triggered and this will be
treated as crash on all platforms.

BUG=127724
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=137013

[eroman, 2012-05-11 03:55:53]

LGTM

http://codereview.chromium.org/10384117/diff/1/third_party/tcmalloc/chromium/...
File third_party/tcmalloc/chromium/src/base/abort.h (right):

http://codereview.chromium.org/10384117/diff/1/third_party/tcmalloc/chromium/...
third_party/tcmalloc/chromium/src/base/abort.h:45: inline void Abort() {
I think this should be inside of namespace base.

http://codereview.chromium.org/10384117/diff/1/third_party/tcmalloc/chromium/...
File third_party/tcmalloc/chromium/src/base/atomicops-internals-arm-generic.h
(right):

http://codereview.chromium.org/10384117/diff/1/third_party/tcmalloc/chromium/...
third_party/tcmalloc/chromium/src/base/atomicops-internals-arm-generic.h:40:
#include <stdio.h>
[optional] I wander if these headers are needed anymore.

[kaiwang, 2012-05-11 20:54:59]

http://codereview.chromium.org/10384117/diff/1/third_party/tcmalloc/chromium/...
File third_party/tcmalloc/chromium/src/base/abort.h (right):

http://codereview.chromium.org/10384117/diff/1/third_party/tcmalloc/chromium/...
third_party/tcmalloc/chromium/src/base/abort.h:45: inline void Abort() {
Actually most content in base dir do not have namespace (I think they just keep
their content from google3).
And put Abort() inside base namespace will also cause some inconvenience:
base::Abort()...

Tend to suggest for this common function we just keep it no namespace... What do
you think?

http://codereview.chromium.org/10384117/diff/1/third_party/tcmalloc/chromium/...
File third_party/tcmalloc/chromium/src/base/atomicops-internals-arm-generic.h
(right):

http://codereview.chromium.org/10384117/diff/1/third_party/tcmalloc/chromium/...
third_party/tcmalloc/chromium/src/base/atomicops-internals-arm-generic.h:40:
#include <stdio.h>
On 2012/05/11 03:55:53, eroman wrote:
> [optional] I wander if these headers are needed anymore.

Done.

[eroman, 2012-05-11 21:11:14]

http://codereview.chromium.org/10384117/diff/1/third_party/tcmalloc/chromium/...
File third_party/tcmalloc/chromium/src/base/abort.h (right):

http://codereview.chromium.org/10384117/diff/1/third_party/tcmalloc/chromium/...
third_party/tcmalloc/chromium/src/base/abort.h:45: inline void Abort() {
On 2012/05/11 20:54:59, kaiwang wrote:
> Actually most content in base dir do not have namespace (I think they just
keep
> their content from google3).
> And put Abort() inside base namespace will also cause some inconvenience:
> base::Abort()...
> 
> Tend to suggest for this common function we just keep it no namespace... What
do
> you think?

Sounds fine to me. I'll defer to Jim

[jar (doing other things), 2012-05-11 21:57:56]

I'm very psyched to see what this reveals when landed!!

A few comments below.

http://codereview.chromium.org/10384117/diff/1/third_party/tcmalloc/chromium/...
File third_party/tcmalloc/chromium/src/base/abort.h (right):

http://codereview.chromium.org/10384117/diff/1/third_party/tcmalloc/chromium/...
third_party/tcmalloc/chromium/src/base/abort.h:31: // Author: Kai Wang
<opensource@google.com>
nit: The whole copyright message for Chrome is only about 3 lines, and we use
that as a standard.

We also moved away from listing the file author, and even went so far (when we
first pushed out Chrome) to remove all the file author entries in all the Chrome
src files.  You'll still see it in third_party stuff, such as TCMalloc, but that
is only to avoid changing things on a vendor branch, and making integration
harder.

http://codereview.chromium.org/10384117/diff/1/third_party/tcmalloc/chromium/...
third_party/tcmalloc/chromium/src/base/abort.h:42: #ifdef
TCMALLOC_USE_SYSTEM_ABORT
nit: I think folks like the format:
#if defined(...)

Also, we usually don't have base depend on third party (explicitly), so it
probably shouldn't mention TCMALLOC (I think).

Can we go with something simple like USE_SYSTEM_ABORT?

Is there a stronger reason for TCMALLOC reference??

http://codereview.chromium.org/10384117/diff/1/third_party/tcmalloc/chromium/...
third_party/tcmalloc/chromium/src/base/abort.h:45: inline void Abort() {
namespaces are good... so I'd go with Eric's suggestion of putting it in base.  

On 2012/05/11 21:11:14, eroman wrote:
> On 2012/05/11 20:54:59, kaiwang wrote:
> > Actually most content in base dir do not have namespace (I think they just
> keep
> > their content from google3).
> > And put Abort() inside base namespace will also cause some inconvenience:
> > base::Abort()...
> > 
> > Tend to suggest for this common function we just keep it no namespace...
What
> do
> > you think?
> 
> Sounds fine to me. I'll defer to Jim

http://codereview.chromium.org/10384117/diff/1/third_party/tcmalloc/chromium/...
third_party/tcmalloc/chromium/src/base/abort.h:51: *((int*)0) = 0x2001;  // Make
a segmentation fault to force abort.
nit: always avoid C style casts.

*reinterpret_cast<int*>(NULL) = 0x2001;

[jar (doing other things), 2012-05-11 22:46:35]

https://chromiumcodereview.appspot.com/10384117/diff/1/third_party/tcmalloc/c...
File third_party/tcmalloc/chromium/src/base/abort.h (right):

https://chromiumcodereview.appspot.com/10384117/diff/1/third_party/tcmalloc/c...
third_party/tcmalloc/chromium/src/base/abort.h:42: #ifdef
TCMALLOC_USE_SYSTEM_ABORT
<doh>.   As per conversation, this is over in the third_party TCMalloc area... I
misread the path (looking only at the last few words).

I now agree we don't want to add it to "base," since that will needlessly
conflict with the "real" base.  I do still like namespaces, and I'm wary about
having a global with a name like "Abort()" that might interfere with other code.

What namespace would be reasonable here?

tcmalloc_base

or you suggested:

tcmalloc

Your choice ;-)

Sorry about my confusion.


On 2012/05/11 21:57:56, jar wrote:
> nit: I think folks like the format:
> #if defined(...)
> 
> Also, we usually don't have base depend on third party (explicitly), so it
> probably shouldn't mention TCMALLOC (I think).
> 
> Can we go with something simple like USE_SYSTEM_ABORT?
> 
> Is there a stronger reason for TCMALLOC reference??

[kaiwang, 2012-05-12 00:06:09]

Uploaded a new version

http://codereview.chromium.org/10384117/diff/1/third_party/tcmalloc/chromium/...
File third_party/tcmalloc/chromium/src/base/abort.h (right):

http://codereview.chromium.org/10384117/diff/1/third_party/tcmalloc/chromium/...
third_party/tcmalloc/chromium/src/base/abort.h:31: // Author: Kai Wang
<opensource@google.com>

Done.

http://codereview.chromium.org/10384117/diff/1/third_party/tcmalloc/chromium/...
third_party/tcmalloc/chromium/src/base/abort.h:42: #ifdef
TCMALLOC_USE_SYSTEM_ABORT
Done, added to tcmalloc namespace

http://codereview.chromium.org/10384117/diff/1/third_party/tcmalloc/chromium/...
third_party/tcmalloc/chromium/src/base/abort.h:51: *((int*)0) = 0x2001;  // Make
a segmentation fault to force abort.
On 2012/05/11 21:57:56, jar wrote:
> nit: always avoid C style casts.
> 
> *reinterpret_cast<int*>(NULL) = 0x2001;
> 

Done.

[jar (doing other things), 2012-05-12 14:53:50]

lgtm

[commit-bot: I haz the power, 2012-05-14 21:26:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kaiwang@chromium.org/10384117/5005

[commit-bot: I haz the power, 2012-05-14 23:43:58]

Change committed as 137013

[hans, 2012-05-15 11:38:06]

https://chromiumcodereview.appspot.com/10384117/diff/5005/third_party/tcmallo...
File third_party/tcmalloc/chromium/src/base/abort.h (right):

https://chromiumcodereview.appspot.com/10384117/diff/5005/third_party/tcmallo...
third_party/tcmalloc/chromium/src/base/abort.h:28: *reinterpret_cast<int*>(NULL)
= 0x2001;
This causes a Clang warning:

abort.h:28:3: warning: indirection of non-volatile null pointer will be deleted,
not trap [-Wnull-dereference]
  *reinterpret_cast<int*>(NULL) = 0x2001;
  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~


The write to null will be optimized away (since it's undefined behavior, the
compiler is free to do that), and hence the Abort() function will do nothing.

This is easy to fix by making the pointer volatile:

  *reinterpret_cast<volatile int*>(NULL) = 0x2001;

Would it be possible to fix this?

[kaiwang, 2012-05-15 17:56:03]

https://chromiumcodereview.appspot.com/10384117/diff/5005/third_party/tcmallo...
File third_party/tcmalloc/chromium/src/base/abort.h (right):

https://chromiumcodereview.appspot.com/10384117/diff/5005/third_party/tcmallo...
third_party/tcmalloc/chromium/src/base/abort.h:28: *reinterpret_cast<int*>(NULL)
= 0x2001;
Sure, I'll fix. Sorry for the inconvenience

[kaiwang, 2012-05-15 18:04:51]

https://chromiumcodereview.appspot.com/10384117/diff/5005/third_party/tcmallo...
File third_party/tcmalloc/chromium/src/base/abort.h (right):

https://chromiumcodereview.appspot.com/10384117/diff/5005/third_party/tcmallo...
third_party/tcmalloc/chromium/src/base/abort.h:28: *reinterpret_cast<int*>(NULL)
= 0x2001;
Just curious, I don't see this warning while compiling. So it's by default
turned off? Then is the optimizing by default on?

[hans, 2012-05-16 09:02:33]

On 2012/05/15 18:04:51, kaiwang wrote:
>
https://chromiumcodereview.appspot.com/10384117/diff/5005/third_party/tcmallo...
> File third_party/tcmalloc/chromium/src/base/abort.h (right):
> 
>
https://chromiumcodereview.appspot.com/10384117/diff/5005/third_party/tcmallo...
> third_party/tcmalloc/chromium/src/base/abort.h:28:
*reinterpret_cast<int*>(NULL)
> = 0x2001;
> Just curious, I don't see this warning while compiling. So it's by default
> turned off? Then is the optimizing by default on?

Are you using Clang? GCC doesn't have this warning.

I saw this when building Chrome on Linux with Clang as decribed on
http://code.google.com/p/chromium/wiki/Clang