Bug 124945 – [MIPS] Small stack frame causes regressions.

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Balazs Kilvady]

On MIPS the LLInt stack frame size was only 20 bytes while code generated for jsCodeEntryFor(JSC::CodeForConstruct) uses more stack space.

[Balazs Kilvady]

Created attachment 217960 [details]
proposed patch.

[Julien Brianceau]

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

[Michael Saboff]

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?

[Balazs Kilvady]

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.

[Julien Brianceau]

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

[Balazs Kilvady]

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

[WebKit Commit Bot]

Comment on attachment 218110 [details]
fixed patch.

Clearing flags on attachment: 218110

Committed r159935: <http://trac.webkit.org/changeset/159935>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.