Bug 132740 – REGRESSION(jsCStack branch merge): It caused ~180 failures on ARM Thumb2 Linux

• Status: RESOLVED
• Resolution: DUPLICATE
• Component: JavaScriptCore

[Csaba Osztrogonác]

EFL ARM Thumb2 bot: (GCC 4.8)
 - http://build.webkit.sed.hu/builders/EFL%20ARMv7%20Linux%20Release%20%28Build%29/builds/4026
GTK bot:
 - http://build.webkit.org/builders/GTK%20Linux%20ARM%20Release/builds/1870

We started to debug it, but haven't found the root of the problem yet.
It seems getbyid can't find __proto__ member of Object, Number, ...
after CStack branch merge for some reason. It's so strange, but
on debug build and on release build with -O0 (instead of the default -O3)
__proto__ exists as expected and tests pass. So it might be some
conflict between the CStack branch code and GCC compiler optimizations.

We are going to allocate somebody from the University to 
debug and fix this annoying bug next week.

[Zan Dobersek]

Created attachment 234911 [details]
Patch

[Zoltan Herczeg]

The patch looks good.

[Csaba Osztrogonác]

*** Bug 133283 has been marked as a duplicate of this bug. ***

[Csaba Osztrogonác]

Thanks for the patch, I tested it on the EFL bot, it really fixes this bug:
http://build.webkit.sed.hu/builders/EFL%20ARMv7%20Linux%20Release%20%28Build%29/builds/5074

[Mark Lam]

(In reply to comment #4)
> Thanks for the patch, I tested it on the EFL bot, it really fixes this bug:
> http://build.webkit.sed.hu/builders/EFL%20ARMv7%20Linux%20Release%20%28Build%29/builds/5074

I think this patch is doing the wrong thing.  I’m still thinking it true and will provide a detailed review with suggestions soon.

[Mark Lam]

Comment on attachment 234911 [details]
Patch

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/.

[Zan Dobersek]

Created attachment 235346 [details]
Patch

[WebKit Commit Bot]

Attachment 235346 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/jit/JITCall32_64.cpp:231:  The parameter name """" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/llint/LowLevelInterpreter.cpp:535:  The parameter name """" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

Comment on attachment 235346 [details]
Patch

This looks like a giant hack. It's really disgusting. Can you be more specific about what kind of frame layout the compiler generates and how it breaks our assumptions?

[Zan Dobersek]

(In reply to comment #9)
> (From update of attachment 235346 [details])
> This looks like a giant hack. It's really disgusting. Can you be more specific about what kind of frame layout the compiler generates and how it breaks our assumptions?

It definitely is a dirty hack.

This is an example of a function prologue that should outline the problem. Generated with GCC 4.8.

00055c9e <operationNewArrayWithSizeAndProfile>:
   55c9e:	e92d 43b0 	stmdb	sp!, {r4, r5, r7, r8, r9, lr}
   55ca2:	b084      	sub	sp, #16
   55ca4:	af02      	add	r7, sp, #8

Epilogue:
   55cd8:	3708      	adds	r7, #8
   55cda:	46bd      	mov	sp, r7
   55cdc:	e8bd 83b0 	ldmia.w	sp!, {r4, r5, r7, r8, r9, pc}

isARMArea1Register() and isARMArea2Register() functions in LLVM's ARMBaseRegisterInfo.h show that for iOS two register spill areas are used to get r7 and lr placed on the stack side by side.
http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/ARM/ARMBaseRegisterInfo.h?view=markup

[Michael Saboff]

(In reply to comment #10)
> (In reply to comment #9)
> > (From update of attachment 235346 [details] [details])
> > This looks like a giant hack. It's really disgusting. Can you be more specific about what kind of frame layout the compiler generates and how it breaks our assumptions?
> 
> It definitely is a dirty hack.
> 
> This is an example of a function prologue that should outline the problem. Generated with GCC 4.8.
> 
> 00055c9e <operationNewArrayWithSizeAndProfile>:
>    55c9e:    e92d 43b0     stmdb    sp!, {r4, r5, r7, r8, r9, lr}
>    55ca2:    b084          sub    sp, #16
>    55ca4:    af02          add    r7, sp, #8
> 
> Epilogue:
>    55cd8:    3708          adds    r7, #8
>    55cda:    46bd          mov    sp, r7
>    55cdc:    e8bd 83b0     ldmia.w    sp!, {r4, r5, r7, r8, r9, pc}
> 
> isARMArea1Register() and isARMArea2Register() functions in LLVM's ARMBaseRegisterInfo.h show that for iOS two register spill areas are used to get r7 and lr placed on the stack side by side.
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/ARM/ARMBaseRegisterInfo.h?view=markup

The prologue and epilogue look like they save and restore lr at the highest address in the callee's stack.  This is consistent with what the return PC location in struct CallerFrameAndPC.  The frame pointer (r7) isn't stored in the location we'd like for struct CallerFrameAndPC, but that may not be an issue (more below).

We may need to move the stack pointer down similar to what is proposed in https://bugs.webkit.org/attachment.cgi?id=237526 posted for https://bugs.webkit.org/show_bug.cgi?id=136436 so that we don't try to use the saved contents of r9 as our caller's frame pointer.  If we made an update to that proposed change along with the already landed changes http://trac.webkit.org/changeset/173131 and http://trac.webkit.org/changeset/173031 that set the caller frame when transitioning from C++ to JavaScriptCode, I think we'd be good or nearly so.  The change to the proposed patch might only be adding || (COMPILER_SUPPORTS(EABI) && CPU(ARM)) to the #if NUMBER_OF_ARGUMENT_REGISTERS < 4 and changing the comment.

The only remaining issue relates to the fp (r7) not being stored in the "right" place for JavaScriptCore.  The issue here is unwinding.  JavaScriptCore has it's own unwinding code that skips over native (C++) frames.  I believe with the changes landed and the the additional one described above, the current unwind code will work.  Though I don't know how a debugger like gdb would handle unwinding through a mix of native and JavaScript frames.

[Michael Saboff]

Think this may be fixed by change set r173282: <http://trac.webkit.org/changeset/173282> from https://bugs.webkit.org/show_bug.cgi?id=136436.  Could someone with access to an appropriate system verify that this has been fixed and if so, close out as a duplicate?

[Csaba Osztrogonác]

(In reply to comment #12)
> Think this may be fixed by change set r173282: <http://trac.webkit.org/changeset/173282> from https://bugs.webkit.org/show_bug.cgi?id=136436.  Could someone with access to an appropriate system verify that this has been fixed and if so, close out as a duplicate?

Thanks for the fix, it seems it solved the problem, at least the EFL Thumb2 bot
is happy now: http://build.webkit.sed.hu/builders/EFL%20ARMv7%20Linux%20Release%20%28Build%29/builds/5916

*** This bug has been marked as a duplicate of bug 136436 ***

[Csaba Osztrogonác]

Comment on attachment 235346 [details]
Patch

Cleared review? from attachment 235346 [details] so that this bug does not appear in http://webkit.org/pending-review.  If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).