175772 2017-08-21 09:17:34 -0700 [Win][Release] Crash when running testmasm executable. 2017-08-22 08:51:32 -0700 1 1 1 Unclassified WebKit JavaScriptCore WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 pvollan pvollan achristensen bfulgham buildbot commit-queue keith_miller mark.lam msaboff saam webkit-bug-importer oldest_to_newest 1340612 0 pvollan 2017-08-21 09:17:34 -0700 I believe we need to save and restore the modified registers in case one or more registers are callee saved on the relevant platforms. 1340616 1 318637 pvollan 2017-08-21 09:19:51 -0700 Created attachment 318637 Patch 1340762 2 318637 achristensen 2017-08-21 14:40:08 -0700 Comment on attachment 318637 Patch Let's definitely just do this for win64, which has this as a requirement in its calling convention, and not for every platform. 1340959 3 318637 mark.lam 2017-08-21 22:19:13 -0700 Comment on attachment 318637 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=318637&action=review r=me > Source/JavaScriptCore/ChangeLog:9 > + We need to save and restore the modified registers in case one or more registers are callee saved > + on the relevant platforms. When I wrote the tests, I chose argument registers on purpose because I thought they would not be callee saved registers. Of course, I forgot that 32-bit x86 does not actually pass arguments in registers, and it turns out that argumentGPR3 is ebx, which is a callee saved register. I think it doesn't hurt to do what you're doing with pushing and popping the argument registers for all platforms. It makes the test agnostic of whether the target platform treats any of the registers as callee saved or not, and therefore, is more robust in a self-contained way. 1340960 4 mark.lam 2017-08-21 22:21:07 -0700 (In reply to Alex Christensen from comment #2) > Comment on attachment 318637 [details] > Patch > > Let's definitely just do this for win64, which has this as a requirement in > its calling convention, and not for every platform. Actually, this only needed for win32. However, I think it doesn't hurt to make the test more robust and agnostic of platform details as to whether those registers are callee saved or not. The test really doesn't care about that detail. It only wants to set some register values for test. So, I think we should go forward with this fix. 1341054 5 pvollan 2017-08-22 08:22:22 -0700 (In reply to Mark Lam from comment #4) > (In reply to Alex Christensen from comment #2) > > Comment on attachment 318637 [details] > > Patch > > > > Let's definitely just do this for win64, which has this as a requirement in > > its calling convention, and not for every platform. > > Actually, this only needed for win32. However, I think it doesn't hurt to > make the test more robust and agnostic of platform details as to whether > those registers are callee saved or not. The test really doesn't care about > that detail. It only wants to set some register values for test. So, I > think we should go forward with this fix. Thanks for reviewing, all! 1341061 6 318637 commit-queue 2017-08-22 08:50:54 -0700 Comment on attachment 318637 Patch Clearing flags on attachment: 318637 Committed r221012: <http://trac.webkit.org/changeset/221012> 1341062 7 commit-queue 2017-08-22 08:50:56 -0700 All reviewed patches have been landed. Closing bug. 1341063 8 webkit-bug-importer 2017-08-22 08:51:32 -0700 <rdar://problem/34013544> 318637 2017-08-21 09:19:51 -0700 2017-08-22 08:50:54 -0700 Patch bug-175772-20170821181950.patch text/plain 2910 pvollan SW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKPT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291 cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkocmV2aXNpb24gMjIwOTY1KQorKysgU291cmNl L0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE3IEBA CisyMDE3LTA4LTIxICBQZXIgQXJuZSBWb2xsYW4gIDxwdm9sbGFuQGFwcGxlLmNvbT4KKworICAg ICAgICBbV2luXVtSZWxlYXNlXSBDcmFzaCB3aGVuIHJ1bm5pbmcgdGVzdG1hc20gZXhlY3V0YWJs ZS4KKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTE3NTc3 MgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIFdlIG5l ZWQgdG8gc2F2ZSBhbmQgcmVzdG9yZSB0aGUgbW9kaWZpZWQgcmVnaXN0ZXJzIGluIGNhc2Ugb25l IG9yIG1vcmUgcmVnaXN0ZXJzIGFyZSBjYWxsZWUgc2F2ZWQKKyAgICAgICAgb24gdGhlIHJlbGV2 YW50IHBsYXRmb3Jtcy4KKworICAgICAgICAqIGFzc2VtYmxlci90ZXN0bWFzbS5jcHA6CisgICAg ICAgIChKU0M6OnRlc3RQcm9iZVJlYWRzQXJndW1lbnRSZWdpc3RlcnMpOgorICAgICAgICAoSlND Ojp0ZXN0UHJvYmVXcml0ZXNBcmd1bWVudFJlZ2lzdGVycyk6CisKIDIwMTctMDgtMjAgIE1hcmsg TGFtICA8bWFyay5sYW1AYXBwbGUuY29tPgogCiAgICAgICAgIEdhcmRlbmluZzogZml4IENMb29w IGJ1aWxkLgpJbmRleDogU291cmNlL0phdmFTY3JpcHRDb3JlL2Fzc2VtYmxlci90ZXN0bWFzbS5j cHAKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PQotLS0gU291cmNlL0phdmFTY3JpcHRDb3JlL2Fzc2VtYmxlci90ZXN0bWFz bS5jcHAJKHJldmlzaW9uIDIyMDkzMikKKysrIFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9hc3NlbWJs ZXIvdGVzdG1hc20uY3BwCSh3b3JraW5nIGNvcHkpCkBAIC0xNjgsNiArMTY4LDExIEBAIHZvaWQg dGVzdFByb2JlUmVhZHNBcmd1bWVudFJlZ2lzdGVycygpCiAgICAgY29tcGlsZUFuZFJ1bjx2b2lk PihbJl0gKENDYWxsSGVscGVycyYgaml0KSB7CiAgICAgICAgIGppdC5lbWl0RnVuY3Rpb25Qcm9s b2d1ZSgpOwogCisgICAgICAgIGppdC5wdXNoKEdQUkluZm86OmFyZ3VtZW50R1BSMCk7CisgICAg ICAgIGppdC5wdXNoKEdQUkluZm86OmFyZ3VtZW50R1BSMSk7CisgICAgICAgIGppdC5wdXNoKEdQ UkluZm86OmFyZ3VtZW50R1BSMik7CisgICAgICAgIGppdC5wdXNoKEdQUkluZm86OmFyZ3VtZW50 R1BSMyk7CisKICAgICAgICAgaml0Lm1vdmUoQ0NhbGxIZWxwZXJzOjpUcnVzdGVkSW1tMzIodGVz dFdvcmQzMigwKSksIEdQUkluZm86OmFyZ3VtZW50R1BSMCk7CiAgICAgICAgIGppdC5jb252ZXJ0 SW50MzJUb0RvdWJsZShHUFJJbmZvOjphcmd1bWVudEdQUjAsIEZQUkluZm86OmZwUmVnVDApOwog ICAgICAgICBqaXQubW92ZShDQ2FsbEhlbHBlcnM6OlRydXN0ZWRJbW0zMih0ZXN0V29yZDMyKDEp KSwgR1BSSW5mbzo6YXJndW1lbnRHUFIwKTsKQEAgLTE5NSw2ICsyMDAsMTIgQEAgdm9pZCB0ZXN0 UHJvYmVSZWFkc0FyZ3VtZW50UmVnaXN0ZXJzKCkKICAgICAgICAgICAgIENIRUNLX0VRKGNwdS5m cHIoRlBSSW5mbzo6ZnBSZWdUMCksIHRlc3RXb3JkMzIoMCkpOwogICAgICAgICAgICAgQ0hFQ0tf RVEoY3B1LmZwcihGUFJJbmZvOjpmcFJlZ1QxKSwgdGVzdFdvcmQzMigxKSk7CiAgICAgICAgIH0p OworCisgICAgICAgIGppdC5wb3AoR1BSSW5mbzo6YXJndW1lbnRHUFIzKTsKKyAgICAgICAgaml0 LnBvcChHUFJJbmZvOjphcmd1bWVudEdQUjIpOworICAgICAgICBqaXQucG9wKEdQUkluZm86OmFy Z3VtZW50R1BSMSk7CisgICAgICAgIGppdC5wb3AoR1BSSW5mbzo6YXJndW1lbnRHUFIwKTsKKwog ICAgICAgICBqaXQuZW1pdEZ1bmN0aW9uRXBpbG9ndWUoKTsKICAgICAgICAgaml0LnJldCgpOwog ICAgIH0pOwpAQCAtMjEwLDYgKzIyMSwxMSBAQCB2b2lkIHRlc3RQcm9iZVdyaXRlc0FyZ3VtZW50 UmVnaXN0ZXJzKCkKICAgICBjb21waWxlQW5kUnVuPHZvaWQ+KFsmXSAoQ0NhbGxIZWxwZXJzJiBq aXQpIHsKICAgICAgICAgaml0LmVtaXRGdW5jdGlvblByb2xvZ3VlKCk7CiAKKyAgICAgICAgaml0 LnB1c2goR1BSSW5mbzo6YXJndW1lbnRHUFIwKTsKKyAgICAgICAgaml0LnB1c2goR1BSSW5mbzo6 YXJndW1lbnRHUFIxKTsKKyAgICAgICAgaml0LnB1c2goR1BSSW5mbzo6YXJndW1lbnRHUFIyKTsK KyAgICAgICAgaml0LnB1c2goR1BSSW5mbzo6YXJndW1lbnRHUFIzKTsKKwogICAgICAgICAvLyBQ cmUtaW5pdGlhbGl6ZSB3aXRoIG5vbi1leHBlY3RlZCB2YWx1ZXMuCiAjaWYgVVNFKEpTVkFMVUU2 NCkKICAgICAgICAgaml0Lm1vdmUoQ0NhbGxIZWxwZXJzOjpUcnVzdGVkSW1tNjQoMCksIEdQUklu Zm86OmFyZ3VtZW50R1BSMCk7CkBAIC0yNTEsNiArMjY3LDExIEBAIHZvaWQgdGVzdFByb2JlV3Jp dGVzQXJndW1lbnRSZWdpc3RlcnMoKQogICAgICAgICAgICAgQ0hFQ0tfRVEoY3B1LmZwcjx1aW50 NjRfdD4oRlBSSW5mbzo6ZnBSZWdUMSksIHRlc3RXb3JkNjQoMSkpOwogICAgICAgICB9KTsKIAor ICAgICAgICBqaXQucG9wKEdQUkluZm86OmFyZ3VtZW50R1BSMyk7CisgICAgICAgIGppdC5wb3Ao R1BSSW5mbzo6YXJndW1lbnRHUFIyKTsKKyAgICAgICAgaml0LnBvcChHUFJJbmZvOjphcmd1bWVu dEdQUjEpOworICAgICAgICBqaXQucG9wKEdQUkluZm86OmFyZ3VtZW50R1BSMCk7CisKICAgICAg ICAgaml0LmVtaXRGdW5jdGlvbkVwaWxvZ3VlKCk7CiAgICAgICAgIGppdC5yZXQoKTsKICAgICB9 KTsK