Enable --verify-heap in release mode

Enable --verify-heap in release mode

R=mstarzinger@chromium.org
BUG=v8:2120

[mvstanton1, 2012-10-11 11:00:13]

please review, thx!

[mvstanton1, 2012-10-11 12:16:25]

Hi Michael, now the patch is ready post-rebase.

[mvstanton1, 2012-10-11 12:22:44]

added comments.

http://codereview.chromium.org/11085070/diff/3001/src/heap.cc
File src/heap.cc (right):

http://codereview.chromium.org/11085070/diff/3001/src/heap.cc#newcode339
src/heap.cc:339: new_space_.bottom());
oops, should not have checked in this debugging code.

http://codereview.chromium.org/11085070/diff/3001/src/heap.cc#newcode4710
src/heap.cc:4710: MaybeObject* retptr;
I think this was debugging, I will revert this block.

http://codereview.chromium.org/11085070/diff/3001/src/mark-compact.cc
File src/mark-compact.cc (right):

http://codereview.chromium.org/11085070/diff/3001/src/mark-compact.cc#newcode81
src/mark-compact.cc:81: /*
debugging...will revert.

http://codereview.chromium.org/11085070/diff/3001/src/mark-compact.cc#newcode110
src/mark-compact.cc:110: printf("The parent object is at %p\n",(void *)
current);
same here

http://codereview.chromium.org/11085070/diff/3001/src/mark-compact.cc#newcode129
src/mark-compact.cc:129: // printf("VerifyMarking of NewSpace\n");
and here!

[danno, 2012-10-11 12:24:59]

DBC: We should keep a build flag (VERIFY_HEAP_CODE?) to conditionally add-in the
verification code, but make it something other than DEBUG. Then, add a gyp flag
for the ASAN folks to keep the code in the release build.  DEBUG should imply
VERIFY_HEAP_CODE, and VERIFY_HEAP_CODE should be controllable via a gyp flag. In
the production release, VERIFY_HEAP_CODE can be null to strip unnecessary code.

[Michael Starzinger, 2012-10-11 12:42:46]

First round of comments and nits.

https://codereview.chromium.org/11085070/diff/3001/src/flag-definitions.h
File src/flag-definitions.h (right):

https://codereview.chromium.org/11085070/diff/3001/src/flag-definitions.h#new...
src/flag-definitions.h:572: 
Also drop the empty line.

https://codereview.chromium.org/11085070/diff/3001/src/heap.cc
File src/heap.cc (right):

https://codereview.chromium.org/11085070/diff/3001/src/heap.cc#newcode468
src/heap.cc:468: allow_allocation(true);
Neither the zapping nor the verification should do any allocation, so we can
move this down right before the printing calls I think.

https://codereview.chromium.org/11085070/diff/3001/src/heap.h
File src/heap.h (right):

https://codereview.chromium.org/11085070/diff/3001/src/heap.h#newcode1279
src/heap.h:1279: #endif
Empty newline after the #endif

https://codereview.chromium.org/11085070/diff/3001/src/objects-inl.h
File src/objects-inl.h (right):

https://codereview.chromium.org/11085070/diff/3001/src/objects-inl.h#newcode3647
src/objects-inl.h:3647: #ifndef DEBUG
I think it would be useful to have a static inline function
Heap::ShouldZapGarbage() that captures this predicate so to not spread it out
across the entire code base.

https://codereview.chromium.org/11085070/diff/3001/src/objects-inl.h#newcode3789
src/objects-inl.h:3789: #ifndef DEBUG
Likewise.

https://codereview.chromium.org/11085070/diff/3001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/11085070/diff/3001/src/objects.cc#newcode2188
src/objects.cc:2188: if (trim_mode != FROM_GC || zapInFromGCMode) {
See comment about Heap::ShouldZapGarbage earlier.

https://codereview.chromium.org/11085070/diff/3001/src/objects.cc#newcode9065
src/objects.cc:9065: 
Additional empty newline here.

https://codereview.chromium.org/11085070/diff/3001/src/spaces.h
File src/spaces.h (right):

https://codereview.chromium.org/11085070/diff/3001/src/spaces.h#newcode2553
src/spaces.h:2553: virtual void Verify();
Empty newline after declaration.

[mvstanton1, 2012-10-11 19:36:44]

On 2012/10/11 12:24:59, danno wrote:
> DBC: We should keep a build flag (VERIFY_HEAP_CODE?) to conditionally add-in
the
> verification code, but make it something other than DEBUG. Then, add a gyp
flag
> for the ASAN folks to keep the code in the release build.  DEBUG should imply
> VERIFY_HEAP_CODE, and VERIFY_HEAP_CODE should be controllable via a gyp flag.
In
> the production release, VERIFY_HEAP_CODE can be null to strip unnecessary
code.

Good idea, doing this now. As an aside this makes it easier to bring in full
deep object verification without worrying too much about bloating default
release builds.

[mvstanton1, 2012-10-12 08:40:50]

Hi guys, updated to 1) make it behind a build flag, 2) include deep object
verification, 3) respond to various comments.

http://codereview.chromium.org/11085070/diff/3001/src/flag-definitions.h
File src/flag-definitions.h (right):

http://codereview.chromium.org/11085070/diff/3001/src/flag-definitions.h#newc...
src/flag-definitions.h:572: 
On 2012/10/11 12:42:46, Michael Starzinger wrote:
> Also drop the empty line.

Done.

http://codereview.chromium.org/11085070/diff/3001/src/heap.cc
File src/heap.cc (right):

http://codereview.chromium.org/11085070/diff/3001/src/heap.cc#newcode468
src/heap.cc:468: allow_allocation(true);
On 2012/10/11 12:42:46, Michael Starzinger wrote:
> Neither the zapping nor the verification should do any allocation, so we can
> move this down right before the printing calls I think.

Done.

http://codereview.chromium.org/11085070/diff/3001/src/heap.h
File src/heap.h (right):

http://codereview.chromium.org/11085070/diff/3001/src/heap.h#newcode1279
src/heap.h:1279: #endif
On 2012/10/11 12:42:46, Michael Starzinger wrote:
> Empty newline after the #endif

Done.

http://codereview.chromium.org/11085070/diff/3001/src/objects-inl.h
File src/objects-inl.h (right):

http://codereview.chromium.org/11085070/diff/3001/src/objects-inl.h#newcode3647
src/objects-inl.h:3647: #ifndef DEBUG
On 2012/10/11 12:42:46, Michael Starzinger wrote:
> I think it would be useful to have a static inline function
> Heap::ShouldZapGarbage() that captures this predicate so to not spread it out
> across the entire code base.

Done.

http://codereview.chromium.org/11085070/diff/3001/src/objects-inl.h#newcode3789
src/objects-inl.h:3789: #ifndef DEBUG
On 2012/10/11 12:42:46, Michael Starzinger wrote:
> Likewise.

Done.

http://codereview.chromium.org/11085070/diff/3001/src/objects.cc
File src/objects.cc (right):

http://codereview.chromium.org/11085070/diff/3001/src/objects.cc#newcode2188
src/objects.cc:2188: if (trim_mode != FROM_GC || zapInFromGCMode) {
On 2012/10/11 12:42:46, Michael Starzinger wrote:
> See comment about Heap::ShouldZapGarbage earlier.

Done.

http://codereview.chromium.org/11085070/diff/3001/src/objects.cc#newcode9065
src/objects.cc:9065: 
On 2012/10/11 12:42:46, Michael Starzinger wrote:
> Additional empty newline here.

Done.

http://codereview.chromium.org/11085070/diff/3001/src/spaces.h
File src/spaces.h (right):

http://codereview.chromium.org/11085070/diff/3001/src/spaces.h#newcode2553
src/spaces.h:2553: virtual void Verify();
On 2012/10/11 12:42:46, Michael Starzinger wrote:
> Empty newline after declaration.

Done.

[Michael Starzinger, 2012-10-12 10:53:15]

LGTM (with a bunch of nits).

http://codereview.chromium.org/11085070/diff/17001/src/heap.cc
File src/heap.cc (right):

http://codereview.chromium.org/11085070/diff/17001/src/heap.cc#newcode382
src/heap.cc:382: 
Two empty newlines between function implementations.

http://codereview.chromium.org/11085070/diff/17001/src/heap.cc#newcode658
src/heap.cc:658: 
Two empty newlines between function implementations.

http://codereview.chromium.org/11085070/diff/17001/src/heap.cc#newcode4705
src/heap.cc:4705: ? new_space_.AllocateRaw(size)
Indentation is off.

http://codereview.chromium.org/11085070/diff/17001/src/heap.h
File src/heap.h (right):

http://codereview.chromium.org/11085070/diff/17001/src/heap.h#newcode1272
src/heap.h:1272: 
Drop one of the two empty newlines.

http://codereview.chromium.org/11085070/diff/17001/src/objects-debug.cc
File src/objects-debug.cc (right):

http://codereview.chromium.org/11085070/diff/17001/src/objects-debug.cc#newco...
src/objects-debug.cc:785: 
Two empty newlines between function implementations.

http://codereview.chromium.org/11085070/diff/17001/src/objects-debug.cc#newco...
src/objects-debug.cc:825: 
Two empty newlines between function implementations.

http://codereview.chromium.org/11085070/diff/17001/src/objects-inl.h
File src/objects-inl.h (right):

http://codereview.chromium.org/11085070/diff/17001/src/objects-inl.h#newcode3632
src/objects-inl.h:3632: if (Heap::ShouldZapGarbage()) {
Let's also use the following here, it's equivalent and more concise:

if (Heap::ShouldZapGarbage() && HasTransitionArray()) {
  ZapTransitions();
}

http://codereview.chromium.org/11085070/diff/17001/src/objects.h
File src/objects.h (right):

http://codereview.chromium.org/11085070/diff/17001/src/objects.h#newcode1250
src/objects.h:1250: 
Dropping these two newlines would be consistent with the rest of the file.

http://codereview.chromium.org/11085070/diff/17001/src/objects.h#newcode2452
src/objects.h:2452: 
Dropping this newline would be consistent with the rest of the file.

http://codereview.chromium.org/11085070/diff/17001/src/objects.h#newcode8813
src/objects.h:8813: #undef DECL_ACCESSORS
We should also undef the DECLARE_VERIFIER macro here.

http://codereview.chromium.org/11085070/diff/17001/src/spaces.cc
File src/spaces.cc (right):

http://codereview.chromium.org/11085070/diff/17001/src/spaces.cc#newcode2558
src/spaces.cc:2558: // MVSTANTON: this is weird...the compiler can't make a
vtable unless there is
If you want to leave that comment in, format it like follows:

TODO(mvstanton): This is weird ...

http://codereview.chromium.org/11085070/diff/17001/src/spaces.cc#newcode2569
src/spaces.cc:2569: // MVSTANTON: same as above
Likewise.

http://codereview.chromium.org/11085070/diff/17001/src/spaces.cc#newcode2788
src/spaces.cc:2788: 
Two empty newlines between function implementations.

http://codereview.chromium.org/11085070/diff/17001/src/spaces.cc#newcode2838
src/spaces.cc:2838: 
Two empty newlines between function implementations.

[mvstanton1, 2012-10-12 11:16:27]

addressed comments.

http://codereview.chromium.org/11085070/diff/17001/src/heap.cc
File src/heap.cc (right):

http://codereview.chromium.org/11085070/diff/17001/src/heap.cc#newcode382
src/heap.cc:382: 
On 2012/10/12 10:53:16, Michael Starzinger wrote:
> Two empty newlines between function implementations.

Done.

http://codereview.chromium.org/11085070/diff/17001/src/heap.cc#newcode658
src/heap.cc:658: 
On 2012/10/12 10:53:16, Michael Starzinger wrote:
> Two empty newlines between function implementations.

Done.

http://codereview.chromium.org/11085070/diff/17001/src/heap.cc#newcode4705
src/heap.cc:4705: ? new_space_.AllocateRaw(size)
On 2012/10/12 10:53:16, Michael Starzinger wrote:
> Indentation is off.

Done.

http://codereview.chromium.org/11085070/diff/17001/src/objects-inl.h
File src/objects-inl.h (right):

http://codereview.chromium.org/11085070/diff/17001/src/objects-inl.h#newcode3632
src/objects-inl.h:3632: if (Heap::ShouldZapGarbage()) {
On 2012/10/12 10:53:16, Michael Starzinger wrote:
> Let's also use the following here, it's equivalent and more concise:
> 
> if (Heap::ShouldZapGarbage() && HasTransitionArray()) {
>   ZapTransitions();
> }

Done.

http://codereview.chromium.org/11085070/diff/17001/src/objects.h
File src/objects.h (right):

http://codereview.chromium.org/11085070/diff/17001/src/objects.h#newcode1250
src/objects.h:1250: 
On 2012/10/12 10:53:16, Michael Starzinger wrote:
> Dropping these two newlines would be consistent with the rest of the file.

Done.

http://codereview.chromium.org/11085070/diff/17001/src/objects.h#newcode8813
src/objects.h:8813: #undef DECL_ACCESSORS
On 2012/10/12 10:53:16, Michael Starzinger wrote:
> We should also undef the DECLARE_VERIFIER macro here.

Done.

http://codereview.chromium.org/11085070/diff/17001/src/spaces.cc
File src/spaces.cc (right):

http://codereview.chromium.org/11085070/diff/17001/src/spaces.cc#newcode2558
src/spaces.cc:2558: // MVSTANTON: this is weird...the compiler can't make a
vtable unless there is
On 2012/10/12 10:53:16, Michael Starzinger wrote:
> If you want to leave that comment in, format it like follows:
> 
> TODO(mvstanton): This is weird ...

Done.

http://codereview.chromium.org/11085070/diff/17001/src/spaces.cc#newcode2569
src/spaces.cc:2569: // MVSTANTON: same as above
On 2012/10/12 10:53:16, Michael Starzinger wrote:
> Likewise.

Done.

http://codereview.chromium.org/11085070/diff/17001/src/spaces.cc#newcode2788
src/spaces.cc:2788: 
On 2012/10/12 10:53:16, Michael Starzinger wrote:
> Two empty newlines between function implementations.

Done.

http://codereview.chromium.org/11085070/diff/17001/src/spaces.cc#newcode2838
src/spaces.cc:2838: 
On 2012/10/12 10:53:16, Michael Starzinger wrote:
> Two empty newlines between function implementations.

Done.

[Michael Starzinger, 2012-10-12 11:58:16]

Landed in: https://codereview.chromium.org/11118018