Bug 124945

Summary: [MIPS] Small stack frame causes regressions.
Product: WebKit Reporter: Balazs Kilvady <kilvadyb>
Component: JavaScriptCoreAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, fpizlo, fu, gergely, ggaren, jbriance, msaboff, palfia
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
proposed patch.
none
fixed patch. none

Description Balazs Kilvady 2013-11-27 11:20:56 PST
On MIPS the LLInt stack frame size was only 20 bytes while code generated for jsCodeEntryFor(JSC::CodeForConstruct) uses more stack space.
Comment 1 Balazs Kilvady 2013-11-27 11:33:54 PST
Created attachment 217960 [details]
proposed patch.
Comment 2 Julien Brianceau 2013-11-28 00:25:00 PST
Comment on attachment 217960 [details]
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=217960&action=review

> Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:193
> +        const extraStackSpace = 28

FYI 28 seems to be not enough on my mips board, with run-fast-jsc I get:
- "273 tests passed, 227 tests failed, 227 tests crashed." with extraStackSpace = 20
- "437 tests passed, 63 tests failed, 63 tests crashed." with extraStackSpace = 28
- "490 tests passed, 10 tests failed, 9 tests crashed." with extraStackSpace = 32
Comment 3 Michael Saboff 2013-11-29 10:46:06 PST
Comment on attachment 217960 [details]
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=217960&action=review

>> Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:193
>> +        const extraStackSpace = 28
> 
> FYI 28 seems to be not enough on my mips board, with run-fast-jsc I get:
> - "273 tests passed, 227 tests failed, 227 tests crashed." with extraStackSpace = 20
> - "437 tests passed, 63 tests failed, 63 tests crashed." with extraStackSpace = 28
> - "490 tests passed, 10 tests failed, 9 tests crashed." with extraStackSpace = 32

Due to alignment, I think you want to add space in amounts of 16.  What are the test results with 36?
Comment 4 Balazs Kilvady 2013-12-01 06:12:19 PST
Created attachment 218110 [details]
fixed patch.

I found that for 8 byte alignment reasons the 20 + x * 8 formula is reliable for extraStackSpace and in tests the lowest best value is 36 (It sees we have to add space for the four parameter registers on the stack also even if they aren't used). In mozilla tests there are still 13 regressions but they are caused by invalid register gp values. I think the fix of the gp problem should go to an other bug report/patch.
Comment 5 Julien Brianceau 2013-12-02 00:54:20 PST
(In reply to comment #3)
> Due to alignment, I think you want to add space in amounts of 16.  What are the test results with 36?

I get the same results with 32 and 36 for layout jsc tests.


(In reply to comment #4)
> Created an attachment (id=218110) [details]
> fixed patch.

Patch looks good to me.
Comment 6 Balazs Kilvady 2013-12-02 04:43:44 PST
(In reply to comment #5)
> (In reply to comment #3)
> > Due to alignment, I think you want to add space in amounts of 16.  What are the test results with 36?
> 
> I get the same results with 32 and 36 for layout jsc tests.
Alignment problem comes up on mips32r2 boards where value 32 breaks the 8 by alignment of the stack and causes 1066 failures while value 36 is ok so stack is aligned.

> (In reply to comment #4)
> > Created an attachment (id=218110) [details] [details]
> > fixed patch.
> 
> Patch looks good to me.
Comment 7 WebKit Commit Bot 2013-12-02 08:38:47 PST
Comment on attachment 218110 [details]
fixed patch.

Clearing flags on attachment: 218110

Committed r159935: <http://trac.webkit.org/changeset/159935>
Comment 8 WebKit Commit Bot 2013-12-02 08:38:49 PST
All reviewed patches have been landed.  Closing bug.