[webkit-reviews] review denied: [Bug 115705] Implement a probe mechanism for JIT generated code : [Attachment 200869] the patch.

[bugzilla-daemon at webkit.org]

Michael Saboff <msaboff at apple.com> has denied 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 200869: the patch.
https://bugs.webkit.org/attachment.cgi?id=200869&action=review

------- Additional Comments from Michael Saboff <msaboff at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=200869&action=review


I think that the space allocated for saving registers in the cpu struct is
overly complex.  I suggest that you directly layout the registers using
properly aligned C/C++ types and let the compile do the work.  For example, the
xmm registers can have the type of __m128 which will be aligned. Further, I
would then use offset_of() to get the offset within the cpu register save
structure instead of just using it in a COMPILE_ASSERT.  i.e.
PROBE_CPU_EAX_OFFSET becomes offsetof(struct ASMProbeContext, cpu.eax).  This
should eliminate much of the FOR_EACH_ and the COMPILE_ASSERTs.

> Source/JavaScriptCore/assembler/ASMProbeContext.h:63
> +    struct {
> +	   #define DECLARE_REGISTER(_type, _regName) \
> +	       _type _regName;
> +	   FOR_EACH_CPU_REGISTER(DECLARE_REGISTER)
> +	   #undef DECLARE_REGISTER
> +    } cpu;

I'd prefer if there was a CPUState struct defined in each MacroAssembler and
the used here.

> Source/JavaScriptCore/assembler/MacroAssembler.cpp:107
> +    // Pack the space to allocate so that the stack pointer will remain
> +    // 32 byte aligned.
> +    const int packedCPUContextSize = WTF_PACK(sizeof(ASMProbeContext), 5);

Why do we need 32 byte alignment?

> Source/JavaScriptCore/assembler/MacroAssembler.cpp:115
> +    // Save the original sp as the farme pointer:

Typo "frame"

> Source/JavaScriptCore/assembler/X86Assembler.h:81
> +    struct XMMRegisterValue {
> +	   uint32_t u0;
> +	   uint32_t u1;
> +	   uint32_t u2;
> +	   uint32_t u3;
> +    };

Use __m128 instead.

> Source/JavaScriptCore/assembler/X86Assembler.h:88
> +    // We pad some space after the GPRegs storage because we want the FPRegs

> +    // storage to start on a 16 byte (128 bit) alignment.
> +    #define FOR_EACH_CPU_REGISTER(V) \
> +	   FOR_EACH_CPU_GPREGISTER(V) \
> +	   FOR_EACH_CPU_PADDING(V) \
> +	   FOR_EACH_CPU_FPREGISTER(V)

Use compiler types that are already padded to eliminate explicit padding.

> Source/JavaScriptCore/jit/JITStubs.cpp:95
> +#define PROBE_PROBE_FUNCTION_OFFSET (0 * PTR_SIZE)
> +#define PROBE_ARG1_OFFSET (1 * PTR_SIZE)
> +#define PROBE_ARG2_OFFSET (2 * PTR_SIZE)
> +#define PROBE_JIT_STACK_FRAME_OFFSET (3 * PTR_SIZE)

Use offset_of() for these.

> Source/JavaScriptCore/jit/JITStubs.cpp:104
> +#define PROBE_CPU_EAX_OFFSET (4 * PTR_SIZE)
> +#define PROBE_CPU_ECX_OFFSET (5 * PTR_SIZE)
> +#define PROBE_CPU_EDX_OFFSET (6 * PTR_SIZE)
> +#define PROBE_CPU_EBX_OFFSET (7 * PTR_SIZE)
> +#define PROBE_CPU_ESP_OFFSET (8 * PTR_SIZE)
> +#define PROBE_CPU_EBP_OFFSET (9 * PTR_SIZE)
> +#define PROBE_CPU_ESI_OFFSET (10 * PTR_SIZE)
> +#define PROBE_CPU_EDI_OFFSET (11 * PTR_SIZE)

Ditto

> Source/JavaScriptCore/jit/JITStubs.cpp:122
> +#define PROBE_CPU_R8_OFFSET (12 * PTR_SIZE)
> +#define PROBE_CPU_R9_OFFSET (13 * PTR_SIZE)
> +#define PROBE_CPU_R10_OFFSET (14 * PTR_SIZE)
> +#define PROBE_CPU_R11_OFFSET (15 * PTR_SIZE)
> +#define PROBE_CPU_R12_OFFSET (16 * PTR_SIZE)
> +#define PROBE_CPU_R13_OFFSET (17 * PTR_SIZE)
> +#define PROBE_CPU_R14_OFFSET (18 * PTR_SIZE)
> +#define PROBE_CPU_R15_OFFSET (19 * PTR_SIZE)
> +#define PROBE_CPU_EIP_OFFSET (20 * PTR_SIZE)
> +#define PROBE_FIRST_XMM_OFFSET (22 * PTR_SIZE) // After padding.

Ditto

> Source/JavaScriptCore/jit/JITStubs.cpp:134
> +#define PROBE_CPU_XMM0_OFFSET (PROBE_FIRST_XMM_OFFSET + (0 * XMM_SIZE))
> +#define PROBE_CPU_XMM1_OFFSET (PROBE_FIRST_XMM_OFFSET + (1 * XMM_SIZE))
> +#define PROBE_CPU_XMM2_OFFSET (PROBE_FIRST_XMM_OFFSET + (2 * XMM_SIZE))
> +#define PROBE_CPU_XMM3_OFFSET (PROBE_FIRST_XMM_OFFSET + (3 * XMM_SIZE))
> +#define PROBE_CPU_XMM4_OFFSET (PROBE_FIRST_XMM_OFFSET + (4 * XMM_SIZE))
> +#define PROBE_CPU_XMM5_OFFSET (PROBE_FIRST_XMM_OFFSET + (5 * XMM_SIZE))
> +#define PROBE_CPU_XMM6_OFFSET (PROBE_FIRST_XMM_OFFSET + (6 * XMM_SIZE))
> +#define PROBE_CPU_XMM7_OFFSET (PROBE_FIRST_XMM_OFFSET + (7 * XMM_SIZE))

Ditto

> Source/WTF/wtf/Alignment.h:55
> +    // That means maskBots is 0s followed by N 1s (as expected). We can use

Type *maskBits*