Bug 143809 – Add $vm debugging tool.

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Lam]

For debugging VM bugs, it would be useful to be able to dump VM data structures from JS code that we instrument.  To this end, let's introduce a JS_enableDollarVM option that, if true, installs an $vm property into each JS global object at creation time.  The $vm property refers to an object that provides a collection of useful utility functions.  For this initial implementation, $vm will have the following:

    dfgTrue() - returns true if the current function is DFG compiled, else returns false.
    jitTrue() - returns true if the current function is compiled by the baseline JIT, else returns false.
    llintTrue() - returns true if the current function is interpreted by the LLINT, else returns false.

    gc() - runs a full GC and sweep all dead objects.
    fullGC() - runs a full GC but does not sweep dead objects. 
    edenGC() - runs an eden GC but does not sweep dead objects.

    codeBlockForFrame(frameNumber) - gets the codeBlock at the specified frame (0 = current, 1 = caller, etc).
    printSourceFor(codeBlock) - prints the source code for the codeBlock.
    printByteCodeFor(codeBlock) - prints the bytecode for the codeBlock.

    print(str) - prints a string to dataLog output.
    printCallFrame() - prints the current CallFrame.
    printStack() - prints the JS stack.
    printInternal(value) - prints the JSC internal info for the specified value.

With JS_enableDollarVM=true, JS code can use the above functions like so:

    $vm.print("Using $vm features\n");

[Mark Lam]

Created attachment 250941 [details]
the patch.

[Geoffrey Garen]

Comment on attachment 250941 [details]
the patch.

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

r=me if you fix the GTK build and the comments below.

> Source/JavaScriptCore/CMakeLists.txt:392
> +    tools/DollarVMPrototype.cpp
> +    tools/JSDollarVM.cpp

For consistency, these should be "JSDollarVM" and "JSDollarVMPrototype" or "DollarVM" and "DollarVMPrototype", but not a mix of the two.

We seem to be gravitating toward a style where we put "JS" in front of any JS API object, since it's so common for normal names -- like Map, for example -- to conflict with a standard name. So, I'd suggest "JSDollarVM" and "JSDollarVMPrototype" here.

> Source/JavaScriptCore/interpreter/StackVisitor.cpp:324
> +#define IDT printIndents(indent)

Rather than a macro, which is hard to decipher and debug, and which violates our coding standards by putting two statements on the same line, can you just make a helper function that, given an indentation level and some arguments, calls printIndents and then calls dataLog with its remaining arguments?

> Source/JavaScriptCore/tools/DollarVMPrototype.cpp:38
> +// lldb. The JS versions in DollarVMPrototype are implemented on top of these
> +// these.

"these these"

> Source/JavaScriptCore/tools/DollarVMPrototype.cpp:41
> +JS_EXPORT_PRIVATE void gcAndSweep(ExecState*);

This function name should specify whether it does full GC or eden GC.

> Source/JavaScriptCore/tools/DollarVMPrototype.cpp:48
> +JS_EXPORT_PRIVATE void printInternal(JSValue);

Let's call this printValue. "Internal" has little meaning.

> Source/JavaScriptCore/tools/DollarVMPrototype.cpp:88
> +// We'll pass C++ pointers into JS as a double token and convert back when
> +// we consume them. These are convenience functions to do that conversion.
> +template<typename T>
> +static double ptrToDouble(T* ptr)
> +{
> +    union {
> +        double d;
> +        T* ptr;
> +    } u;
> +    u.ptr = ptr;
> +    return u.d;
> +}
> +
> +template<typename T>
> +static T* doubleToPtr(double d)
> +{
> +    union {
> +        double d;
> +        T* ptr;
> +    } u;
> +    u.d = d;
> +    return u.ptr;
> +}

Can you just use bitwise_cast?

[Mark Lam]

(In reply to comment #2)
> Comment on attachment 250941 [details]
> the patch.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=250941&action=review
> 
> r=me if you fix the GTK build and the comments below.

Looks like a conflict with the previous patch that I landed.  Just need to rebase to ToT.
 
> > Source/JavaScriptCore/CMakeLists.txt:392
> > +    tools/DollarVMPrototype.cpp
> > +    tools/JSDollarVM.cpp
> 
> For consistency, these should be "JSDollarVM" and "JSDollarVMPrototype" or
> "DollarVM" and "DollarVMPrototype", but not a mix of the two.
> 
> ... I'd suggest "JSDollarVM" and "JSDollarVMPrototype" here.

Done.

> > Source/JavaScriptCore/interpreter/StackVisitor.cpp:324
> > +#define IDT printIndents(indent)
> 
> Rather than a macro, ... make a helper function that, given an indentation level and some
> arguments, calls printIndents and then calls dataLog with its remaining
> arguments?

Done.  I now use 2 file local variadic templates (log() and logF()) that does the job of dataLog() and dataLogF() but with indentation support.

> > Source/JavaScriptCore/tools/DollarVMPrototype.cpp:38
> > +// lldb. The JS versions in DollarVMPrototype are implemented on top of these
> > +// these.
> 
> "these these"

Fixed.

> > Source/JavaScriptCore/tools/DollarVMPrototype.cpp:41
> > +JS_EXPORT_PRIVATE void gcAndSweep(ExecState*);
> 
> This function name should specify whether it does full GC or eden GC.

Per our offline discussion, I've removed fullGC().  What we have now is:
   gc() - does a synchronous full GC and sweep.
   edenGC() - does a synchronous eden GC and sweep.

> > Source/JavaScriptCore/tools/DollarVMPrototype.cpp:48
> > +JS_EXPORT_PRIVATE void printInternal(JSValue);
> 
> Let's call this printValue. "Internal" has little meaning.

Done.

> > Source/JavaScriptCore/tools/DollarVMPrototype.cpp:88
> Can you just use bitwise_cast?

Fixed.

In addition, I had to make 2 other changes:

1. Refactored Heap::collectAllGarbage() into Heap::collectAndSweep(HeapOperation).
   collectAllGarbage() is now a wrapper around collectAndSweep().  $vm.edenGC() calls collectAndSweep(EdenCollection).

2. Changed CodeBlock::printCallOp() to not compute the CallLinkStatus for FTL codeBlocks since the FTL does not support this.
   Otherwise, we'll get a crash when dumping bytecode for an FTL compiled codeBlock.

Updated patch coming shortly.

[Mark Lam]

Created attachment 250960 [details]
patch 2: applied Geoff's suggestions and fixes.

[Geoffrey Garen]

Comment on attachment 250960 [details]
patch 2: applied Geoff's suggestions and fixes.

r=me

[WebKit Commit Bot]

Attachment 250960 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/interpreter/StackVisitor.cpp:319:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mark Lam]

(In reply to comment #6)
> ERROR: Source/JavaScriptCore/interpreter/StackVisitor.cpp:319:  Weird number
> of spaces at line-start.  Are you using a 4-space indent? 
> [whitespace/indent] [3]
> Total errors found: 1 in 16 files

Accidental extra space.  Will fix before landing.

[Mark Lam]

Created attachment 250968 [details]
patch 3: with fix for bitwise_cast complaint.

[Mark Lam]

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

[Mark Lam]

FYI, a build fix for this patch on 32-bit ports was landed in http://trac.webkit.org/changeset/182934.