BaselineJIT.cpp:256:42: warning: comparison is always false due to limited range of data type [-Wtype-limits]

RESOLVED FIXED in mozilla27

Status

()

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: dholbert, Assigned: djvj)

Tracking

(Blocks: 1 bug)

Trunk
mozilla27
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

5 years ago
New build warning:
{
js/src/jit/BaselineJIT.cpp: In function ‘js::jit::MethodStatus CanEnterBaselineJIT(JSContext*, JS::HandleScript, bool)’:

js/src/jit/BaselineJIT.cpp:256:42: warning: comparison is always false due to limited range of data type [-Wtype-limits]

      if (script->nslots > BaselineScript::MAX_JSSCRIPT_SLOTS)
                                           ^
}

The comparison is ineffective because "nslots" is a uint16_t (assuming I'm looking at the right variable)...
> 435 class JSScript : public js::gc::Cell
> 436 {
[...]
> 535     uint16_t        nslots;     /* vars plus maximum stack depth */
http://mxr.mozilla.org/mozilla-central/source/js/src/jsscript.h#535

...so, its maximum value is 2^16.

In comparison, MAX_JSSCRIPT_SLOTS is 0xfffffu = 2^20 - 1

This constant and the comparison were both added in this cset for bug 905523:
http://hg.mozilla.org/mozilla-central/rev/0452b5b504d0
(Reporter)

Comment 1

5 years ago
(In reply to Daniel Holbert [:dholbert] from comment #0)
> The comparison is ineffective because "nslots" is a uint16_t [...]
> ...so, its maximum value is 2^16.

(er, make that 2^16 - 1 (not that it matters))
(Reporter)

Comment 2

5 years ago
I suspect the constant wanted to be 0xffff (4 f's instead of 5)?

But even if that's the case, the comparison is still unnecessary (and would still trigger a build warning), because uint16_t's could reach that value but can never go over it.
(Assignee)

Comment 3

5 years ago
Actually I think the assert should just be removed then.  I just wanted to make sure that (nslots * sizeof(Value)) would always fit within an int32_t, and if nslots is a uint16_t, then that'll trivially be true.
(Assignee)

Comment 4

5 years ago
Created attachment 807756 [details] [diff] [review]
fix-warning.patch

Fix nonsensical assert.
Attachment #807756 - Flags: review?(nicolas.b.pierron)
(Assignee)

Updated

5 years ago
Assignee: general → kvijayan
Comment on attachment 807756 [details] [diff] [review]
fix-warning.patch

Review of attachment 807756 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit/BaselineJIT.cpp
@@ +252,5 @@
>  
>      if (script->length > BaselineScript::MAX_JSSCRIPT_LENGTH)
>          return Method_CantCompile;
>  
> +    JS_STATIC_ASSERT(sizeof(script->nslots) == sizeof(uint16_t));

nit: Move the comment about the stack depth above this assertion ;)
Attachment #807756 - Flags: review?(nicolas.b.pierron) → review+

Comment 6

5 years ago
Comment on attachment 807756 [details] [diff] [review]
fix-warning.patch

Review of attachment 807756 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit/BaselineJIT.cpp
@@ +252,5 @@
>  
>      if (script->length > BaselineScript::MAX_JSSCRIPT_LENGTH)
>          return Method_CantCompile;
>  
> +    JS_STATIC_ASSERT(sizeof(script->nslots) == sizeof(uint16_t));

And don't use JS_STATIC_ASSERT!  You want C++11 standard static_assert.
https://hg.mozilla.org/mozilla-central/rev/9a4134bbdae4
https://hg.mozilla.org/mozilla-central/rev/512914f8150f
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
You need to log in before you can comment on or make changes to this bug.