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.
(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
Will fix. I wonder if clang does the right thing here.
Created attachment 324287 [details] patch
gotta love the C++ type system.
Ima let the tests run on this just in case we are hitting entirely new code.
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.
Unfortunately: ** The following JSC stress test failures have been introduced: wasm.yaml/wasm/js-api/call-indirect.js.default-wasm
(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.
(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?
(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.
(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.
Comment on attachment 324287 [details] patch Clearing flags on attachment: 324287 Committed r223729: <https://trac.webkit.org/changeset/223729>
All reviewed patches have been landed. Closing bug.
Wow, I had totally missed that. Thank you for finding it and fixing it so quickly.
Re-opened since this is blocked by bug 178834
There's no reason to re-open this since webkit.org/b/176601 has been re-opened.
<rdar://problem/35568861>