Enable --verify-heap in release mode (issue 11085070)

[mvst...@google.com]

Reviewers: Michael Starzinger,

Message:
please review, thx!

Description:
Enable --verify-heap in release mode

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


Please review this at http://codereview.chromium.org/11085070/

SVN Base: https://v8.googlecode.com/svn/branches/bleeding_edge

Affected files:
M src/flag-definitions.h
M src/heap-inl.h
M src/heap.h
M src/heap.cc
M src/incremental-marking.cc
M src/mark-compact.h
M src/mark-compact.cc
M src/objects-debug.cc
M src/objects-inl.h
M src/objects.h
M src/objects.cc
M src/spaces.h
M src/spaces.cc
M test/cctest/test-debug.cc
M test/cctest/test-heap.cc
M test/cctest/test-weakmaps.cc

[mvst...@google.com]

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

http://codereview.chromium.org/11085070/

[mvst...@google.com]

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!

http://codereview.chromium.org/11085070/

[da...@chromium.org]

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.

http://codereview.chromium.org/11085070/

[mstar...@chromium.org]

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#newcode572
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.

https://codereview.chromium.org/11085070/

[mvst...@google.com]

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.


http://codereview.chromium.org/11085070/

[mvst...@google.com]

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#newcode572
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.

http://codereview.chromium.org/11085070/

[mstar...@chromium.org]

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#newcode785
src/objects-debug.cc:785:
Two empty newlines between function implementations.

http://codereview.chromium.org/11085070/diff/17001/src/objects-debug.cc#newcode825
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.

http://codereview.chromium.org/11085070/

[mvst...@google.com]

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.

http://codereview.chromium.org/11085070/

[mstar...@chromium.org]

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

https://codereview.chromium.org/11085070/