124945 2013-11-27 11:20:56 -0800 [MIPS] Small stack frame causes regressions. 2013-12-02 08:38:49 -0800 1 1 1 Unclassified WebKit JavaScriptCore 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 kilvadyb webkit-unassigned commit-queue fpizlo fu gergely ggaren jbriance msaboff palfia oldest_to_newest 954579 0 kilvadyb 2013-11-27 11:20:56 -0800 On MIPS the LLInt stack frame size was only 20 bytes while code generated for jsCodeEntryFor(JSC::CodeForConstruct) uses more stack space. 954581 1 217960 kilvadyb 2013-11-27 11:33:54 -0800 Created attachment 217960 proposed patch. 954674 2 217960 jbriance 2013-11-28 00:25:00 -0800 Comment on attachment 217960 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 955002 3 217960 msaboff 2013-11-29 10:46:06 -0800 Comment on attachment 217960 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? 955144 4 218110 kilvadyb 2013-12-01 06:12:19 -0800 Created attachment 218110 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. 955274 5 jbriance 2013-12-02 00:54:20 -0800 (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. 955337 6 kilvadyb 2013-12-02 04:43:44 -0800 (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. 955386 7 218110 commit-queue 2013-12-02 08:38:47 -0800 Comment on attachment 218110 fixed patch. Clearing flags on attachment: 218110 Committed r159935: <http://trac.webkit.org/changeset/159935> 955387 8 commit-queue 2013-12-02 08:38:49 -0800 All reviewed patches have been landed. Closing bug. 217960 2013-11-27 11:33:54 -0800 2013-12-01 06:12:19 -0800 proposed patch. stack.diff text/plain 1191 kilvadyb ZGlmZiAtLWdpdCBhL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cgYi9Tb3VyY2UvSmF2 YVNjcmlwdENvcmUvQ2hhbmdlTG9nCmluZGV4IDUyODVkMjUuLmQyZjE4MTAgMTAwNjQ0Ci0tLSBh L1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKKysrIGIvU291cmNlL0phdmFTY3JpcHRD b3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE0IEBACisyMDEzLTExLTI3ICBCYWxhenMgS2lsdmFk eSAgPGtpbHZhZHliQGhvbWVqaW5uaS5jb20+CisKKyAgICAgICAgW01JUFNdIFNtYWxsIHN0YWNr IGZyYW1lIGNhdXNlcyByZWdyZXNzaW9ucy4KKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5v cmcvc2hvd19idWcuY2dpP2lkPTEyNDk0NQorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAo T09QUyEpLgorCisgICAgICAgIEZpeCBzdGFjayBzcGFjZSBmb3IgTExJbnQgb24gTUlQUy4KKwor ICAgICAgICAqIGxsaW50L0xvd0xldmVsSW50ZXJwcmV0ZXIzMl82NC5hc206CisKIDIwMTMtMTEt MjcgIEFuZHJlYXMgS2xpbmcgIDxha2xpbmdAYXBwbGUuY29tPgogCiAgICAgICAgIFN0cnVjdHVy ZTo6bV9zdGF0aWNGdW5jdGlvblJlaWZpZWQgc2hvdWxkIGJlIGEgc2luZ2xlIGJpdC4KZGlmZiAt LWdpdCBhL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9sbGludC9Mb3dMZXZlbEludGVycHJldGVyMzJf NjQuYXNtIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL2xsaW50L0xvd0xldmVsSW50ZXJwcmV0ZXIz Ml82NC5hc20KaW5kZXggODA3NTRmZi4uYmM4N2YwMSAxMDA2NDQKLS0tIGEvU291cmNlL0phdmFT Y3JpcHRDb3JlL2xsaW50L0xvd0xldmVsSW50ZXJwcmV0ZXIzMl82NC5hc20KKysrIGIvU291cmNl L0phdmFTY3JpcHRDb3JlL2xsaW50L0xvd0xldmVsSW50ZXJwcmV0ZXIzMl82NC5hc20KQEAgLTE5 MCw3ICsxOTAsNyBAQCBtYWNybyBkb0NhbGxUb0phdmFTY3JpcHQoKQogICAgICAgICBjb25zdCBl bnRyeSA9IGEwCiAgICAgICAgIGNvbnN0IG5ld0NhbGxGcmFtZSA9IGExCiAgICAgZWxzaWYgTUlQ UwotICAgICAgICBjb25zdCBleHRyYVN0YWNrU3BhY2UgPSAyMAorICAgICAgICBjb25zdCBleHRy YVN0YWNrU3BhY2UgPSAyOAogICAgICAgICBjb25zdCBwcmV2aW91c0NGUiA9IHQyICAKICAgICAg ICAgY29uc3QgcHJldmlvdXNQQyA9IGxyCiAgICAgICAgIGNvbnN0IGVudHJ5ID0gYTAK 218110 2013-12-01 06:12:19 -0800 2013-12-02 08:38:47 -0800 fixed patch. stack.diff text/plain 1234 kilvadyb ZGlmZiAtLWdpdCBhL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cgYi9Tb3VyY2UvSmF2 YVNjcmlwdENvcmUvQ2hhbmdlTG9nCmluZGV4IGZlOWZjYzYuLjRjYjFmYTIgMTAwNjQ0Ci0tLSBh L1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKKysrIGIvU291cmNlL0phdmFTY3JpcHRD b3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE0IEBACisyMDEzLTEyLTAxICBCYWxhenMgS2lsdmFk eSAgPGtpbHZhZHliQGhvbWVqaW5uaS5jb20+CisKKyAgICAgICAgW01JUFNdIFNtYWxsIHN0YWNr IGZyYW1lIGNhdXNlcyByZWdyZXNzaW9ucy4KKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5v cmcvc2hvd19idWcuY2dpP2lkPTEyNDk0NQorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAo T09QUyEpLgorCisgICAgICAgIEZpeCBzdGFjayBzcGFjZSBmb3IgTExJbnQgb24gTUlQUy4KKwor ICAgICAgICAqIGxsaW50L0xvd0xldmVsSW50ZXJwcmV0ZXIzMl82NC5hc206CisKIDIwMTMtMTEt MjkgIEZpbGlwIFBpemxvICA8ZnBpemxvQGFwcGxlLmNvbT4KIAogICAgICAgICBGaW5hbGx5IHJl bW92ZSB0aG9zZSBERkdfRU5BQkxFIHRoaW5ncwpkaWZmIC0tZ2l0IGEvU291cmNlL0phdmFTY3Jp cHRDb3JlL2xsaW50L0xvd0xldmVsSW50ZXJwcmV0ZXIzMl82NC5hc20gYi9Tb3VyY2UvSmF2YVNj cmlwdENvcmUvbGxpbnQvTG93TGV2ZWxJbnRlcnByZXRlcjMyXzY0LmFzbQppbmRleCA2ODczNGRk Li5hN2NhZmJlIDEwMDY0NAotLS0gYS9Tb3VyY2UvSmF2YVNjcmlwdENvcmUvbGxpbnQvTG93TGV2 ZWxJbnRlcnByZXRlcjMyXzY0LmFzbQorKysgYi9Tb3VyY2UvSmF2YVNjcmlwdENvcmUvbGxpbnQv TG93TGV2ZWxJbnRlcnByZXRlcjMyXzY0LmFzbQpAQCAtMTkwLDggKzE5MCw4IEBAIG1hY3JvIGRv Q2FsbFRvSmF2YVNjcmlwdCgpCiAgICAgICAgIGNvbnN0IGVudHJ5ID0gYTAKICAgICAgICAgY29u c3QgbmV3Q2FsbEZyYW1lID0gYTEKICAgICBlbHNpZiBNSVBTCi0gICAgICAgIGNvbnN0IGV4dHJh U3RhY2tTcGFjZSA9IDIwCi0gICAgICAgIGNvbnN0IHByZXZpb3VzQ0ZSID0gdDIgIAorICAgICAg ICBjb25zdCBleHRyYVN0YWNrU3BhY2UgPSAzNgorICAgICAgICBjb25zdCBwcmV2aW91c0NGUiA9 IHQyCiAgICAgICAgIGNvbnN0IHByZXZpb3VzUEMgPSBscgogICAgICAgICBjb25zdCBlbnRyeSA9 IGEwCiAgICAgICAgIGNvbnN0IG5ld0NhbGxGcmFtZSA9IGExCg==