151514 2015-11-20 14:13:18 -0800 [ARMv7] stress/op_rshift.js and stress/op_urshift.js are failing. 2016-02-15 13:09:46 -0800 1 1 1 Unclassified WebKit JavaScriptCore WebKit Local Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 108645 1 mark.lam mark.lam benjamin fpizlo ggaren keith_miller msaboff ossy saam webkit-bug-importer oldest_to_newest 1143814 0 mark.lam 2015-11-20 14:13:18 -0800 These newly added tests shows that we have a latent bug somewhere in the 32-bit ARM JITs. 1143815 1 webkit-bug-importer 2015-11-20 14:14:36 -0800 <rdar://problem/23636019> 1143860 2 mark.lam 2015-11-20 15:47:03 -0800 The tests have been temporarily skipped in r192708: <http://trac.webkit.org/r192708>. 1164798 3 mark.lam 2016-02-15 11:57:18 -0800 The issue turns out to be trivial: on ARMv7 (and traditional ARM too), arithmetic shift right (ASR) and logical shift right (LSR) takes an immediate shift amount from 1-32. See http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0204j/Cjacbgca.html. An immediate shift amount of 0 is interpreted as a shift of 32 bits. Meanwhile, our assembler is expecting the immediate shift value to be between 0-31. As a result, a shift amount of 0 is being wrongly encoded with 0 bits which means shift right by 32 bits. The fix is to check if the shift amount is 0, and if so, emit a move. Else, emit the right shift as usual. This issue does not affect left shifts, as the immediate shift amount for left shifts is between 0-31 as our JIT expects. 1164807 4 271354 mark.lam 2016-02-15 12:10:42 -0800 Created attachment 271354 proposed patch. 1164808 5 271354 fpizlo 2016-02-15 12:11:45 -0800 Comment on attachment 271354 proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=271354&action=review > Source/JavaScriptCore/assembler/MacroAssemblerARM.h:278 > + move(src, dest); This is wrong. Please use a zero-extending move. > Source/JavaScriptCore/assembler/MacroAssemblerARM.h:304 > + move(src, dest); Ditto. > Source/JavaScriptCore/assembler/MacroAssemblerARMv7.h:404 > + move(src, dest); Ditto. > Source/JavaScriptCore/assembler/MacroAssemblerARMv7.h:432 > + move(src, dest); Ditto. 1164812 6 mark.lam 2016-02-15 12:14:16 -0800 These changes are in MacroAssemblerARM.h and MacroAssemblerARMv7.h. These operations are 32-bit only. Why do we need a zero extending move? (In reply to comment #5) > Comment on attachment 271354 [details] > proposed patch. > > View in context: > https://bugs.webkit.org/attachment.cgi?id=271354&action=review > > > Source/JavaScriptCore/assembler/MacroAssemblerARM.h:278 > > + move(src, dest); > > This is wrong. Please use a zero-extending move. > > > Source/JavaScriptCore/assembler/MacroAssemblerARM.h:304 > > + move(src, dest); > > Ditto. > > > Source/JavaScriptCore/assembler/MacroAssemblerARMv7.h:404 > > + move(src, dest); > > Ditto. > > > Source/JavaScriptCore/assembler/MacroAssemblerARMv7.h:432 > > + move(src, dest); > > Ditto. 1164813 7 fpizlo 2016-02-15 12:21:22 -0800 (In reply to comment #6) > These changes are in MacroAssemblerARM.h and MacroAssemblerARMv7.h. These > operations are 32-bit only. Why do we need a zero extending move? > > (In reply to comment #5) > > Comment on attachment 271354 [details] > > proposed patch. > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=271354&action=review > > > > > Source/JavaScriptCore/assembler/MacroAssemblerARM.h:278 > > > + move(src, dest); > > > > This is wrong. Please use a zero-extending move. > > > > > Source/JavaScriptCore/assembler/MacroAssemblerARM.h:304 > > > + move(src, dest); > > > > Ditto. > > > > > Source/JavaScriptCore/assembler/MacroAssemblerARMv7.h:404 > > > + move(src, dest); > > > > Ditto. > > > > > Source/JavaScriptCore/assembler/MacroAssemblerARMv7.h:432 > > > + move(src, dest); > > > > Ditto. OMG you're right. Please ignore me. 1164844 8 mark.lam 2016-02-15 13:09:46 -0800 Thanks for the review. Landed in r196591: <http://trac.webkit.org/r196591>. 271354 2016-02-15 12:10:42 -0800 2016-02-15 12:21:30 -0800 proposed patch. bug-151514.patch text/plain 5028 mark.lam SW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKPT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291 cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkocmV2aXNpb24gMTk2NTg1KQorKysgU291cmNl L0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDM3IEBA CisyMDE2LTAyLTE1ICBNYXJrIExhbSAgPG1hcmsubGFtQGFwcGxlLmNvbT4KKworICAgICAgICBb QVJNdjddIHN0cmVzcy9vcF9yc2hpZnQuanMgYW5kIHN0cmVzcy9vcF91cnNoaWZ0LmpzIGFyZSBm YWlsaW5nLgorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9 MTUxNTE0CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAg VGhlIGlzc3VlIHR1cm5zIG91dCB0byBiZSB0cml2aWFsOiBvbiBBUk12NyAoYW5kIHRyYWRpdGlv bmFsIEFSTSB0b28pLCBhcml0aG1ldGljCisgICAgICAgIHNoaWZ0IHJpZ2h0IChBU1IpIGFuZCBs b2dpY2FsIHNoaWZ0IHJpZ2h0IChMU1IpIHRha2VzIGFuIGltbWVkaWF0ZSBzaGlmdCBhbW91bnQK KyAgICAgICAgZnJvbSAxLTMyLiAgU2VlIGh0dHA6Ly9pbmZvY2VudGVyLmFybS5jb20vaGVscC9p bmRleC5qc3A/dG9waWM9L2NvbS5hcm0uZG9jLmR1aTAyMDRqL0NqYWNiZ2NhLmh0bWwuCisgICAg ICAgIEFuIGltbWVkaWF0ZSBzaGlmdCBhbW91bnQgb2YgMCBpcyBpbnRlcnByZXRlZCBhcyBhIHNo aWZ0IG9mIDMyIGJpdHMuCisKKyAgICAgICAgTWVhbndoaWxlLCBvdXIgbWFjcm8gYXNzZW1ibGVy IGlzIGV4cGVjdGluZyB0aGUgaW1tZWRpYXRlIHNoaWZ0IHZhbHVlIHRvIGJlCisgICAgICAgIGJl dHdlZW4gMC0zMS4gIEFzIGEgcmVzdWx0LCBhIHNoaWZ0IGFtb3VudCBvZiAwIGlzIGJlaW5nIHdy b25nbHkgZW5jb2RlZCB3aXRoIDAKKyAgICAgICAgYml0cyB3aGljaCBtZWFucyBzaGlmdCByaWdo dCBieSAzMiBiaXRzLgorCisgICAgICAgIFRoZSBmaXggaXMgdG8gY2hlY2sgaWYgdGhlIHNoaWZ0 IGFtb3VudCBpcyAwLCBhbmQgaWYgc28sIGVtaXQgYSBtb3ZlLiAgRWxzZSwKKyAgICAgICAgZW1p dCB0aGUgcmlnaHQgc2hpZnQgYXMgdXN1YWwuCisKKyAgICAgICAgVGhpcyBpc3N1ZSBkb2VzIG5v dCBhZmZlY3QgbGVmdCBzaGlmdHMsIGFzIHRoZSBpbW1lZGlhdGUgc2hpZnQgYW1vdW50IGZvciBs ZWZ0CisgICAgICAgIHNoaWZ0cyBpcyBiZXR3ZWVuIDAtMzEgYXMgb3VyIG1hY3JvIGFzc2VtYmxl ciBleHBlY3RzLgorCisgICAgICAgICogYXNzZW1ibGVyL01hY3JvQXNzZW1ibGVyQVJNLmg6Cisg ICAgICAgIChKU0M6Ok1hY3JvQXNzZW1ibGVyQVJNOjpyc2hpZnQzMik6CisgICAgICAgIChKU0M6 Ok1hY3JvQXNzZW1ibGVyQVJNOjp1cnNoaWZ0MzIpOgorICAgICAgICAoSlNDOjpNYWNyb0Fzc2Vt YmxlckFSTTo6c3ViMzIpOgorICAgICAgICAqIGFzc2VtYmxlci9NYWNyb0Fzc2VtYmxlckFSTXY3 Lmg6CisgICAgICAgIChKU0M6Ok1hY3JvQXNzZW1ibGVyQVJNdjc6OnJzaGlmdDMyKToKKyAgICAg ICAgKEpTQzo6TWFjcm9Bc3NlbWJsZXJBUk12Nzo6dXJzaGlmdDMyKToKKworICAgICAgICAqIHRl c3RzL3N0cmVzcy9vcF9yc2hpZnQuanM6CisgICAgICAgICogdGVzdHMvc3RyZXNzL29wX3Vyc2hp ZnQuanM6CisgICAgICAgIC0gVW4tc2tpcCB0aGVzZSB0ZXN0cy4gIFRoZXkgc2hvdWxkIGFsd2F5 cyBwYXNzIG5vdy4KKwogMjAxNi0wMi0xNSAgQmVuamFtaW4gUG91bGFpbiAgPGJwb3VsYWluQGFw cGxlLmNvbT4KIAogICAgICAgICBbSlNDXSBJbXByb3ZlIHRoZSBpbnRlcmZhY2Ugb2YgSW5zdDo6 c2hvdWxkVHJ5QWxpYXNpbmdEZWYoKQpJbmRleDogU291cmNlL0phdmFTY3JpcHRDb3JlL2Fzc2Vt Ymxlci9NYWNyb0Fzc2VtYmxlckFSTS5oCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9KYXZhU2NyaXB0 Q29yZS9hc3NlbWJsZXIvTWFjcm9Bc3NlbWJsZXJBUk0uaAkocmV2aXNpb24gMTk2NTg1KQorKysg U291cmNlL0phdmFTY3JpcHRDb3JlL2Fzc2VtYmxlci9NYWNyb0Fzc2VtYmxlckFSTS5oCSh3b3Jr aW5nIGNvcHkpCkBAIC0yNzQsNyArMjc0LDEwIEBAIHB1YmxpYzoKIAogICAgIHZvaWQgcnNoaWZ0 MzIoUmVnaXN0ZXJJRCBzcmMsIFRydXN0ZWRJbW0zMiBpbW0sIFJlZ2lzdGVySUQgZGVzdCkKICAg ICB7Ci0gICAgICAgIG1fYXNzZW1ibGVyLm1vdnMoZGVzdCwgbV9hc3NlbWJsZXIuYXNyKHNyYywg aW1tLm1fdmFsdWUgJiAweDFmKSk7CisgICAgICAgIGlmICghaW1tLm1fdmFsdWUpCisgICAgICAg ICAgICBtb3ZlKHNyYywgZGVzdCk7CisgICAgICAgIGVsc2UKKyAgICAgICAgICAgIG1fYXNzZW1i bGVyLm1vdnMoZGVzdCwgbV9hc3NlbWJsZXIuYXNyKHNyYywgaW1tLm1fdmFsdWUgJiAweDFmKSk7 CiAgICAgfQogCiAgICAgdm9pZCB1cnNoaWZ0MzIoUmVnaXN0ZXJJRCBzaGlmdEFtb3VudCwgUmVn aXN0ZXJJRCBkZXN0KQpAQCAtMjk3LDcgKzMwMCwxMCBAQCBwdWJsaWM6CiAgICAgCiAgICAgdm9p ZCB1cnNoaWZ0MzIoUmVnaXN0ZXJJRCBzcmMsIFRydXN0ZWRJbW0zMiBpbW0sIFJlZ2lzdGVySUQg ZGVzdCkKICAgICB7Ci0gICAgICAgIG1fYXNzZW1ibGVyLm1vdnMoZGVzdCwgbV9hc3NlbWJsZXIu bHNyKHNyYywgaW1tLm1fdmFsdWUgJiAweDFmKSk7CisgICAgICAgIGlmICghaW1tLm1fdmFsdWUp CisgICAgICAgICAgICBtb3ZlKHNyYywgZGVzdCk7CisgICAgICAgIGVsc2UKKyAgICAgICAgICAg IG1fYXNzZW1ibGVyLm1vdnMoZGVzdCwgbV9hc3NlbWJsZXIubHNyKHNyYywgaW1tLm1fdmFsdWUg JiAweDFmKSk7CiAgICAgfQogCiAgICAgdm9pZCBzdWIzMihSZWdpc3RlcklEIHNyYywgUmVnaXN0 ZXJJRCBkZXN0KQpJbmRleDogU291cmNlL0phdmFTY3JpcHRDb3JlL2Fzc2VtYmxlci9NYWNyb0Fz c2VtYmxlckFSTXY3LmgKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291cmNlL0phdmFTY3JpcHRDb3JlL2Fzc2Vt Ymxlci9NYWNyb0Fzc2VtYmxlckFSTXY3LmgJKHJldmlzaW9uIDE5NjU4NSkKKysrIFNvdXJjZS9K YXZhU2NyaXB0Q29yZS9hc3NlbWJsZXIvTWFjcm9Bc3NlbWJsZXJBUk12Ny5oCSh3b3JraW5nIGNv cHkpCkBAIC00MDAsNyArNDAwLDEwIEBAIHB1YmxpYzoKIAogICAgIHZvaWQgcnNoaWZ0MzIoUmVn aXN0ZXJJRCBzcmMsIFRydXN0ZWRJbW0zMiBpbW0sIFJlZ2lzdGVySUQgZGVzdCkKICAgICB7Ci0g ICAgICAgIG1fYXNzZW1ibGVyLmFzcihkZXN0LCBzcmMsIGltbS5tX3ZhbHVlICYgMHgxZik7Cisg ICAgICAgIGlmICghaW1tLm1fdmFsdWUpCisgICAgICAgICAgICBtb3ZlKHNyYywgZGVzdCk7Cisg ICAgICAgIGVsc2UKKyAgICAgICAgICAgIG1fYXNzZW1ibGVyLmFzcihkZXN0LCBzcmMsIGltbS5t X3ZhbHVlICYgMHgxZik7CiAgICAgfQogCiAgICAgdm9pZCByc2hpZnQzMihSZWdpc3RlcklEIHNo aWZ0QW1vdW50LCBSZWdpc3RlcklEIGRlc3QpCkBAIC00MjUsNyArNDI4LDEwIEBAIHB1YmxpYzoK ICAgICAKICAgICB2b2lkIHVyc2hpZnQzMihSZWdpc3RlcklEIHNyYywgVHJ1c3RlZEltbTMyIGlt bSwgUmVnaXN0ZXJJRCBkZXN0KQogICAgIHsKLSAgICAgICAgbV9hc3NlbWJsZXIubHNyKGRlc3Qs IHNyYywgaW1tLm1fdmFsdWUgJiAweDFmKTsKKyAgICAgICAgaWYgKCFpbW0ubV92YWx1ZSkKKyAg ICAgICAgICAgIG1vdmUoc3JjLCBkZXN0KTsKKyAgICAgICAgZWxzZQorICAgICAgICAgICAgbV9h c3NlbWJsZXIubHNyKGRlc3QsIHNyYywgaW1tLm1fdmFsdWUgJiAweDFmKTsKICAgICB9CiAKICAg ICB2b2lkIHVyc2hpZnQzMihSZWdpc3RlcklEIHNoaWZ0QW1vdW50LCBSZWdpc3RlcklEIGRlc3Qp CkluZGV4OiBTb3VyY2UvSmF2YVNjcmlwdENvcmUvdGVzdHMvc3RyZXNzL29wX3JzaGlmdC5qcwo9 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09Ci0tLSBTb3VyY2UvSmF2YVNjcmlwdENvcmUvdGVzdHMvc3RyZXNzL29wX3JzaGlm dC5qcwkocmV2aXNpb24gMTk2NTg1KQorKysgU291cmNlL0phdmFTY3JpcHRDb3JlL3Rlc3RzL3N0 cmVzcy9vcF9yc2hpZnQuanMJKHdvcmtpbmcgY29weSkKQEAgLTEsNSArMSw0IEBACi0vL0Agc2tp cAotLy8gRklYTUU6IGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xNTE1 MTQKKy8vQCBydW5GVExOb0NKSVQKIAogLy8gSWYgYWxsIGdvZXMgd2VsbCwgdGhpcyB0ZXN0IG1v ZHVsZSB3aWxsIHRlcm1pbmF0ZSBzaWxlbnRseS4gSWYgbm90LCBpdCB3aWxsIHByaW50CiAvLyBl cnJvcnMuIFNlZSBiaW5hcnktb3AtdGVzdC5qcyBmb3IgZGVidWdnaW5nIG9wdGlvbnMgaWYgbmVl ZGVkLgpJbmRleDogU291cmNlL0phdmFTY3JpcHRDb3JlL3Rlc3RzL3N0cmVzcy9vcF91cnNoaWZ0 LmpzCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9KYXZhU2NyaXB0Q29yZS90ZXN0cy9zdHJlc3Mvb3Bf dXJzaGlmdC5qcwkocmV2aXNpb24gMTk2NTg1KQorKysgU291cmNlL0phdmFTY3JpcHRDb3JlL3Rl c3RzL3N0cmVzcy9vcF91cnNoaWZ0LmpzCSh3b3JraW5nIGNvcHkpCkBAIC0xLDUgKzEsNCBAQAot Ly9AIHNraXAKLS8vIEZJWE1FOiBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/ aWQ9MTUxNTE0CisvL0AgcnVuRlRMTm9DSklUCiAKIC8vIElmIGFsbCBnb2VzIHdlbGwsIHRoaXMg dGVzdCBtb2R1bGUgd2lsbCB0ZXJtaW5hdGUgc2lsZW50bHkuIElmIG5vdCwgaXQgd2lsbCBwcmlu dAogLy8gZXJyb3JzLiBTZWUgYmluYXJ5LW9wLXRlc3QuanMgZm9yIGRlYnVnZ2luZyBvcHRpb25z IGlmIG5lZWRlZC4K