223284 2021-03-16 15:19:32 -0700 Fixed undefined behavior bug in Const32Value::checkNegConstant(). 2021-03-16 17:11:15 -0700 1 1 1 Unclassified WebKit JavaScriptCore WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 mark.lam mark.lam darin ews-watchlist keith_miller msaboff saam tzagallo webkit-bug-importer oldest_to_newest 1740247 0 mark.lam 2021-03-16 15:19:32 -0700 This was causing a failure in testb3 on a release build. 1740253 1 423402 mark.lam 2021-03-16 15:22:42 -0700 Created attachment 423402 proposed patch. 1740263 2 423402 darin 2021-03-16 15:36:57 -0700 Comment on attachment 423402 proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=423402&action=review > Source/JavaScriptCore/b3/B3Const32Value.cpp:109 > - if (m_value == -m_value) > + // We're using volatile temps here to force Clang to actually compute these > + // bit values and compare them. Otherwise, Clang can just assume that they > + // are not equal since the result of (m_value == -m_value) is undefined > + // behavior. > + volatile uint32_t value = m_value; > + volatile uint32_t negatedValue = -m_value; > + if (value == negatedValue) If we’re having trouble because we’re depending on undefined behavior, I suggest we select a solution that does not involve undefined behavior. Using volatile seems like the wrong workaround, since it’s depending even more on undefined behavior, other types. The way we are using here is not what volatile is for. I suggest this solution, which should work reliably: if (m_value == std::numeric_limits<int32_t>::min()) return nullptr; > Source/JavaScriptCore/b3/B3Const64Value.cpp:110 > - if (m_value == -m_value) > + // We're using volatile temps here to force Clang to actually compute these > + // bit values and compare them. Otherwise, Clang can just assume that they > + // are not equal since the result of (m_value == -m_value) is undefined > + // behavior. > + volatile uint64_t value = m_value; > + volatile uint64_t negatedValue = -m_value; > + if (value == negatedValue) > return nullptr; Same: if (m_value == std::numeric_limits<int64_t>::min()) return nullptr; 1740266 3 423402 mark.lam 2021-03-16 15:42:25 -0700 Comment on attachment 423402 proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=423402&action=review >> Source/JavaScriptCore/b3/B3Const32Value.cpp:109 >> + if (value == negatedValue) > > If we’re having trouble because we’re depending on undefined behavior, I suggest we select a solution that does not involve undefined behavior. Using volatile seems like the wrong workaround, since it’s depending even more on undefined behavior, other types. The way we are using here is not what volatile is for. I suggest this solution, which should work reliably: > > if (m_value == std::numeric_limits<int32_t>::min()) > return nullptr; That's a great solution. I should have thought of that. Will give it a try. 1740285 4 423411 mark.lam 2021-03-16 15:55:56 -0700 Created attachment 423411 proposed patch. 1740291 5 423411 mark.lam 2021-03-16 16:02:32 -0700 Comment on attachment 423411 proposed patch. Thanks for the review. Landing now. 1740314 6 ews-feeder 2021-03-16 17:10:32 -0700 Committed r274538: <https://commits.webkit.org/r274538> All reviewed patches have been landed. Closing bug and clearing flags on attachment 423411. 1740315 7 webkit-bug-importer 2021-03-16 17:11:15 -0700 <rdar://problem/75502173> 423402 2021-03-16 15:22:42 -0700 2021-03-16 15:47:03 -0700 proposed patch. bug-223284.patch text/plain 3144 mark.lam SW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKPT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291 cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkocmV2aXNpb24gMjc0NTIzKQorKysgU291cmNl L0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE4IEBA CisyMDIxLTAzLTE2ICBNYXJrIExhbSAgPG1hcmsubGFtQGFwcGxlLmNvbT4KKworICAgICAgICBG aXhlZCB1bmRlZmluZWQgYmVoYXZpb3IgYnVnIGluIENvbnN0MzJWYWx1ZTo6Y2hlY2tOZWdDb25z dGFudCgpLgorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9 MjIzMjg0CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAg VGhpcyB3YXMgY2F1c2luZyBhIGZhaWx1cmUgaW4gdGVzdGIzIG9uIGEgcmVsZWFzZSBidWlsZC4K KyAgICAgICAgQWxzbyBkbyB0aGUgc2FtZSBmb3IgQ29uc3Q2NFZhbHVlOjpjaGVja05lZ0NvbnN0 YW50KCkuCisKKyAgICAgICAgKiBiMy9CM0NvbnN0MzJWYWx1ZS5jcHA6CisgICAgICAgIChKU0M6 OkIzOjpDb25zdDMyVmFsdWU6OmNoZWNrTmVnQ29uc3RhbnQgY29uc3QpOgorICAgICAgICAqIGIz L0IzQ29uc3Q2NFZhbHVlLmNwcDoKKyAgICAgICAgKEpTQzo6QjM6OkNvbnN0NjRWYWx1ZTo6Y2hl Y2tOZWdDb25zdGFudCBjb25zdCk6CisKIDIwMjEtMDMtMTYgIE1hcmsgTGFtICA8bWFyay5sYW1A YXBwbGUuY29tPgogCiAgICAgICAgIEFkZCBhIFJvb3RNYXJrUmVhc29uIHByaW50ZXIgYW5kIGFs c28gYWRkIGEgZmV3IGFkZGl0aW9uYWwgcmVhc29ucy4KSW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0 Q29yZS9iMy9CM0NvbnN0MzJWYWx1ZS5jcHAKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291cmNlL0phdmFTY3Jp cHRDb3JlL2IzL0IzQ29uc3QzMlZhbHVlLmNwcAkocmV2aXNpb24gMjY1MDc0KQorKysgU291cmNl L0phdmFTY3JpcHRDb3JlL2IzL0IzQ29uc3QzMlZhbHVlLmNwcAkod29ya2luZyBjb3B5KQpAQCAt MSw1ICsxLDUgQEAKIC8qCi0gKiBDb3B5cmlnaHQgKEMpIDIwMTUtMjAxNiBBcHBsZSBJbmMuIEFs bCByaWdodHMgcmVzZXJ2ZWQuCisgKiBDb3B5cmlnaHQgKEMpIDIwMTUtMjAyMSBBcHBsZSBJbmMu IEFsbCByaWdodHMgcmVzZXJ2ZWQuCiAgKgogICogUmVkaXN0cmlidXRpb24gYW5kIHVzZSBpbiBz b3VyY2UgYW5kIGJpbmFyeSBmb3Jtcywgd2l0aCBvciB3aXRob3V0CiAgKiBtb2RpZmljYXRpb24s IGFyZSBwZXJtaXR0ZWQgcHJvdmlkZWQgdGhhdCB0aGUgZm9sbG93aW5nIGNvbmRpdGlvbnMKQEAg LTEwMCw3ICsxMDAsMTMgQEAgVmFsdWUqIENvbnN0MzJWYWx1ZTo6Y2hlY2tNdWxDb25zdGFudChQ cgogCiBWYWx1ZSogQ29uc3QzMlZhbHVlOjpjaGVja05lZ0NvbnN0YW50KFByb2NlZHVyZSYgcHJv YykgY29uc3QKIHsKLSAgICBpZiAobV92YWx1ZSA9PSAtbV92YWx1ZSkKKyAgICAvLyBXZSdyZSB1 c2luZyB2b2xhdGlsZSB0ZW1wcyBoZXJlIHRvIGZvcmNlIENsYW5nIHRvIGFjdHVhbGx5IGNvbXB1 dGUgdGhlc2UKKyAgICAvLyBiaXQgdmFsdWVzIGFuZCBjb21wYXJlIHRoZW0uIE90aGVyd2lzZSwg Q2xhbmcgY2FuIGp1c3QgYXNzdW1lIHRoYXQgdGhleQorICAgIC8vIGFyZSBub3QgZXF1YWwgc2lu Y2UgdGhlIHJlc3VsdCBvZiAobV92YWx1ZSA9PSAtbV92YWx1ZSkgaXMgdW5kZWZpbmVkCisgICAg Ly8gYmVoYXZpb3IuCisgICAgdm9sYXRpbGUgdWludDMyX3QgdmFsdWUgPSBtX3ZhbHVlOworICAg IHZvbGF0aWxlIHVpbnQzMl90IG5lZ2F0ZWRWYWx1ZSA9IC1tX3ZhbHVlOworICAgIGlmICh2YWx1 ZSA9PSBuZWdhdGVkVmFsdWUpCiAgICAgICAgIHJldHVybiBudWxscHRyOwogICAgIHJldHVybiBu ZWdDb25zdGFudChwcm9jKTsKIH0KSW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9iMy9CM0Nv bnN0NjRWYWx1ZS5jcHAKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291cmNlL0phdmFTY3JpcHRDb3JlL2IzL0Iz Q29uc3Q2NFZhbHVlLmNwcAkocmV2aXNpb24gMjY1MDc0KQorKysgU291cmNlL0phdmFTY3JpcHRD b3JlL2IzL0IzQ29uc3Q2NFZhbHVlLmNwcAkod29ya2luZyBjb3B5KQpAQCAtMSw1ICsxLDUgQEAK IC8qCi0gKiBDb3B5cmlnaHQgKEMpIDIwMTUtMjAxNiBBcHBsZSBJbmMuIEFsbCByaWdodHMgcmVz ZXJ2ZWQuCisgKiBDb3B5cmlnaHQgKEMpIDIwMTUtMjAyMSBBcHBsZSBJbmMuIEFsbCByaWdodHMg cmVzZXJ2ZWQuCiAgKgogICogUmVkaXN0cmlidXRpb24gYW5kIHVzZSBpbiBzb3VyY2UgYW5kIGJp bmFyeSBmb3Jtcywgd2l0aCBvciB3aXRob3V0CiAgKiBtb2RpZmljYXRpb24sIGFyZSBwZXJtaXR0 ZWQgcHJvdmlkZWQgdGhhdCB0aGUgZm9sbG93aW5nIGNvbmRpdGlvbnMKQEAgLTEwMCw3ICsxMDAs MTMgQEAgVmFsdWUqIENvbnN0NjRWYWx1ZTo6Y2hlY2tNdWxDb25zdGFudChQcgogCiBWYWx1ZSog Q29uc3Q2NFZhbHVlOjpjaGVja05lZ0NvbnN0YW50KFByb2NlZHVyZSYgcHJvYykgY29uc3QKIHsK LSAgICBpZiAobV92YWx1ZSA9PSAtbV92YWx1ZSkKKyAgICAvLyBXZSdyZSB1c2luZyB2b2xhdGls ZSB0ZW1wcyBoZXJlIHRvIGZvcmNlIENsYW5nIHRvIGFjdHVhbGx5IGNvbXB1dGUgdGhlc2UKKyAg ICAvLyBiaXQgdmFsdWVzIGFuZCBjb21wYXJlIHRoZW0uIE90aGVyd2lzZSwgQ2xhbmcgY2FuIGp1 c3QgYXNzdW1lIHRoYXQgdGhleQorICAgIC8vIGFyZSBub3QgZXF1YWwgc2luY2UgdGhlIHJlc3Vs dCBvZiAobV92YWx1ZSA9PSAtbV92YWx1ZSkgaXMgdW5kZWZpbmVkCisgICAgLy8gYmVoYXZpb3Iu CisgICAgdm9sYXRpbGUgdWludDY0X3QgdmFsdWUgPSBtX3ZhbHVlOworICAgIHZvbGF0aWxlIHVp bnQ2NF90IG5lZ2F0ZWRWYWx1ZSA9IC1tX3ZhbHVlOworICAgIGlmICh2YWx1ZSA9PSBuZWdhdGVk VmFsdWUpCiAgICAgICAgIHJldHVybiBudWxscHRyOwogICAgIHJldHVybiBuZWdDb25zdGFudChw cm9jKTsKIH0K 423411 2021-03-16 15:55:56 -0700 2021-03-16 17:10:33 -0700 proposed patch. bug-223284.patch text/plain 2504 mark.lam SW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKPT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291 cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkocmV2aXNpb24gMjc0NTI2KQorKysgU291cmNl L0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE4IEBA CisyMDIxLTAzLTE2ICBNYXJrIExhbSAgPG1hcmsubGFtQGFwcGxlLmNvbT4KKworICAgICAgICBG aXhlZCB1bmRlZmluZWQgYmVoYXZpb3IgYnVnIGluIENvbnN0MzJWYWx1ZTo6Y2hlY2tOZWdDb25z dGFudCgpLgorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9 MjIzMjg0CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAg VGhpcyB3YXMgY2F1c2luZyBhIGZhaWx1cmUgaW4gdGVzdGIzIG9uIGEgcmVsZWFzZSBidWlsZC4K KyAgICAgICAgQWxzbyBkbyB0aGUgc2FtZSBmb3IgQ29uc3Q2NFZhbHVlOjpjaGVja05lZ0NvbnN0 YW50KCkuCisKKyAgICAgICAgKiBiMy9CM0NvbnN0MzJWYWx1ZS5jcHA6CisgICAgICAgIChKU0M6 OkIzOjpDb25zdDMyVmFsdWU6OmNoZWNrTmVnQ29uc3RhbnQgY29uc3QpOgorICAgICAgICAqIGIz L0IzQ29uc3Q2NFZhbHVlLmNwcDoKKyAgICAgICAgKEpTQzo6QjM6OkNvbnN0NjRWYWx1ZTo6Y2hl Y2tOZWdDb25zdGFudCBjb25zdCk6CisKIDIwMjEtMDMtMTYgIE1hcmsgTGFtICA8bWFyay5sYW1A YXBwbGUuY29tPgogCiAgICAgICAgIEFkZCBhIFJvb3RNYXJrUmVhc29uIHByaW50ZXIgYW5kIGFs c28gYWRkIGEgZmV3IGFkZGl0aW9uYWwgcmVhc29ucy4KSW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0 Q29yZS9iMy9CM0NvbnN0MzJWYWx1ZS5jcHAKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291cmNlL0phdmFTY3Jp cHRDb3JlL2IzL0IzQ29uc3QzMlZhbHVlLmNwcAkocmV2aXNpb24gMjc0NTI2KQorKysgU291cmNl L0phdmFTY3JpcHRDb3JlL2IzL0IzQ29uc3QzMlZhbHVlLmNwcAkod29ya2luZyBjb3B5KQpAQCAt MSw1ICsxLDUgQEAKIC8qCi0gKiBDb3B5cmlnaHQgKEMpIDIwMTUtMjAxNiBBcHBsZSBJbmMuIEFs bCByaWdodHMgcmVzZXJ2ZWQuCisgKiBDb3B5cmlnaHQgKEMpIDIwMTUtMjAyMSBBcHBsZSBJbmMu IEFsbCByaWdodHMgcmVzZXJ2ZWQuCiAgKgogICogUmVkaXN0cmlidXRpb24gYW5kIHVzZSBpbiBz b3VyY2UgYW5kIGJpbmFyeSBmb3Jtcywgd2l0aCBvciB3aXRob3V0CiAgKiBtb2RpZmljYXRpb24s IGFyZSBwZXJtaXR0ZWQgcHJvdmlkZWQgdGhhdCB0aGUgZm9sbG93aW5nIGNvbmRpdGlvbnMKQEAg LTEwMCw3ICsxMDAsNyBAQCBWYWx1ZSogQ29uc3QzMlZhbHVlOjpjaGVja011bENvbnN0YW50KFBy CiAKIFZhbHVlKiBDb25zdDMyVmFsdWU6OmNoZWNrTmVnQ29uc3RhbnQoUHJvY2VkdXJlJiBwcm9j KSBjb25zdAogewotICAgIGlmIChtX3ZhbHVlID09IC1tX3ZhbHVlKQorICAgIGlmIChtX3ZhbHVl ID09IHN0ZDo6bnVtZXJpY19saW1pdHM8aW50MzJfdD46Om1pbigpKQogICAgICAgICByZXR1cm4g bnVsbHB0cjsKICAgICByZXR1cm4gbmVnQ29uc3RhbnQocHJvYyk7CiB9CkluZGV4OiBTb3VyY2Uv SmF2YVNjcmlwdENvcmUvYjMvQjNDb25zdDY0VmFsdWUuY3BwCj09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJj ZS9KYXZhU2NyaXB0Q29yZS9iMy9CM0NvbnN0NjRWYWx1ZS5jcHAJKHJldmlzaW9uIDI3NDUyNikK KysrIFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9iMy9CM0NvbnN0NjRWYWx1ZS5jcHAJKHdvcmtpbmcg Y29weSkKQEAgLTEsNSArMSw1IEBACiAvKgotICogQ29weXJpZ2h0IChDKSAyMDE1LTIwMTYgQXBw bGUgSW5jLiBBbGwgcmlnaHRzIHJlc2VydmVkLgorICogQ29weXJpZ2h0IChDKSAyMDE1LTIwMjEg QXBwbGUgSW5jLiBBbGwgcmlnaHRzIHJlc2VydmVkLgogICoKICAqIFJlZGlzdHJpYnV0aW9uIGFu ZCB1c2UgaW4gc291cmNlIGFuZCBiaW5hcnkgZm9ybXMsIHdpdGggb3Igd2l0aG91dAogICogbW9k aWZpY2F0aW9uLCBhcmUgcGVybWl0dGVkIHByb3ZpZGVkIHRoYXQgdGhlIGZvbGxvd2luZyBjb25k aXRpb25zCkBAIC0xMDAsNyArMTAwLDcgQEAgVmFsdWUqIENvbnN0NjRWYWx1ZTo6Y2hlY2tNdWxD b25zdGFudChQcgogCiBWYWx1ZSogQ29uc3Q2NFZhbHVlOjpjaGVja05lZ0NvbnN0YW50KFByb2Nl ZHVyZSYgcHJvYykgY29uc3QKIHsKLSAgICBpZiAobV92YWx1ZSA9PSAtbV92YWx1ZSkKKyAgICBp ZiAobV92YWx1ZSA9PSBzdGQ6Om51bWVyaWNfbGltaXRzPGludDY0X3Q+OjptaW4oKSkKICAgICAg ICAgcmV0dXJuIG51bGxwdHI7CiAgICAgcmV0dXJuIG5lZ0NvbnN0YW50KHByb2MpOwogfQo=