[webkit-reviews] review granted: [Bug 115705] Implement a probe mechanism for JIT generated code : [Attachment 201892] revised patch.

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has granted Mark Lam <mark.lam at apple.com>'s
request for review:
Bug 115705: Implement a probe mechanism for JIT generated code
https://bugs.webkit.org/show_bug.cgi?id=115705

Attachment 201892: revised patch.
https://bugs.webkit.org/attachment.cgi?id=201892&action=review

------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=201892&action=review


r=me

> Source/JavaScriptCore/assembler/MacroAssembler.cpp:108
> +// Specifcally, the saved rsp/esp will point to the stack position after we
pop
> +// the ProbeContext frame. The saved rip/eip will point to the address of
the

A little clearer as "...stack position before we push…". The value after we pop
is not known until the probe runs.

> Source/JavaScriptCore/assembler/MacroAssembler.cpp:111
> +void MacroAssembler::probe(MacroAssembler::ProbeFunction function, void*
arg1, void* arg2)

This should move to MacroAssemblerX86Common. It's not so nice to put tons of
#ifdefs into the shared MacroAssembler.

> Source/JavaScriptCore/assembler/MacroAssembler.cpp:114
> +    #define ProbeContextField(field) Address(esp, offsetof(ProbeContext,
field))

The coding style guidelines say this should start with lower case.

> Source/JavaScriptCore/assembler/MacroAssembler.cpp:116
> +    // The X86_64 ABI specifies that the worse case STACK alignment
requirement

Typo: should be "stack".

> Source/JavaScriptCore/assembler/MacroAssembler.cpp:153
> +NO_RETURN_DUE_TO_ASSERT
> +void MacroAssembler::ProbeContext::dumpCPURegisters(const char* indentation)

> +{
> +    UNUSED_PARAM(indentation);
> +    ASSERT_NOT_REACHED();
> +}
> +
> +NO_RETURN_DUE_TO_ASSERT
> +void MacroAssembler::probe(MacroAssembler::ProbeFunction function, void*
arg1, void* arg2)
> +{
> +    UNUSED_PARAM(function);
> +    UNUSED_PARAM(arg1);
> +    UNUSED_PARAM(arg2);
> +    ASSERT_NOT_REACHED();
> +}

I don't think these stubs add anything. Platforms that don't support probing
won't build when probing is enabled.

> Source/JavaScriptCore/assembler/X86Assembler.h:36
> +#include <xmmintrin.h>

Let's #if this to avoid build problems.

> Source/JavaScriptCore/jit/JITStubsX86.h:142
> +    // function may have intentionally changed this values for debugging or

Typo: Should be "these values".

> Source/JavaScriptCore/jit/JITStubsX86.h:143
> +    // "Restore" the register values for returning. Note: the user probe
> +    // function may have intentionally changed this values for debugging or
> +    // testing purposes.

Better to talk about our own API, rather than what other code might do.
Something like: "To enable probes to modify register state, we copy all
registers out of the ProbeContext before returning."

> Source/JavaScriptCore/jit/JITStubsX86.h:167
> +    // Restore the return address for the ret below:
> +    "pushl " STRINGIZE_VALUE_OF(PROBE_CPU_EIP_OFFSET) "(%ebp)" "\n"
> +
> +    // Everything's restored. Lastly, restore the %ebp, and return:
> +    "movl " STRINGIZE_VALUE_OF(PROBE_CPU_EBP_OFFSET) "(%ebp), %ebp" "\n"
> +    "ret" "\n"

These seem like "what" comments.

> Source/JavaScriptCore/jit/JITStubsX86Common.h:41
> +// The following are offsets for MacroAssembler::ProbeContext fields
accessed
> +// the ctiMasmProbeTrampoline stub.

Typo: Should be "accessed by the".

> Source/JavaScriptCore/jit/JITStubsX86_64.h:136
> +    "popq %rax" "\n"
> +    "movq %rax, " STRINGIZE_VALUE_OF(PROBE_CPU_EIP_OFFSET) "(%rsp)" "\n"
> +
> +    "movl %rbx, " STRINGIZE_VALUE_OF(PROBE_CPU_EBX_OFFSET) "(%rsp)" "\n"
> +    "movl %rsp, %rbp" "\n" // Save the ProbeContext*.
> +
> +    "movq %rcx, " STRINGIZE_VALUE_OF(PROBE_CPU_ECX_OFFSET) "(%rbp)" "\n"
> +    "movq %rdx, " STRINGIZE_VALUE_OF(PROBE_CPU_EDX_OFFSET) "(%rbp)" "\n"
> +    "movq %rbx, " STRINGIZE_VALUE_OF(PROBE_CPU_EBX_OFFSET) "(%rbp)" "\n"
> +    "movq %rsi, " STRINGIZE_VALUE_OF(PROBE_CPU_ESI_OFFSET) "(%rbp)" "\n"
> +    "movq %rdi, " STRINGIZE_VALUE_OF(PROBE_CPU_EDI_OFFSET) "(%rbp)" "\n"
> +

This stuff is super hard to read. I think it would be better, in future, for us
to move away from this kind of assembly, and more toward the technique used in
stringLengthTrampolineGenerator() and similar bits of assembly.