[webkit-reviews] review denied: [Bug 130638] [Win64] ASM LLINT is not enabled. : [Attachment 233450] Patch

[bugzilla-daemon at webkit.org]

Mark Lam <mark.lam at apple.com> has denied peavo at outlook.com's request for
review:
Bug 130638: [Win64] ASM LLINT is not enabled.
https://bugs.webkit.org/show_bug.cgi?id=130638

Attachment 233450: Patch
https://bugs.webkit.org/attachment.cgi?id=233450&action=review

------- Additional Comments from Mark Lam <mark.lam at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=233450&action=review


There's a chance that this patch could work and that the errors in stack
pointer adjustments only resulted in a some wasted slots (based on my educated
guess of what is happening here).  However, I'd rather not approve this patch
based on my guesses.  Please fix the math on all the stack pointer adjustments,
or explain what I'm missing.  Thanks.

> Source/JavaScriptCore/assembler/MacroAssemblerX86_64.h:157
> +	   // JIT relies on that the CallerFrame (frame pointer) is put on the
stack,

/is put on the stack,/being on the stack./

> Source/JavaScriptCore/assembler/MacroAssemblerX86_64.h:166
> +	   // We also need to allocate the shadow space on the stack for the 4
parameter registers.
> +	   // Also, to avoid the potential problem that the callee will
overwrite the stored frame pointer
> +	   // when it copies it's parameter registers into the shadow space, we
should allocate another 32 bytes.
> +	   // Note: POKE_ARGUMENT_OFFSET is adjusted accordingly.
> +	   sub64(TrustedImm32(8 * sizeof(int64_t)), X86Registers::esp);

I'm still not sure if I understand what you're trying to do here.  Here's what
I see needs to be done:

1. Allocate space for the CallerFrame pointer and return address (though not
populated) [2 slots]
2. Allocate space for the shadow space for the 4 parameter registers [4 slots]

Each of these slots are 64-bits.  Hence, the stack pointer is still 64-bit (16
byte) aligned after "pushing" the 6 slots.  It doesn't look like you there's
any alignment padding needed.

So, I don't understand the following:
1. why are you allocating 8 slots?
2. You said that the callee may copy the parameter registers into the shadow
space.	Isn't that what the 4 slots is for?

There are 2 slots that are unexplained and unaccounted for here.  Can you
please explain what you're trying to achieve here (with the 2 slots)?  If I've
misunderstood your reasoning in my comments above, please correct me.  Thanks.

> Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:333
> +	   # We need to allocate 48 bytes on the stack; 32 bytes for the shadow
space, and 8 bytes for the frame pointer we put on the stack,
> +	   # and 8 bytes for the function return address (which isn't really
needed, so we only make room for it here).
> +	   subp 48, sp

This is wrong.	It should be "subp 32, sp".

If you look at the "else" case below, you'll see that it adds 2 slots to the
sp.  The reason for this is because the sp is already pointing to the base of a
CallFrame, which starts with a CallerFrame pointer followed by a return
address.  When we call the host function here, we're expecting the callee to
push the return address followed by the CallerFrame pointer.  That is why we
add 16 (i.e. 2 slots) to the sp.

In your case, the callee will not store the return address nor the CallerFrame
pointer.  Hence, there's no need for the "addp 16, sp" adjustment.  It is
sufficient to store the cfr in [sp] as you have done above.  Thereafter, you
only need to "sub 32, sp" to make space for the shadow space.  There's no need
for another 16 bytes there.  This appears to be the same logic issue with your
other adjustments above.  Please correct me if I've misunderstood something
here.

> Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:335
> +	   addp 48, sp

"addp 32, sp" for reasons stated above.