54687 2011-02-17 14:00:38 -0800 Invalid label can be added into backtrack when building with armcc 2011-02-22 08:28:20 -0800 1 1 1 Unclassified WebKit JavaScriptCore 528+ (Nightly build) Other Other RESOLVED FIXED P1 Blocker --- 1 yong.li.webkit webkit-unassigned ap barraclough commit-queue eric msaboff staikos oldest_to_newest 353158 0 yong.li.webkit 2011-02-17 14:00:38 -0800 Yarr::GenerationState::BacktrackDestination::hasDataLabel() doesn't work when it is built with armcc, because JmpDst::isSet compares m_offset against -1, and m_offset is declared with "int m_offset : 31". According to standards, it depends on the compiler implementation that "int" bit field is signed or unsigned. After changing it to "signed int m_offset : 31", it works well. 353160 1 82858 yong.li.webkit 2011-02-17 14:04:10 -0800 Created attachment 82858 the fix 353171 2 yong.li.webkit 2011-02-17 14:17:24 -0800 There are other cases where we use "int" to declare signed bit field. We should also change them to "signed int". That should be low priority and I prefer to do that in another bug 353382 3 ap 2011-02-17 20:50:02 -0800 Is this covered by existing regression tests? 353554 4 yong.li.webkit 2011-02-18 06:05:11 -0800 (In reply to comment #3) > Is this covered by existing regression tests? I'll give a try. It is hard for me to write a test case for this because I know very little about how YarrJIT works. The change is harmless. I also plan to make such change to other places including those in WebCore even they haven't caused any known problem. Probably we should have this rule in coding guidelines: "Always explicitly use "signed"/"unsigned" to declare integral bit fields" 353722 5 82858 eric 2011-02-18 11:56:04 -0800 Comment on attachment 82858 the fix We've had related trouble with bitfields using MSVC as well. There the problem was that enum is a "signed int" and when you make it a bitfield it uses one of the bits as a sign when you don't expect it to. :) This change seems fine. Seems you might want a comment here to explain why you need the "signed" part. 354156 6 yong.li.webkit 2011-02-19 07:43:00 -0800 (In reply to comment #5) > (From update of attachment 82858 [details]) > We've had related trouble with bitfields using MSVC as well. There the problem was that enum is a "signed int" and when you make it a bitfield it uses one of the bits as a sign when you don't expect it to. :) > > This change seems fine. Seems you might want a comment here to explain why you need the "signed" part. Thanks. I plan to add "sign" to all similar places to make the code independent from the implementation differences of compilers (seems Qt always uses "signed" to declare signed bitfields). After that is done, the change in ARMAssember.h will not be special. 354166 7 yong.li.webkit 2011-02-19 07:58:26 -0800 Bug 54807 created for the rest places we use "int"/"short" to declare bitfields 354167 8 82858 commit-queue 2011-02-19 08:06:40 -0800 Comment on attachment 82858 the fix Clearing flags on attachment: 82858 Committed r79124: <http://trac.webkit.org/changeset/79124> 354168 9 commit-queue 2011-02-19 08:06:45 -0800 All reviewed patches have been landed. Closing bug. 354238 10 ap 2011-02-19 20:53:01 -0800 This patch makes no sense to me. "int" is always signed in C++. Eric, I don't understand why this has been given green light. There is no explanation of what specific problem this patch fixes, and according to comment 4, even regression tests haven't been run! 354241 11 eric 2011-02-19 21:52:53 -0800 (In reply to comment #10) > This patch makes no sense to me. "int" is always signed in C++. > > Eric, I don't understand why this has been given green light. There is no explanation of what specific problem this patch fixes, and according to comment 4, even regression tests haven't been run! The patch seemed reasonable to me. Yes, ideally there should have been a link to the armcc bug (as I requested in the other bug he filed on this subject). He states in comment 0 that according to his (I assume yong is a he?) reading of the c++ spec ints in bitfields are unspecified as to their sign. I've not confirmed this. But I could believe that armcc might have such a bug. The cq obviously ran the regression tests. :) Feel free to roll out the change. 354244 12 ap 2011-02-19 22:34:00 -0800 > The cq obviously ran the regression tests. :) I meant, the tests haven't been run on the affected platform. Since this isn't breaking anything, there is no rush to roll out, and I'll give some time for everyone to comment. 354252 13 ap 2011-02-19 23:13:37 -0800 Daniel Bates explained in bug 54807 that I was wrong - bitfields are an exception where "signed int" is different from "int". I think that this should have used "signed", not "signed int" though, just like we never write "unsigned int" in WebKit code. And it doesn't look like we had a good explanation of why there is no regression test. 355289 14 yong.li.webkit 2011-02-22 08:28:20 -0800 (In reply to comment #13) > Daniel Bates explained in bug 54807 that I was wrong - bitfields are an exception where "signed int" is different from "int". > > I think that this should have used "signed", not "signed int" though, just like we never write "unsigned int" in WebKit code. And it doesn't look like we had a good explanation of why there is no regression test. I just tried a few existing JS tests, and I haven't been able to find one that doesn't crash without the fix. (For example, fast/js/const.html, fast/js/array-foreach.html, ...) 82858 2011-02-17 14:04:10 -0800 2011-02-19 08:06:40 -0800 the fix 54687.patch text/plain 1585 yong.li.webkit ZGlmZiAtLWdpdCBhL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cgYi9Tb3VyY2UvSmF2 YVNjcmlwdENvcmUvQ2hhbmdlTG9nCmluZGV4IDM0MjVjYWIuLjMzOWY0ZTQgMTAwNjQ0Ci0tLSBh L1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKKysrIGIvU291cmNlL0phdmFTY3JpcHRD b3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE1IEBACisyMDExLTAyLTE3ICBZb25nIExpICA8eW9s aUByaW0uY29tPgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAg ICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD01NDY4NworICAgICAg ICBXaGVuIGJlaW5nIGJ1aWx0IHdpdGggYXJtY2MsICJpbnQiIGJpdCBmaWVsZHMgYXJlIHRyZWF0 ZWQgYXMKKyAgICAgICAgdW5zaWduZWQgaW50ZWdlcnMsIHdoaWNoIHdpbGwgZmFpbCB0aGUgY29t cGFyaXNvbnMgbGlrZSAibV9vZmZzZXQgPT0gLTEiLgorICAgICAgICBVc2luZyAic2lnbmVkIiBm aXhlcyB0aGUgcHJvYmxlbS4KKworICAgICAgICAqIGFzc2VtYmxlci9BUk1Bc3NlbWJsZXIuaDoK KyAgICAgICAgKiBhc3NlbWJsZXIvQVJNdjdBc3NlbWJsZXIuaDoKKwogMjAxMS0wMi0xNSAgSm9u IEhvbmV5Y3V0dCAgPGpob25leWN1dHRAYXBwbGUuY29tPgogCiAgICAgICAgIFdpbmRvd3MgYnVp bGQgZml4IGZvcgpkaWZmIC0tZ2l0IGEvU291cmNlL0phdmFTY3JpcHRDb3JlL2Fzc2VtYmxlci9B Uk1Bc3NlbWJsZXIuaCBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9hc3NlbWJsZXIvQVJNQXNzZW1i bGVyLmgKaW5kZXggNzdlYzYwZi4uY2Q1MmYzMSAxMDA2NDQKLS0tIGEvU291cmNlL0phdmFTY3Jp cHRDb3JlL2Fzc2VtYmxlci9BUk1Bc3NlbWJsZXIuaAorKysgYi9Tb3VyY2UvSmF2YVNjcmlwdENv cmUvYXNzZW1ibGVyL0FSTUFzc2VtYmxlci5oCkBAIC0yNTAsNyArMjUwLDcgQEAgbmFtZXNwYWNl IEpTQyB7CiAgICAgICAgICAgICAgICAgQVNTRVJUKG1fb2Zmc2V0ID09IG9mZnNldCk7CiAgICAg ICAgICAgICB9CiAKLSAgICAgICAgICAgIGludCBtX29mZnNldCA6IDMxOworICAgICAgICAgICAg c2lnbmVkIGludCBtX29mZnNldCA6IDMxOwogICAgICAgICAgICAgaW50IG1fdXNlZCA6IDE7CiAg ICAgICAgIH07CiAKZGlmZiAtLWdpdCBhL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9hc3NlbWJsZXIv QVJNdjdBc3NlbWJsZXIuaCBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9hc3NlbWJsZXIvQVJNdjdB c3NlbWJsZXIuaAppbmRleCA4NzRkY2RlLi5lMmM3ZTI1IDEwMDY0NAotLS0gYS9Tb3VyY2UvSmF2 YVNjcmlwdENvcmUvYXNzZW1ibGVyL0FSTXY3QXNzZW1ibGVyLmgKKysrIGIvU291cmNlL0phdmFT Y3JpcHRDb3JlL2Fzc2VtYmxlci9BUk12N0Fzc2VtYmxlci5oCkBAIC01MjgsNyArNTI4LDcgQEAg cHVibGljOgogICAgICAgICAgICAgQVNTRVJUKG1fb2Zmc2V0ID09IG9mZnNldCk7CiAgICAgICAg IH0KIAotICAgICAgICBpbnQgbV9vZmZzZXQgOiAzMTsKKyAgICAgICAgc2lnbmVkIGludCBtX29m ZnNldCA6IDMxOwogICAgICAgICBpbnQgbV91c2VkIDogMTsKICAgICB9OwogCg==