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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Daniel Holbert [:dholbert]]

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

Comment 1

[Daniel Holbert [:dholbert]]

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

Comment 2

[Daniel Holbert [:dholbert]]

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.

Comment 3

[Kannan Vijayan [:djvj]]

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.

Comment 4

[Kannan Vijayan [:djvj]]

Attachment: fix-warning.patch

Fix nonsensical assert.

Comment 5

[Nicolas B. Pierron [:nbp]]

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 ;)

Comment 6

[Jeff Walden [:Waldo]]

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.

Comment 7

[Kannan Vijayan [:djvj]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9a4134bbdae4
https://hg.mozilla.org/integration/mozilla-inbound/rev/512914f8150f

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9a4134bbdae4
https://hg.mozilla.org/mozilla-central/rev/512914f8150f