[Webkit-unassigned] [Bug 132740] REGRESSION(jsCStack branch merge): It caused ~180 failures on ARM Thumb2 Linux

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=132740


Mark Lam <mark.lam at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #234911|review?                     |review-
               Flag|                            |




--- Comment #6 from Mark Lam <mark.lam at apple.com>  2014-07-15 13:30:43 PST ---
(From update of attachment 234911)
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/.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.