[webkit-reviews] review denied: [Bug 132740] REGRESSION(jsCStack branch merge): It caused ~180 failures on ARM Thumb2 Linux : [Attachment 234911] Patch

[bugzilla-daemon at webkit.org]

Mark Lam <mark.lam at apple.com> has denied Zan Dobersek <zandobersek at gmail.com>'s
request for review:
Bug 132740: REGRESSION(jsCStack branch merge): It caused ~180 failures on ARM
Thumb2 Linux
https://bugs.webkit.org/show_bug.cgi?id=132740

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

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


> Source/JavaScriptCore/ChangeLog:17
> +	   The current ARM procedure call code in JIT::compileCallEval and
LLInt's makeHostFunctionCall
> +	   is tailored for the iOS target, which has a neat feature of ensuring
that the cfr (r7) and
> +	   lr (r14) registers are pushed on the stack as the last two registers
in the first of two spill
> +	   areas for the general-purpose registers. The location of the
JSC::CallFrame object can then be
> +	   easily predicted by deducing the location of the to-be-spilled cfr
register.
> +
> +	   On Linux, the generated code closely follows the AAPCS and hence
can't ensure that the two
> +	   registers will be placed side-by-side when pushed on the stack. This
does happen when building
> +	   with disabled optimizations, but that's not preferrable. Instead,
the performance is traded for
> +	   correctness by manually storing the cfr and lr registers on the
stack.

This is inaccurate.  According to the AAPCS spec
(http://infocenter.arm.com/help/topic/com.arm.doc.ihi0042e/IHI0042E_aapcs.pdf)
Section 5.1.1 on Page 15, the callee subroutine must preserve the SP register:
"A subroutine must preserve the contents of the registers r4-r8, r10, r11 and
SP ...”.  According to Section 5.3 on Page 17,	the return address must be
stored in the LR register.  Therefore, it also follows that the value in LR
should preserved before a subroutine makes a call to another subroutine.  The
AAPCS does not specify nor restrict the way in which the SP and LR are
preserved.  Hence, this is not a AAPCS compliance issue.

It looks like the issue is that your toolchain implements ARM EABI, which
according to http://sourceware.org/ml/libc-ports/2009-06/msg00012.html, "has no
defined standard frame layout”.

The JSC JIT however, expects a standard frame layout which looks like the
CallerFrameAndPC (in JSStack.h).  On ARM, we expect the callee to push LR
followed by the previous frame register.  The frame register is then set to SP
(in effect, preserving SP).

The issue here is that your GCC flavor’s implementation of the ARM EABI allows
for optimizing out the standard frame layout.  Hence, the fix needed here
should be to add EABI support.	This is not an iOS vs AAPCS issue.

With this understanding in mind, let’s look at your patch below.

> Source/JavaScriptCore/ChangeLog:21
> +	   (JSC::JIT::compileCallEval): Store the cfr and lr registers in the
properly allocated space on
> +	   the stack, cleaning it up after the call to the operation..

Redundant ‘.’

> Source/JavaScriptCore/ChangeLog:22
> +	   * llint/LLIntOfflineAsmConfig.h: Define OFFLINE_ASM_ARM_AAPCS_ABI to
1 for non-iOS ARM targets.

Please fix this comment.

> Source/JavaScriptCore/jit/JITCall32_64.cpp:222
> +#if CPU(ARM) && !PLATFORM(IOS)

This should be “#if COMPILER_SUPPORTS(EABI) && CPU(ARM)” instead.

> Source/JavaScriptCore/jit/JITCall32_64.cpp:224
> +    store32(linkRegister, Address(stackPointerRegister, 4));

This is wrong.	The saved LR value should be the return point after the call
emitted by the callOperationNoExceptionCheck() below.  Instead, you’re saving
the LR of the caller i.e. the return address of the caller’s caller.

> Source/JavaScriptCore/jit/JITCall32_64.cpp:233
> +#if CPU(ARM) && !PLATFORM(IOS)

This should be “#if COMPILER_SUPPORTS(EABI) && CPU(ARM)” instead.

> Source/JavaScriptCore/llint/LLIntOfflineAsmConfig.h:173
> +#if CPU(ARM) && !PLATFORM(IOS)
> +#define OFFLINE_ASM_ARM_AAPCS_ABI 1
> +#else
> +#define OFFLINE_ASM_ARM_AAPCS_ABI 0
> +#endif

This should be:
#if COMPILER_SUPPORTS(EABI) && CPU(ARM)
#define OFFLINE_ASM_ARM_EABI 1
#else
#define OFFLINE_ASM_ARM_EABI 0
#endif

> Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:362
> +	   elsif ARM_AAPCS_ABI

s/ARM_AAPCS_ABI/ARM_EABI/.

> Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:364
> +	       storep lr, 4[sp]

This is wrong.	The LR value that should be stored is the return address after
the call instruction below.  At this point, LR does not contain the correct
value.

> Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:373
> +	   elsif ARM_AAPCS_ABI

s/ARM_AAPCS_ABI/ARM_EABI/.