Bug 150186 - Add MacroAssembler::callProbe() for supporting lambda JIT probes.
Summary: Add MacroAssembler::callProbe() for supporting lambda JIT probes.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-10-15 13:58 PDT by Mark Lam
Modified: 2015-10-16 13:38 PDT (History)
8 users (show)

See Also:


Attachments
the patch. (5.32 KB, patch)
2015-10-15 14:00 PDT, Mark Lam
ggaren: review+
Details | Formatted Diff | Diff
patch for landing. (5.27 KB, patch)
2015-10-16 13:32 PDT, Mark Lam
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Lam 2015-10-15 13:58:20 PDT
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.
Comment 1 Mark Lam 2015-10-15 14:00:53 PDT
Created attachment 263187 [details]
the patch.
Comment 2 WebKit Commit Bot 2015-10-15 14:02:18 PDT
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.
Comment 3 Mark Lam 2015-10-15 14:04:52 PDT
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.
Comment 4 Geoffrey Garen 2015-10-15 16:01:02 PDT
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
Comment 5 Mark Lam 2015-10-16 13:32:42 PDT
Created attachment 263319 [details]
patch for landing.
Comment 6 Mark Lam 2015-10-16 13:38:17 PDT
Thanks for the review.  Landed in r191202: <http://trac.webkit.org/r191202>.