Bug 150186 – Add MacroAssembler::callProbe() for supporting lambda JIT probes.

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

[Mark Lam]

With callProbe(), we can now make probes that are lambdas.  For example, we can now conveniently add probes like so: 

    // When you know exactly which register you want to inspect:
    jit.callProbe([] (MacroAssembler::ProbeContext* context) {
        intptr_t value = reinterpret_cast<intptr_t>(context->cpu.eax);
        dataLogF("eax %p\n", context->cpu.eax); // Inspect the register.
        ASSERT(value > 10); // Add test code for debugging.
    });

    // When you want to inspect whichever register the JIT allocated:
    auto reg = op1.gpr();
    jit.callProbe([reg] (MacroAssembler::ProbeContext* context) {
        intptr_t value = reinterpret_cast<intptr_t>(context->gpr(reg));
        dataLogF("reg %s: %ld\n", context->gprName(reg), value);
        ASSERT(value > 10);
    });

callProbe() is only meant to be used for debugging sessions.  It is not appropriate to use it in permanent code (even for debug builds).  This is because:
1. The probe mechanism saves and restores all (and I really mean "all") registers, and is inherently slow.
2. callProbe() currently works by allocating (via new) StdFunctionData structs to keep the callback std::function alive, but never deletes them i.e. it leaks a bit of memory each time the JIT runs it.

These limitations are acceptable for a debugging session (assuming you're not debugging a memory leak), but not for deployment code.  If there's a need, we can plug that leak in another patch.

[Mark Lam]

Created attachment 263187 [details]
the patch.

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/assembler/MacroAssembler.h:1593:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssembler.cpp:37:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssembler.cpp:38:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssembler.cpp:42:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssembler.cpp:51:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 5 in 4 files


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

[Mark Lam]

Comment on attachment 263187 [details]
the patch.

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

> Source/JavaScriptCore/assembler/MacroAssembler.cpp:37
> +struct StdFunctionData
> +{

Oops.  Should put these on the same line.

[Geoffrey Garen]

Comment on attachment 263187 [details]
the patch.

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

r=me

Please fix non-idiomatic use of std::function.

> Source/JavaScriptCore/assembler/MacroAssembler.cpp:42
> +    StdFunctionData(std::function<void (MacroAssembler::ProbeContext*)> func)
> +        : func(func)
> +    { }
> +    
> +    std::function<void (MacroAssembler::ProbeContext*)> func;

You shouldn't wrap std::function. std::function is the wrapper.

> Source/JavaScriptCore/assembler/MacroAssembler.cpp:47
> +    auto stdFunctionData = reinterpret_cast<StdFunctionData*>(context->arg1);

static_cast

[Mark Lam]

Created attachment 263319 [details]
patch for landing.

[Mark Lam]

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