1389904 - bug 1387968 caused coverity to think some codes are dead

Status: RESOLVED FIXED

(Core :: JavaScript: Standard Library, defect)

Description

[Sylvestre Ledru [:Sylvestre]]

For
if (JS7_ISDEC(c)) {
coverity thinks
cond_at_least: Condition (unsigned int)c - 48U <= 9U, taking false branch. Now the value of c is at least 58.


https://hg.mozilla.org/mozilla-central/annotate/tip/js/src/builtin/RegExp.cpp#l1273


    at_least: When switching on c, the value of c must be at least 58.
1273    switch (c) {
1274      default:
1275        return false;
    CID undefined (2) (#1-5 of 8): Logically dead code (DEADCODE) [select issue]
1276      case '$':
1277        out->init(replacement, currentDollar - replacementBegin, 1);
1278        break;
    CID undefined (2) (#2-6 of 8): Logically dead code (DEADCODE) [select issue]
1279      case '&':
1280        out->init(matched, 0, matched->length());
1281        break;
    CID undefined (2) (#3-7 of 8): Logically dead code (DEADCODE) [select issue]
[...]

    dead_error_condition: The switch value c cannot be 39.
    CID undefined (#4-8 of 8): Logically dead code (DEADCODE)dead_error_begin: Execution cannot reach this statement: case 39:.
1292      case '\'':
1293        out->init(string, tailPos, string->length() - tailPos);
1294        break;

Comment 1

[Sylvestre Ledru [:Sylvestre]]

André, do you agree?

Comment 2

[André Bargull [:anba]]

(In reply to Sylvestre Ledru [:sylvestre] from comment #1)
> André, do you agree?

It looks like a misclassification. c is of type CharT, where CharT is either |unsigned char| or |char16_t|. So CharT is definitely unsigned, and converting it to |unsigned int| in JS_ISDEC will keep it unsigned. But it looks like Coverity performs a signed subtraction/comparison, otherwise I can't explain why it assumes c is at least 58 in the false branch, because subtracting 48U from an unsigned value below 48U should wrap around. 

But I guess I'll just change the type of |c| back to |char16_t|, if that helps to avoid this Coverity warning.

Comment 3

[André Bargull [:anba]]

Attachment: bug1389904.patch

This reverts the variable type change from https://hg.mozilla.org/mozilla-central/rev/cd1d769cabe8 to avoid a false-positive from Coverity.

Comment 4

[Till Schneidereit [:till]]

Comment on attachment 8897863 [details] [diff] [review]
bug1389904.patch

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

r=me

Comment 5

[André Bargull [:anba]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c94993d0bd5230f391512f0b30ca2642646896e8

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6ec1b10f8474
Change variable type back to char16_t to avoid false-positives from coverity. r=till

Comment 7

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

https://hg.mozilla.org/mozilla-central/rev/6ec1b10f8474