131495 2014-04-10 09:56:20 -0700 LLINT loadisFromInstruction should handle the big endian case 2014-04-29 06:13:34 -0700 1 1 1 Unclassified WebKit JavaScriptCore 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 128743 1 mark.lam mark.lam commit-queue fpizlo ggaren mhahnenberg mmirman msaboff oliver tpopela oldest_to_newest 999550 0 mark.lam 2014-04-10 09:56:20 -0700 The LLINT loadisFromInstruction macro aims to load the least significant 32-bit word from the 64-bit bytecode instruction stream and sign extend it. For big endian machines, the current implementation would load the wrong 32-bit word. 999551 1 229055 mark.lam 2014-04-10 10:01:55 -0700 Created attachment 229055 The patch. 999552 2 229056 mark.lam 2014-04-10 10:03:47 -0700 Created attachment 229056 patch 2: updated comment. 999557 3 229056 mhahnenberg 2014-04-10 10:12:37 -0700 Comment on attachment 229056 patch 2: updated comment. Does this fix all big endian issues? It seems to me like we do lots of loadi instructions assuming little endianness. 999558 4 mark.lam 2014-04-10 10:14:22 -0700 (In reply to comment #3) > (From update of attachment 229056 [details]) > Does this fix all big endian issues? It seems to me like we do lots of loadi instructions assuming little endianness. I would not presume that. Tomas has been analyzing big endian failures in https://bugs.webkit.org/show_bug.cgi?id=12874. I’m just knocking them down one at a time when he reports them. This one happens to be the next one. It may or may not be the last. 999559 5 mhahnenberg 2014-04-10 10:15:31 -0700 (In reply to comment #4) > (In reply to comment #3) > > (From update of attachment 229056 [details] [details]) > > Does this fix all big endian issues? It seems to me like we do lots of loadi instructions assuming little endianness. > > I would not presume that. Tomas has been analyzing big endian failures in https://bugs.webkit.org/show_bug.cgi?id=12874. I’m just knocking them down one at a time when he reports them. This one happens to be the next one. It may or may not be the last. Hmm. Assuming this isn't the last one, is there a better way to handle these bugs rather than slapping if BIG_ENDIAN everywhere? Could this be handled in the backend somehow? 999561 6 mhahnenberg 2014-04-10 10:18:46 -0700 (In reply to comment #5) > (In reply to comment #4) > > (In reply to comment #3) > > > (From update of attachment 229056 [details] [details] [details]) > > > Does this fix all big endian issues? It seems to me like we do lots of loadi instructions assuming little endianness. > > > > I would not presume that. Tomas has been analyzing big endian failures in https://bugs.webkit.org/show_bug.cgi?id=12874. I’m just knocking them down one at a time when he reports them. This one happens to be the next one. It may or may not be the last. > > Hmm. Assuming this isn't the last one, is there a better way to handle these bugs rather than slapping if BIG_ENDIAN everywhere? Could this be handled in the backend somehow? Or maybe we just need a macro that provides a good abstraction for loading 32 bits that has a single if BIG_ENDIAN and then use that everywhere. 999564 7 mark.lam 2014-04-10 10:19:50 -0700 (In reply to comment #5) > (In reply to comment #4) > > (In reply to comment #3) > > > (From update of attachment 229056 [details] [details] [details]) > > > Does this fix all big endian issues? It seems to me like we do lots of loadi instructions assuming little endianness. > > > > I would not presume that. Tomas has been analyzing big endian failures in https://bugs.webkit.org/show_bug.cgi?id=12874. I’m just knocking them down one at a time when he reports them. This one happens to be the next one. It may or may not be the last. > > Hmm. Assuming this isn't the last one, is there a better way to handle these bugs rather than slapping if BIG_ENDIAN everywhere? Could this be handled in the backend somehow? Correction on the url from my previous comment: it should be https://bugs.webkit.org/show_bug.cgi?id=128743. That would be inappropriate to rig loadi in the backend because a loadi can be used to load the most significant 32-bit if that is the intent. I’m going by the rule that the LLINT asm is assembly, and we should coding it the same way an assembly programmer would code. In this case, the issue isolated to a macro, and that “if BIG_ENDIAN” is isolated. Note: this is not a new idiom. It was already in use previously, and I adopted it here to solve the issue. 999567 8 229056 mhahnenberg 2014-04-10 10:20:42 -0700 Comment on attachment 229056 patch 2: updated comment. Okay, we'll refactor as we encounter new issues. YAGNI! r=me 999576 9 229056 commit-queue 2014-04-10 10:50:42 -0700 Comment on attachment 229056 patch 2: updated comment. Clearing flags on attachment: 229056 Committed r167076: <http://trac.webkit.org/changeset/167076> 999577 10 commit-queue 2014-04-10 10:50:46 -0700 All reviewed patches have been landed. Closing bug. 1004922 11 mark.lam 2014-04-29 06:13:34 -0700 For the record, this fix was invalid. https://bugs.webkit.org/show_bug.cgi?id=132330 will undo it, and make it right. 229055 2014-04-10 10:01:55 -0700 2014-04-10 10:03:47 -0700 The patch. bug-131495.patch text/plain 1547 mark.lam SW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKPT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291 cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkocmV2aXNpb24gMTY3MDczKQorKysgU291cmNl L0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDIwIEBA CisyMDE0LTA0LTEwICBNYXJrIExhbSAgPG1hcmsubGFtQGFwcGxlLmNvbT4KKworICAgICAgICBM TElOVCBsb2FkaXNGcm9tSW5zdHJ1Y3Rpb24gc2hvdWxkIGhhbmRsZSBiaWcgZW5kaWFuIGNhc2Uu CisgICAgICAgIDxodHRwczovL3dlYmtpdC5vcmcvYi8xMzE0OTU+CisKKyAgICAgICAgUmV2aWV3 ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgVGhlIExMSU5UIGxvYWRpc0Zyb21JbnN0 cnVjdGlvbiBtYWNybyBhaW1zIHRvIGxvYWQgdGhlIGxlYXN0IHNpZ25pZmljYW50CisgICAgICAg IDMyLWJpdCB3b3JkIGZyb20gdGhlIDY0LWJpdCBieXRlY29kZSBpbnN0cnVjdGlvbiBzdHJlYW0g YW5kIHNpZ24gZXh0ZW5kCisgICAgICAgIGl0LiAgRm9yIGJpZyBlbmRpYW4gbWFjaGluZXMsIHRo ZSBjdXJyZW50IGltcGxlbWVudGF0aW9uIHdvdWxkIGxvYWQgdGhlCisgICAgICAgIHdyb25nIDMy LWJpdCB3b3JkLgorCisgICAgICAgIFdpdGhvdXQgdGhpcyBmaXgsIHRoZSBKU0MgdGVzdHMgd2ls bCBjcmFzaCBvbiBiaWcgZW5kaWFuIG1hY2hpbmVzLgorICAgICAgICBUaGFua3MgdG8gVG9tYXMg UG9wZWxhIGZvciBkaWFnbm9zaW5nIHRoaXMgaXNzdWUuCisKKyAgICAgICAgKiBsbGludC9Mb3dM ZXZlbEludGVycHJldGVyLmFzbToKKwogMjAxNC0wNC0wOSAgTWFyayBMYW0gIDxtYXJrLmxhbUBh cHBsZS5jb20+CiAKICAgICAgICAgVGVtcG9yYXJpbHkgZGlzYWJsZSB0aGUgSklUIGZvciB0aGUg V2luZG93cyBwb3J0LgpJbmRleDogU291cmNlL0phdmFTY3JpcHRDb3JlL2xsaW50L0xvd0xldmVs SW50ZXJwcmV0ZXIuYXNtCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9sbGlu dC9Mb3dMZXZlbEludGVycHJldGVyLmFzbQkocmV2aXNpb24gMTY3MDczKQorKysgU291cmNlL0ph dmFTY3JpcHRDb3JlL2xsaW50L0xvd0xldmVsSW50ZXJwcmV0ZXIuYXNtCSh3b3JraW5nIGNvcHkp CkBAIC0xMDYsNyArMTA2LDExIEBAIGlmIEpTVkFMVUU2NAogICAgIGNvbnN0IHRhZ01hc2sgPSBj c3IyCiAgICAgCiAgICAgbWFjcm8gbG9hZGlzRnJvbUluc3RydWN0aW9uKG9mZnNldCwgZGVzdCkK K2lmIEJJR19FTkRJQU4KKyAgICAgICAgbG9hZGlzIDQgKyBvZmZzZXQgKiA4W1BCLCBQQywgOF0s IGRlc3QKK2Vsc2UKICAgICAgICAgbG9hZGlzIG9mZnNldCAqIDhbUEIsIFBDLCA4XSwgZGVzdAor ZW5kCiAgICAgZW5kCiAgICAgCiAgICAgbWFjcm8gbG9hZHBGcm9tSW5zdHJ1Y3Rpb24ob2Zmc2V0 LCBkZXN0KQo= 229056 2014-04-10 10:03:47 -0700 2014-04-10 10:50:42 -0700 patch 2: updated comment. bug-131495.patch text/plain 1551 mark.lam SW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKPT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291 cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkocmV2aXNpb24gMTY3MDczKQorKysgU291cmNl L0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDIwIEBA CisyMDE0LTA0LTEwICBNYXJrIExhbSAgPG1hcmsubGFtQGFwcGxlLmNvbT4KKworICAgICAgICBM TElOVCBsb2FkaXNGcm9tSW5zdHJ1Y3Rpb24gc2hvdWxkIGhhbmRsZSB0aGUgYmlnIGVuZGlhbiBj YXNlLgorICAgICAgICA8aHR0cHM6Ly93ZWJraXQub3JnL2IvMTMxNDk1PgorCisgICAgICAgIFJl dmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIFRoZSBMTElOVCBsb2FkaXNGcm9t SW5zdHJ1Y3Rpb24gbWFjcm8gYWltcyB0byBsb2FkIHRoZSBsZWFzdCBzaWduaWZpY2FudAorICAg ICAgICAzMi1iaXQgd29yZCBmcm9tIHRoZSA2NC1iaXQgYnl0ZWNvZGUgaW5zdHJ1Y3Rpb24gc3Ry ZWFtIGFuZCBzaWduIGV4dGVuZAorICAgICAgICBpdC4gIEZvciBiaWcgZW5kaWFuIG1hY2hpbmVz LCB0aGUgY3VycmVudCBpbXBsZW1lbnRhdGlvbiB3b3VsZCBsb2FkIHRoZQorICAgICAgICB3cm9u ZyAzMi1iaXQgd29yZC4KKworICAgICAgICBXaXRob3V0IHRoaXMgZml4LCB0aGUgSlNDIHRlc3Rz IHdpbGwgY3Jhc2ggb24gYmlnIGVuZGlhbiBtYWNoaW5lcy4KKyAgICAgICAgVGhhbmtzIHRvIFRv bWFzIFBvcGVsYSBmb3IgZGlhZ25vc2luZyB0aGlzIGlzc3VlLgorCisgICAgICAgICogbGxpbnQv TG93TGV2ZWxJbnRlcnByZXRlci5hc206CisKIDIwMTQtMDQtMDkgIE1hcmsgTGFtICA8bWFyay5s YW1AYXBwbGUuY29tPgogCiAgICAgICAgIFRlbXBvcmFyaWx5IGRpc2FibGUgdGhlIEpJVCBmb3Ig dGhlIFdpbmRvd3MgcG9ydC4KSW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9sbGludC9Mb3dM ZXZlbEludGVycHJldGVyLmFzbQo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvSmF2YVNjcmlwdENvcmUv bGxpbnQvTG93TGV2ZWxJbnRlcnByZXRlci5hc20JKHJldmlzaW9uIDE2NzA3MykKKysrIFNvdXJj ZS9KYXZhU2NyaXB0Q29yZS9sbGludC9Mb3dMZXZlbEludGVycHJldGVyLmFzbQkod29ya2luZyBj b3B5KQpAQCAtMTA2LDcgKzEwNiwxMSBAQCBpZiBKU1ZBTFVFNjQKICAgICBjb25zdCB0YWdNYXNr ID0gY3NyMgogICAgIAogICAgIG1hY3JvIGxvYWRpc0Zyb21JbnN0cnVjdGlvbihvZmZzZXQsIGRl c3QpCitpZiBCSUdfRU5ESUFOCisgICAgICAgIGxvYWRpcyA0ICsgb2Zmc2V0ICogOFtQQiwgUEMs IDhdLCBkZXN0CitlbHNlCiAgICAgICAgIGxvYWRpcyBvZmZzZXQgKiA4W1BCLCBQQywgOF0sIGRl c3QKK2VuZAogICAgIGVuZAogICAgIAogICAgIG1hY3JvIGxvYWRwRnJvbUluc3RydWN0aW9uKG9m ZnNldCwgZGVzdCkK