Bug 178543 – REGRESSION(r223691): DFGByteCodeParser.cpp:1483:83: warning: comparison is always false due to limited range of data type [-Wtype-limits]

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Michael Catanzaro]

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.

[Saam Barati]

(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

[Saam Barati]

Will fix. I wonder if clang does the right thing here.

[Saam Barati]

Created attachment 324287 [details]
patch

[Saam Barati]

gotta love the C++ type system.

[Saam Barati]

Ima let the tests run on this just in case we are hitting entirely new code.

[Michael Catanzaro]

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.

[Michael Catanzaro]

Unfortunately:

** The following JSC stress test failures have been introduced:
	wasm.yaml/wasm/js-api/call-indirect.js.default-wasm

[Saam Barati]

(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.

[Saam Barati]

(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?

[Michael Catanzaro]

(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.

[Saam Barati]

(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.

[WebKit Commit Bot]

Comment on attachment 324287 [details]
patch

Clearing flags on attachment: 324287

Committed r223729: <https://trac.webkit.org/changeset/223729>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Robin Morisset]

Wow, I had totally missed that. Thank you for finding it and fixing it so quickly.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 178834

[Ryosuke Niwa]

There's no reason to re-open this since webkit.org/b/176601 has been re-opened.

[Radar WebKit Bug Importer]

<rdar://problem/35568861>