Issue 2120 in v8: Enable --verify-heap in release mode

[codesite...@google.com]

Status: Assigned
Owner: veg...@chromium.org
Labels: Type-Bug Priority-Medium

New issue 2120 by veg...@chromium.org: Enable --verify-heap in release mode
http://code.google.com/p/v8/issues/detail?id=2120

Enable faster version of --verify-heap in release mode.

[codesite...@google.com]

Updates:
Cc: jsc...@chromium.org infe...@chromium.org

Comment #1 on issue 2120 by infe...@chromium.org: Enable --verify-heap in
release mode
http://code.google.com/p/v8/issues/detail?id=2120

Thanks! Looking forward to this awesomeness!

[codesite...@google.com]

Updates:
Owner: mstar...@chromium.org

Comment #2 on issue 2120 by da...@chromium.org: Enable --verify-heap in
release mode
http://code.google.com/p/v8/issues/detail?id=2120

(No comment was entered for this change.)

[codesite...@google.com]

Updates:
Owner: mvstan...@chromium.org
Cc: mstar...@chromium.org

Comment #3 on issue 2120 by mstar...@chromium.org: Enable --verify-heap

[codesite...@google.com]

Updates:
Status: Fixed

Comment #4 on issue 2120 by mvstan...@chromium.org: Enable --verify-heap in
release mode
http://code.google.com/p/v8/issues/detail?id=2120

Verifyheap can now run in release mode but it's behind a build flag:
GYP: v8_enable_verify_heap (default 0) When 1, it defines:

VERIFY_HEAP

in the C++ build.

For debug configuration it is always on (as before). You'll need to build
release with v8_enable_verify_heap on to have access to the flag in a
release build.

[codesite...@google.com]

Comment #5 on issue 2120 by aa...@google.com: Enable --verify-heap in
release mode
http://code.google.com/p/v8/issues/detail?id=2120

Please take a look at https://chromiumcodereview.appspot.com/11106021/.
wanna make sure i did it right. So, we won't need any command line flags to
pass to chrome right ?

[codesite...@google.com]

Updates:
Cc: k...@chromium.org gli...@chromium.org

Comment #6 on issue 2120 by infe...@chromium.org: Enable --verify-heap in
release mode
http://code.google.com/p/v8/issues/detail?id=2120

Do you have any reproducible testcase (any open v8 bug) that i can try this
against to see how the crash stack looks like and make sure that
ClusterFuzz is parsing it right. I am enabling this on an asanified build,
i don't know if asan will conflict with the verify heap functionality.

[codesite...@google.com]

Comment #7 on issue 2120 by mvstan...@chromium.org: Enable --verify-heap in
release mode
http://code.google.com/p/v8/issues/detail?id=2120

Hi, I didn't find a good open bug, but I made a little cctest that shows
one of the call stacks that results from a verification. There are actually
many different checks done at different times, but many of them will be
from under:

v8::internal::Heap::Verify()

Here is the test you could add to test-heap.cc (don't check it in):

TEST(VerifyHeapStompMapPointer) {
// Ensure that we get a compacting collection so that objects are promoted
// from new space.
FLAG_gc_global = true;
FLAG_always_compact = true;
#ifdef VERIFY_HEAP
FLAG_verify_heap = true;
#endif

HEAP->ConfigureHeap(2*256*KB, 8*MB, 8*MB);

InitializeVM();

v8::HandleScope scope;

// Allocate an object
int array_size =
(Page::kMaxNonCodeHeapObjectSize - FixedArray::kHeaderSize) /
(kPointerSize * 4);
Object* obj = HEAP->AllocateFixedArray(array_size)->ToObjectChecked();

Handle<FixedArray> array(FixedArray::cast(obj));
Address addr = reinterpret_cast<Address>(*array);

// Lets damage the object
int *beefLocation = reinterpret_cast<int *>(addr);
*beefLocation = 0xdeadbeef;

// Call the m-c collector, allow heap verification to run
HEAP->CollectGarbage(OLD_POINTER_SPACE);

// We should crash with a call stack that indicates heap verification
// caught the bad map
}

Then, run under the debugger like:

$ gdb --args ./cctest test-heap/VerifyHeapStompMapPointer

Here was the call stack:

(gdb) bt
#0 v8::internal::HeapObject::map_word () at :1191
#1 v8::internal::Object::IsHeapObject () at :1139
#2 0x0057d542 in v8::internal::VerifyPointersVisitor::VisitPointers
(this=0xbffff728, start=<value temporarily unavailable, due to
optimizations>) at objects-inl.h:751
#3 0x001c8cf2 in v8::internal::HandleScopeImplementer::IterateThis
(this=0x1559e90, v=0xbffff728) at ../src/api.cc:6512
#4 0x002c2595 in v8::internal::Heap::IterateStrongRoots (this=0x1abec08,
v=0xbffff728, mode=<value temporarily unavailable, due to optimizations>)
at ../src/heap.cc:5785
#5 0x002ad82a in v8::internal::Heap::IterateRoots () at :5743
#6 0x002ad82a in v8::internal::Heap::Verify (this=0x1abec08)
at ../src/heap.cc:751
#7 0x002ad3d6 in v8::internal::Heap::GarbageCollectionPrologue
(this=0x1abec08) at ../src/heap.cc:411
#8 0x002af540 in v8::internal::Heap::CollectGarbage (this=0x1abec08,
space=v8::internal::OLD_POINTER_SPACE,
collector=v8::internal::MARK_COMPACTOR, collector_reason=0xa8fc6e "GC in
old space requested") at ../src/heap.cc:620
#9 0x00107272 in v8::internal::Isolate::Current () at :440
#10 0x00107272 in TestVerifyHeapStompMapPointer () at heap-inl.h:751
#11 0x0000256d in main (argc=2, argv=<value temporarily unavailable, due to
optimizations>) at ../test/cctest/cctest.cc:96

Maybe in future we could add a set of tests to verify the different kinds
of things we check for. I hope that helps!

[codesite...@google.com]

Comment #8 on issue 2120 by infe...@chromium.org: Enable --verify-heap in
release mode
http://code.google.com/p/v8/issues/detail?id=2120

Thanks Michael. We have enabled it on ClusterFuzz and i will see if I can
directly test it on Chrome since that is what the fuzzed testcases are
running on.

[codesite...@google.com]

Updates:
Cc: yan...@chromium.org verwa...@chromium.org da...@chromium.org
stefa...@chromium.org

Comment #9 on issue 2120 by infe...@chromium.org: Enable --verify-heap in
release mode
http://code.google.com/p/v8/issues/detail?id=2120

Michael (mvstanton@), can you please try the repro in
https://code.google.com/p/chromium/issues/detail?id=160010 and see why
v8_enable_verify_heap in release is not catching it.

[codesite...@google.com]

Comment #10 on issue 2120 by mvstan...@chromium.org: Enable --verify-heap

in release mode
http://code.google.com/p/v8/issues/detail?id=2120

I ran the repro for 160010 and from my experience and the comments it seems
to crash every time in
v8::internal::BasicJsonStringifier::SerializeString_(). As this method
doesn't allocate in the crashing loop (writing into a string in the heap),
verify_heap doesn't have the chance to intervene before it writes off the
end of NewSpace (new gc heap objects).

So catching this bug would need some kind of red zone support within
objects or at least knowledge about our heap segments to recognize when we
write outside of them. It's beyond the current capabilities of verify-heap.