Bug 178543 - REGRESSION(r223691): DFGByteCodeParser.cpp:1483:83: warning: comparison is always false due to limited range of data type [-Wtype-limits]
Summary: REGRESSION(r223691): DFGByteCodeParser.cpp:1483:83: warning: comparison is al...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: Other
Hardware: PC Linux
: P2 Normal
Assignee: Saam Barati
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-10-19 14:10 PDT by Michael Catanzaro
Modified: 2017-11-15 13:08 PST (History)
12 users (show)

See Also:


Attachments
patch (1.59 KB, patch)
2017-10-19 14:27 PDT, Saam Barati
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Catanzaro 2017-10-19 14:10:36 PDT
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.
Comment 1 Saam Barati 2017-10-19 14:14:30 PDT
(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
Comment 2 Saam Barati 2017-10-19 14:20:30 PDT
Will fix. I wonder if clang does the right thing here.
Comment 3 Saam Barati 2017-10-19 14:27:34 PDT
Created attachment 324287 [details]
patch
Comment 4 Saam Barati 2017-10-19 14:28:02 PDT
gotta love the C++ type system.
Comment 5 Saam Barati 2017-10-19 14:30:19 PDT
Ima let the tests run on this just in case we are hitting entirely new code.
Comment 6 Michael Catanzaro 2017-10-19 15:02:02 PDT
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.
Comment 7 Michael Catanzaro 2017-10-19 15:03:17 PDT
Unfortunately:

** The following JSC stress test failures have been introduced:
	wasm.yaml/wasm/js-api/call-indirect.js.default-wasm
Comment 8 Saam Barati 2017-10-19 15:05:10 PDT
(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.
Comment 9 Saam Barati 2017-10-19 15:05:26 PDT
(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?
Comment 10 Michael Catanzaro 2017-10-19 15:07:33 PDT
(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.
Comment 11 Saam Barati 2017-10-19 15:09:02 PDT
(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 12 WebKit Commit Bot 2017-10-19 16:49:27 PDT
Comment on attachment 324287 [details]
patch

Clearing flags on attachment: 324287

Committed r223729: <https://trac.webkit.org/changeset/223729>
Comment 13 WebKit Commit Bot 2017-10-19 16:49:28 PDT
All reviewed patches have been landed.  Closing bug.
Comment 14 Robin Morisset 2017-10-20 05:01:26 PDT
Wow, I had totally missed that. Thank you for finding it and fixing it so quickly.
Comment 15 WebKit Commit Bot 2017-10-25 15:11:50 PDT
Re-opened since this is blocked by bug 178834
Comment 16 Ryosuke Niwa 2017-10-25 15:15:43 PDT
There's no reason to re-open this since webkit.org/b/176601 has been re-opened.
Comment 17 Radar WebKit Bug Importer 2017-11-15 13:08:29 PST
<rdar://problem/35568861>