131146 2014-04-02 21:00:38 -0700 ARMv7 compare32() should not use TST to do CMP's job 2014-04-03 07:39:15 -0700 1 1 1 Unclassified WebKit JavaScriptCore 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 108645 1 mark.lam mark.lam barraclough fpizlo ggaren mhahnenberg mmirman msaboff oliver ossy oldest_to_newest 996973 0 mark.lam 2014-04-02 21:00:38 -0700 The ARMv7 implementation of "compare32(RegisterID left, TrustedImm32 right)" was being cute and was using "tst reg, reg" to implement "cmp reg, #0". Unfortunately, the tst instruction doesn't set the Overflow (V) flag and this results in random results depending on whether there was a preceeding instruction that did set the Overflow (V) flag. This results in seemingly random manifestations of wrongful behavior that are hard to diagnose. Here's the specific instruction stream I was anaylzing: // initial values: // r3 = 0xa1a1a1a1 // r4 = 0x786bcc7a // r10 = 0xa1a1a1a1 0x66d1b5bc: mov r0, r4 // r0 = 0x786bcc7a 0x66d1b5be: subs r0, r0, r3 // r0 = 0x786bcc7a - 0xa1a1a1a1 = 0xd6ca2ad9, and sets the V flag 0x66d1b5c0: mov r2, r10 // r2 = 0xa1a1a1a1 // Check if r2 < 0: 0x66d1b5c2: tst r2, r2 // Sets the N flag and leaves the V flag 0x66d1b5c4: blt 0x66d1b7dc // "lt" checks if "N != V". Hence, no branch because both N and V are set. 0x66d1b5c8: ldr r9, [r7, #-8] 0x66d1b5cc: mov r8, r9 // r8 = 0x12015037 // Check if r8 < 0: 0x66d1b5ce: tst r8, r8 // Clears the N flag but leaves the V flag set. 0x66d1b5d2: blt 0x66d1b7fe // "lt" checks if "N != V". N is 0. V is 1. Hence, we branch. That second blt is where I see the branch taken when I don't expect it to. The value in r8 (0x12015037) is obviously not less than 0, and the branch should not be taken. The first blt is also wrong. It didn't take the branch when it should have. The value in r2 (0xa1a1a1a1) is obviously less than 0, and the branch should have been taken. The fix is to use "cmp reg, #0" instead of "tst reg, reg" when we actually want to compare to 0. 996974 1 mark.lam 2014-04-02 21:01:11 -0700 <rdar://problem/16064304> 996978 2 228459 mark.lam 2014-04-02 21:09:26 -0700 Created attachment 228459 the patch. 997014 3 228459 ggaren 2014-04-02 23:06:17 -0700 Comment on attachment 228459 the patch. r=me You should mention the test that covers this in your ChangeLog. 997167 4 mark.lam 2014-04-03 07:39:15 -0700 Thanks for the review. Landed in r166716: <http://trac.webkit.org/r166716>. 228459 2014-04-02 21:09:26 -0700 2014-04-02 23:06:16 -0700 the patch. bug-131146.patch text/plain 2397 mark.lam SW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKPT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291 cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkocmV2aXNpb24gMTY2NjkxKQorKysgU291cmNl L0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDIxIEBA CisyMDE0LTA0LTAyICBNYXJrIExhbSAgPG1hcmsubGFtQGFwcGxlLmNvbT4KKworICAgICAgICBB Uk12NyBjb21wYXJlMzIoKSBzaG91bGQgbm90IHVzZSBUU1QgdG8gZG8gQ01QJ3Mgam9iLgorICAg ICAgICA8aHR0cHM6Ly93ZWJraXQub3JnL2IvMTMxMTQ2PgorCisgICAgICAgIFJldmlld2VkIGJ5 IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIFRoZSBBUk12NyBpbXBsZW1lbnRhdGlvbiBvZiAi Y29tcGFyZTMyKFJlZ2lzdGVySUQgbGVmdCwgVHJ1c3RlZEltbTMyIHJpZ2h0KSIKKyAgICAgICAg d2FzIHVzaW5nICJ0c3QgcmVnLCByZWciIHRvIGltcGxlbWVudCAiY21wIHJlZywgIzAiLiAgVW5m b3J0dW5hdGVseSwgdGhlIHRzdAorICAgICAgICBpbnN0cnVjdGlvbiBkb2Vzbid0IHNldCB0aGUg T3ZlcmZsb3cgKFYpIGZsYWcgYW5kIHRoaXMgcmVzdWx0cyBpbiByYW5kb20KKyAgICAgICAgcmVz dWx0cyBkZXBlbmRpbmcgb24gd2hldGhlciB0aGVyZSB3YXMgYSBwcmVjZWVkaW5nIGluc3RydWN0 aW9uIHRoYXQgZGlkIHNldAorICAgICAgICB0aGUgT3ZlcmZsb3cgKFYpIGZsYWcuCisKKyAgICAg ICAgVGhlIGZpeCBpcyB0byB1c2UgImNtcCByZWcsICMwIiB0byBkbyB0aGUgam9iIHByb3Blcmx5 LgorCisgICAgICAgICogYXNzZW1ibGVyL01hY3JvQXNzZW1ibGVyQVJNdjcuaDoKKyAgICAgICAg KEpTQzo6TWFjcm9Bc3NlbWJsZXJBUk12Nzo6Y29tcGFyZTMyKToKKwogMjAxNC0wNC0wMiAgTWFy ayBIYWhuZW5iZXJnICA8bWhhaG5lbmJlcmdAYXBwbGUuY29tPgogCiAgICAgICAgIENvZGVCbG9j a1NldCBzaG91bGQgYmUgZ2VuZXJhdGlvbmFsCkluZGV4OiBTb3VyY2UvSmF2YVNjcmlwdENvcmUv YXNzZW1ibGVyL01hY3JvQXNzZW1ibGVyQVJNdjcuaAo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvSmF2 YVNjcmlwdENvcmUvYXNzZW1ibGVyL01hY3JvQXNzZW1ibGVyQVJNdjcuaAkocmV2aXNpb24gMTY2 NjkxKQorKysgU291cmNlL0phdmFTY3JpcHRDb3JlL2Fzc2VtYmxlci9NYWNyb0Fzc2VtYmxlckFS TXY3LmgJKHdvcmtpbmcgY29weSkKQEAgLTEyODcsMTggKzEyODcsMTQgQEAgcHJpdmF0ZToKICAg ICB2b2lkIGNvbXBhcmUzMihSZWdpc3RlcklEIGxlZnQsIFRydXN0ZWRJbW0zMiByaWdodCkKICAg ICB7CiAgICAgICAgIGludDMyX3QgaW1tID0gcmlnaHQubV92YWx1ZTsKLSAgICAgICAgaWYgKCFp bW0pCi0gICAgICAgICAgICBtX2Fzc2VtYmxlci50c3QobGVmdCwgbGVmdCk7CisgICAgICAgIEFS TVRodW1iSW1tZWRpYXRlIGFybUltbSA9IEFSTVRodW1iSW1tZWRpYXRlOjptYWtlRW5jb2RlZElt bShpbW0pOworICAgICAgICBpZiAoYXJtSW1tLmlzVmFsaWQoKSkKKyAgICAgICAgICAgIG1fYXNz ZW1ibGVyLmNtcChsZWZ0LCBhcm1JbW0pOworICAgICAgICBlbHNlIGlmICgoYXJtSW1tID0gQVJN VGh1bWJJbW1lZGlhdGU6Om1ha2VFbmNvZGVkSW1tKC1pbW0pKS5pc1ZhbGlkKCkpCisgICAgICAg ICAgICBtX2Fzc2VtYmxlci5jbW4obGVmdCwgYXJtSW1tKTsKICAgICAgICAgZWxzZSB7Ci0gICAg ICAgICAgICBBUk1UaHVtYkltbWVkaWF0ZSBhcm1JbW0gPSBBUk1UaHVtYkltbWVkaWF0ZTo6bWFr ZUVuY29kZWRJbW0oaW1tKTsKLSAgICAgICAgICAgIGlmIChhcm1JbW0uaXNWYWxpZCgpKQotICAg ICAgICAgICAgICAgIG1fYXNzZW1ibGVyLmNtcChsZWZ0LCBhcm1JbW0pOwotICAgICAgICAgICAg ZWxzZSBpZiAoKGFybUltbSA9IEFSTVRodW1iSW1tZWRpYXRlOjptYWtlRW5jb2RlZEltbSgtaW1t KSkuaXNWYWxpZCgpKQotICAgICAgICAgICAgICAgIG1fYXNzZW1ibGVyLmNtbihsZWZ0LCBhcm1J bW0pOwotICAgICAgICAgICAgZWxzZSB7Ci0gICAgICAgICAgICAgICAgbW92ZShUcnVzdGVkSW1t MzIoaW1tKSwgZGF0YVRlbXBSZWdpc3Rlcik7Ci0gICAgICAgICAgICAgICAgbV9hc3NlbWJsZXIu Y21wKGxlZnQsIGRhdGFUZW1wUmVnaXN0ZXIpOwotICAgICAgICAgICAgfQorICAgICAgICAgICAg bW92ZShUcnVzdGVkSW1tMzIoaW1tKSwgZGF0YVRlbXBSZWdpc3Rlcik7CisgICAgICAgICAgICBt X2Fzc2VtYmxlci5jbXAobGVmdCwgZGF0YVRlbXBSZWdpc3Rlcik7CiAgICAgICAgIH0KICAgICB9 CiAK