Bug 138851 – Add Heap verification infrastructure

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Mark Lam]

This is debugging code I wrote for a prior debugging session.  Just cleaning it up for landing so that we can reuse / build on it in the future as needed.

The code is always build in but disabled by default.  When disabled, the cost is minimal:
1. Heap has a m_verifier field.
2. GC does a few "if (m_verifier)" checks that should fail.
3. HeapVerifier takes up code space though not used.

When enabled:
1. The HeapVerifier will keep N number of GC pass data.
   Each GC pass will contain a "before marking" and "after marking" live object list.
   The GC passes is a circular buffer.  Only data for the last N GC passes will be retained.
2. During GC, the current GC pass' live objects lists will be populated before and after marking.
3. The current GC pass' live object lists will be validated before GC, after marking, and after GC.

Currently, the only validation being done is to verify that object butterflies are allocated from valid blocks in the Storage (aka Copied) space.

[Mark Lam]

Created attachment 241826 [details]
the patch

[Mark Hahnenberg]

Comment on attachment 241826 [details]
the patch

View in context: https://bugs.webkit.org/attachment.cgi?id=241826&action=review

Did this find a bug?

> Source/JavaScriptCore/heap/Heap.cpp:1015
> +    if (m_verifier) {
> +        m_verifier->initializeGCPass();
> +        m_verifier->gatherLiveObjects(HeapVerifier::Phase::BeforeMarking);
> +    }

Sort of a nit, but this is done after the start of GC, but the AfterGC portion of the verification occurs after the final call to currentTimeMS. Might be more consistent to do both during the "official" collection.

> Source/JavaScriptCore/heap/HeapVerifier.cpp:197
> +    // dataLogF("Verifying @ phase %s, object list '%s' (size %zu)\n", phaseName(phase), list.name, liveObjects.size());

Remove or uncomment.

> Source/JavaScriptCore/runtime/Options.h:291
> +    v(unsigned, numberOfGCPassesToRecordForVerification, 3) \

"Pass" is sort of an odd term for this. I'm not sure I know the difference between a "Pass" and a "Phase". Not sure what else to call it though :-/ Maybe..."Cycle"?

[Mark Lam]

(In reply to comment #2)
> Comment on attachment 241826 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=241826&action=review
> 
> Did this find a bug?

Yes.  It found a tricky write barrier bug.

> > Source/JavaScriptCore/heap/Heap.cpp:1015
> > +    if (m_verifier) {
> > +        m_verifier->initializeGCPass();
> > +        m_verifier->gatherLiveObjects(HeapVerifier::Phase::BeforeMarking);
> > +    }
> 
> Sort of a nit, but this is done after the start of GC, but the AfterGC
> portion of the verification occurs after the final call to currentTimeMS.
> Might be more consistent to do both during the "official" collection.

I've moved the BeforeGC verification above this into this block and similarly moved the AfterGC verification before we sample the GC end time.

> > Source/JavaScriptCore/heap/HeapVerifier.cpp:197
> > +    // dataLogF("Verifying @ phase %s, object list '%s' (size %zu)\n", phaseName(phase), list.name, liveObjects.size());
> 
> Remove or uncomment.

Thought this might be useful but may be verbose if printed all time.  Hence, I left it there commented out for convenience, but have removed it now since it's not hard to add it back when I want it.

> > Source/JavaScriptCore/runtime/Options.h:291
> > +    v(unsigned, numberOfGCPassesToRecordForVerification, 3) \
> 
> "Pass" is sort of an odd term for this. I'm not sure I know the difference
> between a "Pass" and a "Phase". Not sure what else to call it though :-/
> Maybe..."Cycle"?

I think GCPass is easier to read then GCCycle (both mean the same thing), but we already use the term cycle elsewhere in the GC code.  I renamed Pass to Cycle.

[Mark Lam]

Created attachment 241837 [details]
revised patch after Mark H's feedback

[Mark Lam]

Created attachment 241841 [details]
take 3: with some needed #include of inline files.

[Mark Lam]

Created attachment 241842 [details]
take 4: plus missing build file changes for other platforms.

[Mark Lam]

The Win EWS is barfing because:

     1>..\heap\HeapVerifier.cpp(227): fatal error C1001: An internal error has occurred in the compiler.
         (compiler file 'msc1.cpp', line 1325)
          To work around this problem, try simplifying or changing the program near the locations listed above.
         Please choose the Technical Support command on the Visual C++ 
          Help menu, or open the Technical Support help file for more information

In other words, VC++ fail.  Will upload some speculative patches to see if I can work around VC++'s short coming.

[Mark Lam]

Created attachment 241843 [details]
take 5: speculative fix for VC++'s short coming. Uploading to test on Win EWS.

[Mark Lam]

Created attachment 241861 [details]
take 6: suppress compiler warnings because EFL compiler is not smart enough to detect a dead branch.

[Geoffrey Garen]

Comment on attachment 241861 [details]
take 6: suppress compiler warnings because EFL compiler is not smart enough to detect a dead branch.

r=me if it you get it to build -- looks like the Windows build is still broken.

[Mark Lam]

(In reply to comment #10)
> r=me if it you get it to build -- looks like the Windows build is still
> broken.

The Windows EWS was unable to build even without my patch. The 5th version of my patch shows that my changes do build on Windows.  Will land.

[Mark Lam]

Thanks for the review.  Landed in r176424: <http://trac.webkit.org/r176424>.