1239269 - [Static Analysis][Operands don't affect result] In functions js::RegExpMatcherRaw() and js::RegExpTesterRaw()

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Andi [:andi]]

The Static Analysis tool Coverity added that below code from functions js::RegExpMatcherRaw() and js::RegExpTesterRaw() don't affect the outcome of the flow:

>>    MOZ_ASSERT(lastIndex <= INT32_MAX);

Since in both cases type of lastIndex is int32_t and INT32_MAX is:

>>   #define INT32_MAX        2147483647i32 

MOZ_ASSERT becomes useless. Although code is used only on debug we should still remove it.

Comment 1

[Andi [:andi]]

Attachment: MozReview Request: Bug 1239269 - as lastIndex cannot be negative change it's storage class from int32_t to uint32_t, r?Waldo

Review commit: https://reviewboard.mozilla.org/r/30673/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/30673/

Comment 2

[Nicolas B. Pierron [:nbp]]

Maybe it would make sense to change this assertion to test that lastIndex does not go above the Max string length?

Comment 3

[Andi [:andi]]

(In reply to Nicolas B. Pierron [:nbp] from comment #2)
> Maybe it would make sense to change this assertion to test that lastIndex
> does not go above the Max string length?

1. For path
 js::RegExpTesterRaw test is done in function ExecuteRegExp():

>>    /* Handled by caller */
>>    MOZ_ASSERT(lastIndex >= 0 && size_t(lastIndex) <= input->length());
>>
>>    /* Steps 4-10 performed by the caller. */

2. For path js::RegExpMatcherRaw test is done in function RegExpMatcherImpl()->ExecuteRegExp()

So i guess we are safe to avoid any further checks.

Comment 4

[Jeff Walden [:Waldo]]

Seems to me like lastIndex should be uint32_t.  I'm fairly sure we pass int32_t as uint32_t in some JIT entry points, and this seems like another place where that should be done.  That would also make the assertion reasonable.

Comment 5

[Andi [:andi]]

Attachment: as index cannot be negative make change it's storage from int32_t to uint32_t,

Comment 6

[Jeff Walden [:Waldo]]

Comment on attachment 8715768 [details] [diff] [review]
as index cannot be negative make change it's storage from int32_t to uint32_t,

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

::: js/src/builtin/RegExp.cpp
@@ +932,4 @@
>                       MatchPairs* maybeMatches, MutableHandleValue output)
> +{
> +    MOZ_ASSERT(lastIndex <= INT32_MAX);
> +    

Kill the trailing whitespace here.  (Not sure how any line but the |uint32_t lastIndex| line here got changed, to be honest.)

::: js/src/builtin/RegExp.h
@@ +55,5 @@
>  RegExpTester(JSContext* cx, unsigned argc, Value* vp);
>  
>  extern bool
>  RegExpTesterRaw(JSContext* cx, HandleObject regexp, HandleString input,
> +                uint32_t lastIndex, bool sticky, uint32_t* endIndex);

|endIndex| here shouldn't be changed to uint32_t.  RegExp testing returns -1 in the case where no match was found, so it makes sense for this to be signed.

::: js/src/jit/CodeGenerator.cpp
@@ +1915,5 @@
>      }
>  };
>  
>  typedef bool (*RegExpTesterRawFn)(JSContext* cx, HandleObject regexp, HandleString input,
> +                                  uint32_t lastIndex, bool sticky, uint32_t* result);

And then we should continue to have |int32_t* result| here.

::: js/src/jit/Recover.cpp
@@ +1040,5 @@
>      RootedString string(cx, iter.read().toString());
>      RootedObject regexp(cx, &iter.read().toObject());
>      int32_t lastIndex = iter.read().toInt32();
>      bool sticky = iter.read().toBoolean();
> +    uint32_t endIndex;

And |int32_t| again.

Comment 7

[Andi [:andi]]

Comment on attachment 8707389 [details]
MozReview Request: Bug 1239269 - as lastIndex cannot be negative change it's storage class from int32_t to uint32_t, r?Waldo

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/30673/diff/1-2/

Comment 8

[Jeff Walden [:Waldo]]

Comment on attachment 8707389 [details]
MozReview Request: Bug 1239269 - as lastIndex cannot be negative change it's storage class from int32_t to uint32_t, r?Waldo

https://reviewboard.mozilla.org/r/30673/#review31155

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/97c7a71cce02

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/97c7a71cce02