Use tcmalloc's debugallocation for Debug builds

Add an option to use tcmalloc's debugallocation for Debug builds
to catch memory problems early, easily, and cleanly.

This is disabled by default.

Because of gyp limitations, the flag is to force-disable the feature, but there is no flag to force-enable it.

BUG=30715
TEST=none

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

[willchan no longer on Chromium, 2012-10-24 22:43:09]

If this doesn't break tests, then LGTM.

[Dai Mikurube (NOT FULLTIME), 2012-10-25 00:06:44]

Does this introduce debugallocation unconditionally?  And, can't we use any flag
to select debugallocation, actually?

Some of memory profiling tools (which will be working on buildbots soon) depends
on Debug builds, and I'm not sure of debugallocation's impact.  Could you
introduce a flag to switch debugallocation, or describe debugallocation's
impact?

[willchan no longer on Chromium, 2012-10-25 00:53:16]

debugallocation allocates extra bytes around each block to mark them as
unused, freed, in use, etc, so we can catch use after free and etc. It also
does checking to make sure we correctly use new[] and delete[].

So yes, for the deep memory profiler, it might screw up the numbers by a
bit. I didn't realize deep memory profiler was a debug build feature.
That's a good point. We should probably change the deep memory profiler so
this doesn't interfere.


On Wed, Oct 24, 2012 at 5:06 PM, <dmikurube@chromium.org> wrote:

> Does this introduce debugallocation unconditionally?  And, can't we use
> any flag
> to select debugallocation, actually?
>
> Some of memory profiling tools (which will be working on buildbots soon)
> depends
> on Debug builds, and I'm not sure of debugallocation's impact.  Could you
> introduce a flag to switch debugallocation, or describe debugallocation's
> impact?
>
>
https://chromiumcodereview.**appspot.com/11266019/<https://chromiumcodereview...
>

[Dai Mikurube (NOT FULLTIME), 2012-10-25 02:19:56]

It may work only with release_extra_cflags=-fno-omit-frame-pointer on Release
builds.  But, Chromium developers usually/sometimes use Debug builds also in
profiling on their local machines.

debugallocation by default in Debug builds sounds reasonable, but unconditional
debugallocation ("we can't turn off") sounds inconvenient...


On 2012/10/25 00:53:16, willchan wrote:
> debugallocation allocates extra bytes around each block to mark them as
> unused, freed, in use, etc, so we can catch use after free and etc. It also
> does checking to make sure we correctly use new[] and delete[].
> 
> So yes, for the deep memory profiler, it might screw up the numbers by a
> bit. I didn't realize deep memory profiler was a debug build feature.
> That's a good point. We should probably change the deep memory profiler so
> this doesn't interfere.
> 
> 
> On Wed, Oct 24, 2012 at 5:06 PM, <mailto:dmikurube@chromium.org> wrote:
> 
> > Does this introduce debugallocation unconditionally?  And, can't we use
> > any flag
> > to select debugallocation, actually?
> >
> > Some of memory profiling tools (which will be working on buildbots soon)
> > depends
> > on Debug builds, and I'm not sure of debugallocation's impact.  Could you
> > introduce a flag to switch debugallocation, or describe debugallocation's
> > impact?
> >
> >
>
https://chromiumcodereview.**appspot.com/11266019/%3Chttps://chromiumcoderevi...>
> >

[Paweł Hajdan Jr., 2012-10-25 17:18:00]

On 2012/10/25 02:19:56, Dai Mikurube wrote:
> It may work only with release_extra_cflags=-fno-omit-frame-pointer on Release
> builds.  But, Chromium developers usually/sometimes use Debug builds also in
> profiling on their local machines.
> 
> debugallocation by default in Debug builds sounds reasonable, but
unconditional
> debugallocation ("we can't turn off") sounds inconvenient...

Dai, don't worry, I'm not going to land this until we find solution to that.
Thanks for pointing out possible issues early.

I see two possibilities (will have to see if they work for gyp configurations):

1. Is there some gyp flag you use for profiling? I would just disable
debugallocation when profiling.

2. We could add a new flag to force disable debugallocation in Debug builds. A
bit ugly because of the asymmetry (e.g. it would not enable debugallocation for
Release builds - it's a limitation of configurations), but should work.

WDYT?

[Dai Mikurube (NOT FULLTIME), 2012-10-26 03:25:41]

On 2012/10/25 17:18:00, Paweł Hajdan Jr. wrote:
> On 2012/10/25 02:19:56, Dai Mikurube wrote:
> > It may work only with release_extra_cflags=-fno-omit-frame-pointer on
Release
> > builds.  But, Chromium developers usually/sometimes use Debug builds also in
> > profiling on their local machines.
> > 
> > debugallocation by default in Debug builds sounds reasonable, but
> unconditional
> > debugallocation ("we can't turn off") sounds inconvenient...
> 
> Dai, don't worry, I'm not going to land this until we find solution to that.
> Thanks for pointing out possible issues early.
> 
> I see two possibilities (will have to see if they work for gyp
configurations):
> 
> 1. Is there some gyp flag you use for profiling? I would just disable
> debugallocation when profiling.
> 
> 2. We could add a new flag to force disable debugallocation in Debug builds. A
> bit ugly because of the asymmetry (e.g. it would not enable debugallocation
for
> Release builds - it's a limitation of configurations), but should work.
> 
> WDYT?

Good to hear that. :)

By <2>, you mean :
* debugallocation is always disabled in Release builds.
* debugallocation is enabled by default in Debug builds.
* debugallocation can be disabled by a flag in Debug builds.
right?

I think it sounds more convenient and enough reasonable.  It's also ok if
disabled by default and enabled by a flag in Debug builds.

[Paweł Hajdan Jr., 2012-10-26 17:08:46]

PTAL

debugallocation is disabled in Release builds and for now there is no flag to
turn it on for Release. I don't think this is a big deal, at least for now.

For Debug builds, I added a new gyp flag that allows you to disable
debugallocation.

[Paweł Hajdan Jr., 2012-10-26 17:09:18]

PTAL

debugallocation is disabled in Release builds and for now there is no flag to
turn it on for Release. I don't think this is a big deal, at least for now.

For Debug builds, I added a new gyp flag that allows you to disable
debugallocation.

[Paweł Hajdan Jr., 2012-10-26 17:10:43]

PTAL

debugallocation is disabled in Release builds and for now there is no flag to
turn it on for Release. I don't think this is a big deal, at least for now.

For Debug builds, I added a new gyp flag that allows you to disable
debugallocation.

[Dai Mikurube (NOT FULLTIME), 2012-10-27 02:03:01]

On 2012/10/26 17:10:43, Paweł Hajdan Jr. wrote:
> PTAL
> 
> debugallocation is disabled in Release builds and for now there is no flag to
> turn it on for Release. I don't think this is a big deal, at least for now.
> 
> For Debug builds, I added a new gyp flag that allows you to disable
> debugallocation.

It basically looks good to me.  One question: how does it live together with
allocator_shim.cc?  It looks that both allocator_shim.cc and
debugallocation_shim.cc are including tcmalloc.cc.  (Tell me if I'm wrong. ;) )

[willchan no longer on Chromium, 2012-10-27 02:35:20]

allocator_shim.cc shouldn't do anything in posix, right? I think that's a
windows only file. I haven't checked. debugallocation_shim.cc should be
posix only.


On Fri, Oct 26, 2012 at 7:03 PM, <dmikurube@chromium.org> wrote:

> On 2012/10/26 17:10:43, Paweł Hajdan Jr. wrote:
>
>> PTAL
>>
>
>  debugallocation is disabled in Release builds and for now there is no
>> flag to
>> turn it on for Release. I don't think this is a big deal, at least for
>> now.
>>
>
>  For Debug builds, I added a new gyp flag that allows you to disable
>> debugallocation.
>>
>
> It basically looks good to me.  One question: how does it live together
> with
> allocator_shim.cc?  It looks that both allocator_shim.cc and
> debugallocation_shim.cc are including tcmalloc.cc.  (Tell me if I'm wrong.
> ;) )
>
>
http://codereview.chromium.**org/11266019/<http://codereview.chromium.org/112...
>

[Dai Mikurube (NOT FULLTIME), 2012-10-29 07:07:11]

Yes, I think allocator_shim.cc is posix-only.  But, allocator.gyp looked using
debugallocation_shim.cc unconditionally even for Windows.  (Though I'm not
sure... I'm not familiar with gyp.)

If debugallocation_shim.cc is used only in Linux, the change looks good to me.

On 2012/10/27 02:35:20, willchan wrote:
> allocator_shim.cc shouldn't do anything in posix, right? I think that's a
> windows only file. I haven't checked. debugallocation_shim.cc should be
> posix only.
> 
> 
> On Fri, Oct 26, 2012 at 7:03 PM, <mailto:dmikurube@chromium.org> wrote:
> 
> > On 2012/10/26 17:10:43, Paweł Hajdan Jr. wrote:
> >
> >> PTAL
> >>
> >
> >  debugallocation is disabled in Release builds and for now there is no
> >> flag to
> >> turn it on for Release. I don't think this is a big deal, at least for
> >> now.
> >>
> >
> >  For Debug builds, I added a new gyp flag that allows you to disable
> >> debugallocation.
> >>
> >
> > It basically looks good to me.  One question: how does it live together
> > with
> > allocator_shim.cc?  It looks that both allocator_shim.cc and
> > debugallocation_shim.cc are including tcmalloc.cc.  (Tell me if I'm wrong.
> > ;) )
> >
> >
>
http://codereview.chromium.**org/11266019/%3Chttp://codereview.chromium.org/1...>
> >

[willchan no longer on Chromium, 2012-10-29 17:29:23]

I see. While I think we could make debugallocation_shim.cc work on Windows
too (and that'd be neat!), it might be better to start with just posix. I
defer to phajdan for how he wants to execute on this.


On Mon, Oct 29, 2012 at 12:07 AM, <dmikurube@chromium.org> wrote:

> Yes, I think allocator_shim.cc is posix-only.  But, allocator.gyp looked
> using
> debugallocation_shim.cc unconditionally even for Windows.  (Though I'm not
> sure... I'm not familiar with gyp.)
>
> If debugallocation_shim.cc is used only in Linux, the change looks good to
> me.
>
>
> On 2012/10/27 02:35:20, willchan wrote:
>
>> allocator_shim.cc shouldn't do anything in posix, right? I think that's a
>> windows only file. I haven't checked. debugallocation_shim.cc should be
>> posix only.
>>
>
>
>  On Fri, Oct 26, 2012 at 7:03 PM, <mailto:dmikurube@chromium.org**> wrote:
>>
>
>  > On 2012/10/26 17:10:43, Paweł Hajdan Jr. wrote:
>> >
>> >> PTAL
>> >>
>> >
>> >  debugallocation is disabled in Release builds and for now there is no
>> >> flag to
>> >> turn it on for Release. I don't think this is a big deal, at least for
>> >> now.
>> >>
>> >
>> >  For Debug builds, I added a new gyp flag that allows you to disable
>> >> debugallocation.
>> >>
>> >
>> > It basically looks good to me.  One question: how does it live together
>> > with
>> > allocator_shim.cc?  It looks that both allocator_shim.cc and
>> > debugallocation_shim.cc are including tcmalloc.cc.  (Tell me if I'm
>> wrong.
>> > ;) )
>> >
>> >
>>
>
> http://codereview.chromium.****org/11266019/%3Chttp://coderev**
> iew.chromium.org/11266019/ <http://codereview.chromium.org/11266019/>>
>
>> >
>>
>
>
>
http://codereview.chromium.**org/11266019/<http://codereview.chromium.org/112...
>

[Dai Mikurube (NOT FULLTIME), 2012-10-30 07:46:10]

Ah, I said a wrong thing.  allocator_shim.cc is *Windows*-only, and
debugallocation_shim.cc looks used also in Windows.

Both ways sound reasonable.  I defer it to phajdan how to do it, too.


On 2012/10/29 17:29:23, willchan wrote:
> I see. While I think we could make debugallocation_shim.cc work on Windows
> too (and that'd be neat!), it might be better to start with just posix. I
> defer to phajdan for how he wants to execute on this.
> 
> 
> On Mon, Oct 29, 2012 at 12:07 AM, <mailto:dmikurube@chromium.org> wrote:
> 
> > Yes, I think allocator_shim.cc is posix-only.  But, allocator.gyp looked
> > using
> > debugallocation_shim.cc unconditionally even for Windows.  (Though I'm not
> > sure... I'm not familiar with gyp.)
> >
> > If debugallocation_shim.cc is used only in Linux, the change looks good to
> > me.
> >
> >
> > On 2012/10/27 02:35:20, willchan wrote:
> >
> >> allocator_shim.cc shouldn't do anything in posix, right? I think that's a
> >> windows only file. I haven't checked. debugallocation_shim.cc should be
> >> posix only.
> >>
> >
> >
> >  On Fri, Oct 26, 2012 at 7:03 PM, <mailto:dmikurube@chromium.org**> wrote:
> >>
> >
> >  > On 2012/10/26 17:10:43, Paweł Hajdan Jr. wrote:
> >> >
> >> >> PTAL
> >> >>
> >> >
> >> >  debugallocation is disabled in Release builds and for now there is no
> >> >> flag to
> >> >> turn it on for Release. I don't think this is a big deal, at least for
> >> >> now.
> >> >>
> >> >
> >> >  For Debug builds, I added a new gyp flag that allows you to disable
> >> >> debugallocation.
> >> >>
> >> >
> >> > It basically looks good to me.  One question: how does it live together
> >> > with
> >> > allocator_shim.cc?  It looks that both allocator_shim.cc and
> >> > debugallocation_shim.cc are including tcmalloc.cc.  (Tell me if I'm
> >> wrong.
> >> > ;) )
> >> >
> >> >
> >>
> >
> > http://codereview.chromium.****org/11266019/%253Chttp://coderev**
> > iew.chromium.org/11266019/ <http://codereview.chromium.org/11266019/>>
> >
> >> >
> >>
> >
> >
> >
>
http://codereview.chromium.**org/11266019/%3Chttp://codereview.chromium.org/1...>
> >