211783 2020-05-12 09:22:20 -0700 -Wsign-compare warnings in FTLLowerDFGToB3.cpp and DFGSpeculativeJIT.cpp 2020-05-12 14:04:15 -0700 1 1 1 Unclassified WebKit JavaScriptCore WebKit Nightly Build PC Linux RESOLVED FIXED https://bugs.webkit.org/show_bug.cgi?id=206182 InRadar P2 Normal --- 1 mcatanzaro mcatanzaro darin ews-watchlist keith_miller mark.lam mcatanzaro msaboff saam tzagallo webkit-bug-importer oldest_to_newest 1651762 0 mcatanzaro 2020-05-12 09:22:20 -0700 This fixes -Wsign-compare warnings introduced in bug #206182. See https://bugs.webkit.org/show_bug.cgi?id=206182#c47. Note that I'm only submitting a *simple* solution. JSValue::BigInt32Mask is currently a negative-valued signed integer, and I'm skeptical that that's desirable, since unsigned is usually better to represent bit patterns. I'm not volunteering to try changing that, though. 1651765 1 399134 mcatanzaro 2020-05-12 09:25:35 -0700 Created attachment 399134 Patch 1651768 2 399134 mark.lam 2020-05-12 09:31:04 -0700 Comment on attachment 399134 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=399134&action=review > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3600 > + static_assert(JSValue::BigInt32Mask == static_cast<int64_t>(0xfffe000000000012)); Would using a "ll" suffix suffice? 0xfffe000000000012ll > Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:3346 > + static_assert(JSValue::BigInt32Mask == static_cast<int64_t>(0xfffe000000000012)); Ditto. 1651800 3 399134 darin 2020-05-12 10:36:57 -0700 Comment on attachment 399134 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=399134&action=review r=me because I don’t see a better solution >> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3600 >> + static_assert(JSValue::BigInt32Mask == static_cast<int64_t>(0xfffe000000000012)); > > Would using a "ll" suffix suffice? 0xfffe000000000012ll I think that won’t work because the value doesn’t fit in a 64-bit long long, and so the language makes it unsigned long long in that case. I consulted https://en.cppreference.com/w/cpp/language/integer_literal to explore this. 1651801 4 ews-feeder 2020-05-12 10:40:25 -0700 Committed r261564: <https://trac.webkit.org/changeset/261564> All reviewed patches have been landed. Closing bug and clearing flags on attachment 399134. 1651802 5 webkit-bug-importer 2020-05-12 10:41:15 -0700 <rdar://problem/63143618> 1651924 6 mcatanzaro 2020-05-12 14:04:15 -0700 (In reply to Darin Adler from comment #3) > I think that won’t work because the value doesn’t fit in a 64-bit long long, > and so the language makes it unsigned long long in that case. I consulted > https://en.cppreference.com/w/cpp/language/integer_literal to explore this. Exactly. I had consulted the same page to figure out how integer literals work. Learning is good! 399134 2020-05-12 09:25:35 -0700 2020-05-12 10:40:26 -0700 Patch bug-211783-20200512112534.patch text/plain 2817 mcatanzaro U3VidmVyc2lvbiBSZXZpc2lvbjogMjYxNDU2CmRpZmYgLS1naXQgYS9Tb3VyY2UvSmF2YVNjcmlw dENvcmUvQ2hhbmdlTG9nIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwppbmRleCBl MTI5MjRhZTgzNDkzMTEyYTBjOGUxZGQ1Yzk5MjQ5Mzc3NWE3YTAzLi4yNzc0NTVhYjc4ZjBlNDFh ZWZmMTM1YmRiNDY4MzBjZDhlZGY1NTZlIDEwMDY0NAotLS0gYS9Tb3VyY2UvSmF2YVNjcmlwdENv cmUvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKQEAgLTEs MyArMSwxNyBAQAorMjAyMC0wNS0xMiAgTWljaGFlbCBDYXRhbnphcm8gIDxtY2F0YW56YXJvQGdu b21lLm9yZz4KKworICAgICAgICAtV3NpZ24tY29tcGFyZSB3YXJuaW5ncyBpbiBGVExMb3dlckRG R1RvQjMuY3BwIGFuZCBERkdTcGVjdWxhdGl2ZUpJVC5jcHAKKyAgICAgICAgaHR0cHM6Ly9idWdz LndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTIxMTc4MworCisgICAgICAgIFJldmlld2VkIGJ5 IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIFRoaXMgZml4ZXMgLVdzaWduLWNvbXBhcmUgd2Fy bmluZ3MgaW50cm9kdWNlZCBpbiByMjYwMzMxLgorCisgICAgICAgICogZGZnL0RGR1NwZWN1bGF0 aXZlSklULmNwcDoKKyAgICAgICAgKEpTQzo6REZHOjpTcGVjdWxhdGl2ZUpJVDo6Y29tcGlsZVZh bHVlQml0Tm90KToKKyAgICAgICAgKiBmdGwvRlRMTG93ZXJERkdUb0IzLmNwcDoKKyAgICAgICAg KEpTQzo6RlRMOjpERkc6Okxvd2VyREZHVG9CMzo6Y29tcGlsZVZhbHVlQml0Tm90KToKKwogMjAy MC0wNS0wOSAgUm9zcyBLaXJzbGluZyAgPHJvc3Mua2lyc2xpbmdAc29ueS5jb20+CiAKICAgICAg ICAgRml4IGJ1aWxkIGVycm9ycyBhbmQgd2FybmluZ3MgZm9yIG5vbi11bmlmaWVkIEpTQ09ubHkK ZGlmZiAtLWdpdCBhL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9kZmcvREZHU3BlY3VsYXRpdmVKSVQu Y3BwIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL2RmZy9ERkdTcGVjdWxhdGl2ZUpJVC5jcHAKaW5k ZXggZmEyOGYzOTMwZDRkODJlZTY4Y2QwMjg3MGZjMTY4N2QwMmMyNTkyYy4uYzQ2ZTg5NGQxNTdk ZWJmOGMyN2E3NjJjOWEwYzMxM2M1ZTFjODQzOCAxMDA2NDQKLS0tIGEvU291cmNlL0phdmFTY3Jp cHRDb3JlL2RmZy9ERkdTcGVjdWxhdGl2ZUpJVC5jcHAKKysrIGIvU291cmNlL0phdmFTY3JpcHRD b3JlL2RmZy9ERkdTcGVjdWxhdGl2ZUpJVC5jcHAKQEAgLTM1OTcsNyArMzU5Nyw3IEBAIHZvaWQg U3BlY3VsYXRpdmVKSVQ6OmNvbXBpbGVWYWx1ZUJpdE5vdChOb2RlKiBub2RlKQogCiAgICAgICAg IC8vIFRoZSBmb2xsb3dpbmcgdHJpY2sgcmVsaWVzIG9uIGRldGFpbHMgb2YgdGhlIHJlcHJlc2Vu dGF0aW9uIG9mIEJpZ0ludDMyLCBhbmQgd2lsbCBoYXZlIHRvIGJlIHVwZGF0ZWQgaWYgd2UgbW92 ZSBiaXRzIGFyb3VuZC4KICAgICAgICAgc3RhdGljX2Fzc2VydChKU1ZhbHVlOjpCaWdJbnQzMlRh ZyA9PSAweDEyKTsKLSAgICAgICAgc3RhdGljX2Fzc2VydChKU1ZhbHVlOjpCaWdJbnQzMk1hc2sg PT0gMHhmZmZlMDAwMDAwMDAwMDEyKTsKKyAgICAgICAgc3RhdGljX2Fzc2VydChKU1ZhbHVlOjpC aWdJbnQzMk1hc2sgPT0gc3RhdGljX2Nhc3Q8aW50NjRfdD4oMHhmZmZlMDAwMDAwMDAwMDEyKSk7 CiAgICAgICAgIGNvbnN0ZXhwciB1aW50NjRfdCBtYXNrRm9yQmlnSW50MzJCaXRzID0gMHgwMDAw ZmZmZmZmZmYwMDAwOwogICAgICAgICBzdGF0aWNfYXNzZXJ0KCEoSlNWYWx1ZTo6QmlnSW50MzJN YXNrICYgbWFza0ZvckJpZ0ludDMyQml0cykpOwogICAgICAgICBtX2ppdC5tb3ZlKFRydXN0ZWRJ bW02NChtYXNrRm9yQmlnSW50MzJCaXRzKSwgcmVzdWx0R1BSKTsKZGlmZiAtLWdpdCBhL1NvdXJj ZS9KYXZhU2NyaXB0Q29yZS9mdGwvRlRMTG93ZXJERkdUb0IzLmNwcCBiL1NvdXJjZS9KYXZhU2Ny aXB0Q29yZS9mdGwvRlRMTG93ZXJERkdUb0IzLmNwcAppbmRleCA1MmE2OTlmZmM5Y2U2YjgzYWIw NGZjYmQzMDc0M2YwNDY0Y2E0MjM0Li4xNjk4ODU2NzJjMTRjMWU1ZWE2YzZlZGIwNDdlN2E5MGVm ZTAyOGM2IDEwMDY0NAotLS0gYS9Tb3VyY2UvSmF2YVNjcmlwdENvcmUvZnRsL0ZUTExvd2VyREZH VG9CMy5jcHAKKysrIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL2Z0bC9GVExMb3dlckRGR1RvQjMu Y3BwCkBAIC0zMzQzLDcgKzMzNDMsNyBAQCBwcml2YXRlOgogICAgICAgICAgICAgTFZhbHVlIG9w ZXJhbmQgPSBsb3dCaWdJbnQzMihtX25vZGUtPmNoaWxkMSgpKTsKICAgICAgICAgICAgIC8vIFRo ZSBmb2xsb3dpbmcgdHJpY2sgcmVsaWVzIG9uIGRldGFpbHMgb2YgdGhlIHJlcHJlc2VudGF0aW9u IG9mIEJpZ0ludDMyLCBhbmQgd2lsbCBoYXZlIHRvIGJlIHVwZGF0ZWQgaWYgd2UgbW92ZSBiaXRz IGFyb3VuZC4KICAgICAgICAgICAgIHN0YXRpY19hc3NlcnQoSlNWYWx1ZTo6QmlnSW50MzJUYWcg PT0gMHgxMik7Ci0gICAgICAgICAgICBzdGF0aWNfYXNzZXJ0KEpTVmFsdWU6OkJpZ0ludDMyTWFz ayA9PSAweGZmZmUwMDAwMDAwMDAwMTIpOworICAgICAgICAgICAgc3RhdGljX2Fzc2VydChKU1Zh bHVlOjpCaWdJbnQzMk1hc2sgPT0gc3RhdGljX2Nhc3Q8aW50NjRfdD4oMHhmZmZlMDAwMDAwMDAw MDEyKSk7CiAgICAgICAgICAgICB1aW50NjRfdCBtYXNrRm9yQmlnSW50MzJCaXRzID0gMHgwMDAw ZmZmZmZmZmYwMDAwOwogICAgICAgICAgICAgTFZhbHVlIHJlc3VsdCA9IG1fb3V0LmJpdFhvcihv cGVyYW5kLCBtX291dC5jb25zdEludDY0KG1hc2tGb3JCaWdJbnQzMkJpdHMpKTsKICAgICAgICAg ICAgIHNldEpTVmFsdWUocmVzdWx0KTsK