178543 2017-10-19 14:10:36 -0700 REGRESSION(r223691): DFGByteCodeParser.cpp:1483:83: warning: comparison is always false due to limited range of data type [-Wtype-limits] 2017-11-15 13:08:29 -0800 1 1 1 Unclassified WebKit JavaScriptCore Other PC Linux RESOLVED FIXED https://bugs.webkit.org/show_bug.cgi?id=176601 InRadar P2 Normal --- 1 mcatanzaro saam buildbot commit-queue fpizlo keith_miller mark.lam mcatanzaro msaboff rmorisset rniwa saam webkit-bug-importer ysuzuki oldest_to_newest 1362316 0 mcatanzaro 2017-10-19 14:10:36 -0700 r223691 "Turn recursive tail calls into loops" introduced the following warning when building WebKitGTK+ with GCC 7.2.1: ../../Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp: In member function ‘bool JSC::DFG::ByteCodeParser::handleRecursiveTailCall(JSC::DFG::Node*, const JSC::CallLinkStatus&, int, JSC::VirtualRegister, int)’: ../../Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:1483:83: warning: comparison is always false due to limited range of data type [-Wtype-limits] } while (stackEntry->m_inlineCallFrame && stackEntry->m_inlineCallFrame->kind == TailCall && (stackEntry = stackEntry->m_caller)); ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~ It's a bit surprising because InlineCallFrame::Kind has only eight possible values (including TailCall), and InlineCallFrame::kind is an unsigned bitfield three bits wide, which seems like it should be enough. 1362320 1 saam 2017-10-19 14:14:30 -0700 (In reply to Michael Catanzaro from comment #0) > r223691 "Turn recursive tail calls into loops" introduced the following > warning when building WebKitGTK+ with GCC 7.2.1: > > ../../Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp: In member function > ‘bool JSC::DFG::ByteCodeParser::handleRecursiveTailCall(JSC::DFG::Node*, > const JSC::CallLinkStatus&, int, JSC::VirtualRegister, int)’: > ../../Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:1483:83: warning: > comparison is always false due to limited range of data type [-Wtype-limits] > } while (stackEntry->m_inlineCallFrame && > stackEntry->m_inlineCallFrame->kind == TailCall && (stackEntry = > stackEntry->m_caller)); > > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~ > > It's a bit surprising because InlineCallFrame::Kind has only eight possible > values (including TailCall), and InlineCallFrame::kind is an unsigned > bitfield three bits wide, which seems like it should be enough. Oh wow. This should be stackEntry->m_inlineCallFrame->kind == InlineCallFrame::TailCall I think 1362326 2 saam 2017-10-19 14:20:30 -0700 Will fix. I wonder if clang does the right thing here. 1362336 3 324287 saam 2017-10-19 14:27:34 -0700 Created attachment 324287 patch 1362337 4 saam 2017-10-19 14:28:02 -0700 gotta love the C++ type system. 1362346 5 saam 2017-10-19 14:30:19 -0700 Ima let the tests run on this just in case we are hitting entirely new code. 1362377 6 mcatanzaro 2017-10-19 15:02:02 -0700 Ahhh, I was super confused. So TailCall there must be coming from the NodeType enum in DFGNodeType.h. If NodeType were changed to be an enum class, weird bugs like this could be avoided in the future. 1362380 7 mcatanzaro 2017-10-19 15:03:17 -0700 Unfortunately: ** The following JSC stress test failures have been introduced: wasm.yaml/wasm/js-api/call-indirect.js.default-wasm 1362381 8 saam 2017-10-19 15:05:10 -0700 (In reply to Michael Catanzaro from comment #6) > Ahhh, I was super confused. So TailCall there must be coming from the > NodeType enum in DFGNodeType.h. > > If NodeType were changed to be an enum class, weird bugs like this could be > avoided in the future. Indeed. 1362382 9 saam 2017-10-19 15:05:26 -0700 (In reply to Michael Catanzaro from comment #7) > Unfortunately: > > ** The following JSC stress test failures have been introduced: > wasm.yaml/wasm/js-api/call-indirect.js.default-wasm With the change applied locally on your machine? 1362383 10 mcatanzaro 2017-10-19 15:07:33 -0700 (In reply to Saam Barati from comment #9) > (In reply to Michael Catanzaro from comment #7) > > Unfortunately: > > > > ** The following JSC stress test failures have been introduced: > > wasm.yaml/wasm/js-api/call-indirect.js.default-wasm > > With the change applied locally on your machine? No, I actually got that from the JSC EWS: https://webkit-queues.webkit.org/results/4924194 But the EWS just reran the tests and the it passed this time. The test must be flaky. 1362385 11 saam 2017-10-19 15:09:02 -0700 (In reply to Michael Catanzaro from comment #10) > (In reply to Saam Barati from comment #9) > > (In reply to Michael Catanzaro from comment #7) > > > Unfortunately: > > > > > > ** The following JSC stress test failures have been introduced: > > > wasm.yaml/wasm/js-api/call-indirect.js.default-wasm > > > > With the change applied locally on your machine? > > No, I actually got that from the JSC EWS: > > https://webkit-queues.webkit.org/results/4924194 > > But the EWS just reran the tests and the it passed this time. The test must > be flaky. It's still possible this patch introduced the failure. I'll run it locally without CJIT on. 1362452 12 324287 commit-queue 2017-10-19 16:49:27 -0700 Comment on attachment 324287 patch Clearing flags on attachment: 324287 Committed r223729: <https://trac.webkit.org/changeset/223729> 1362453 13 commit-queue 2017-10-19 16:49:28 -0700 All reviewed patches have been landed. Closing bug. 1362665 14 rmorisset 2017-10-20 05:01:26 -0700 Wow, I had totally missed that. Thank you for finding it and fixing it so quickly. 1364519 15 commit-queue 2017-10-25 15:11:50 -0700 Re-opened since this is blocked by bug 178834 1364526 16 rniwa 2017-10-25 15:15:43 -0700 There's no reason to re-open this since webkit.org/b/176601 has been re-opened. 1372375 17 webkit-bug-importer 2017-11-15 13:08:29 -0800 <rdar://problem/35568861> 324287 2017-10-19 14:27:34 -0700 2017-10-19 16:49:27 -0700 patch b-backup.diff text/plain 1629 saam SW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKPT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291 cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkocmV2aXNpb24gMjIzNzEyKQorKysgU291cmNl L0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDEzIEBA CisyMDE3LTEwLTE5ICBTYWFtIEJhcmF0aSAgPHNiYXJhdGlAYXBwbGUuY29tPgorCisgICAgICAg IFJFR1JFU1NJT04ocjIyMzY5MSk6IERGR0J5dGVDb2RlUGFyc2VyLmNwcDoxNDgzOjgzOiB3YXJu aW5nOiBjb21wYXJpc29uIGlzIGFsd2F5cyBmYWxzZSBkdWUgdG8gbGltaXRlZCByYW5nZSBvZiBk YXRhIHR5cGUgWy1XdHlwZS1saW1pdHNdCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3Jn L3Nob3dfYnVnLmNnaT9pZD0xNzg1NDMKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9P UFMhKS4KKworICAgICAgICAqIGRmZy9ERkdCeXRlQ29kZVBhcnNlci5jcHA6CisgICAgICAgIChK U0M6OkRGRzo6Qnl0ZUNvZGVQYXJzZXI6OmhhbmRsZVJlY3Vyc2l2ZVRhaWxDYWxsKToKKwogMjAx Ny0xMC0xOSAgU2FhbSBCYXJhdGkgIDxzYmFyYXRpQGFwcGxlLmNvbT4KIAogICAgICAgICBUdXJu IHZhcmlvdXMgcG9seSBwcm90byBSRUxFQVNFX0FTU0VSVHMgaW50byBBU1NFUlRzIGJlY2F1c2Ug dGhleSdyZSBvbiB0aGUgaG90IHBhdGggaW4gc3BlZWRvbWV0ZXIKSW5kZXg6IFNvdXJjZS9KYXZh U2NyaXB0Q29yZS9kZmcvREZHQnl0ZUNvZGVQYXJzZXIuY3BwCj09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJj ZS9KYXZhU2NyaXB0Q29yZS9kZmcvREZHQnl0ZUNvZGVQYXJzZXIuY3BwCShyZXZpc2lvbiAyMjM3 MTEpCisrKyBTb3VyY2UvSmF2YVNjcmlwdENvcmUvZGZnL0RGR0J5dGVDb2RlUGFyc2VyLmNwcAko d29ya2luZyBjb3B5KQpAQCAtMTQ4MCw3ICsxNDgwLDcgQEAgYm9vbCBCeXRlQ29kZVBhcnNlcjo6 aGFuZGxlUmVjdXJzaXZlVGFpbAogICAgICAgICBhZGRKdW1wVG8oKmVudHJ5QmxvY2tQdHIpOwog ICAgICAgICByZXR1cm4gdHJ1ZTsKICAgICAgICAgLy8gSXQgd291bGQgYmUgdW5zb3VuZCB0byBq dW1wIG92ZXIgYSBub24tdGFpbCBjYWxsOiB0aGUgInRhaWwiIGNhbGwgaXMgbm90IHJlYWxseSBh IHRhaWwgY2FsbCBpbiB0aGF0IGNhc2UuCi0gICAgfSB3aGlsZSAoc3RhY2tFbnRyeS0+bV9pbmxp bmVDYWxsRnJhbWUgJiYgc3RhY2tFbnRyeS0+bV9pbmxpbmVDYWxsRnJhbWUtPmtpbmQgPT0gVGFp bENhbGwgJiYgKHN0YWNrRW50cnkgPSBzdGFja0VudHJ5LT5tX2NhbGxlcikpOworICAgIH0gd2hp bGUgKHN0YWNrRW50cnktPm1faW5saW5lQ2FsbEZyYW1lICYmIHN0YWNrRW50cnktPm1faW5saW5l Q2FsbEZyYW1lLT5raW5kID09IElubGluZUNhbGxGcmFtZTo6VGFpbENhbGwgJiYgKHN0YWNrRW50 cnkgPSBzdGFja0VudHJ5LT5tX2NhbGxlcikpOwogCiAgICAgLy8gVGhlIHRhaWwgY2FsbCB3YXMg bm90IHJlY3Vyc2l2ZQogICAgIHJldHVybiBmYWxzZTsK