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

[bugzilla-daemon at webkit.org]

Brent Fulgham <bfulgham at webkit.org> 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 229176: Patch
https://bugs.webkit.org/attachment.cgi?id=229176&action=review

------- Additional Comments from Brent Fulgham <bfulgham at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=229176&action=review


A few minor corrections. Also, could you confirm that you really wanted to
change the 'nativeCallTrampoline' method to use the t0 rather than t1? I don't
see why that changed.

> Source/JavaScriptCore/llint/LLIntData.cpp:129
> +#if CPU(X86_64) && !OS(WINDOWS) || CPU(ARM64) || ENABLE(LLINT_C_LOOP)

Are we short-circuiting here? Maybe we need parentheses around the combined
X86_64 and !OS(WINDOWS) test? I think the meaning would be clearer.

> Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:300
> +	   # 4 parameter registers, and the call instruction won't put the
return address at the correct stack location then.

This should read:
# Also, we need to manually copy the return address to the stack, since before
the call instruction we allocated space for the 4 parameter registers, and the
call instruction won't put the return address at the correct stack location.

> Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:2084
> +	   loadp JSFunction::m_executable[arg2], t0

Why was this changed from t1 to t0? Do we need to add an 'arg3' case to the
condition windows/non-windows code above?

> Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:2089
> +	   call executableOffsetToFunction[t0]

Again, why is t1 being changed to t0?