Closed Bug 1389904 Opened 3 years ago Closed 3 years ago

bug 1387968 caused coverity to think some codes are dead

Categories

(Core :: JavaScript: Standard Library, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: Sylvestre, Assigned: anba)

References

(Blocks 1 open bug)

Details

(Keywords: coverity, Whiteboard: [CID 1416387])

Attachments

(1 file)

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;
André, do you agree?
Flags: needinfo?(andrebargull)
(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.
Flags: needinfo?(andrebargull)
Assignee: nobody → andrebargull
Status: NEW → ASSIGNED
Attached patch bug1389904.patchSplinter Review
This reverts the variable type change from https://hg.mozilla.org/mozilla-central/rev/cd1d769cabe8 to avoid a false-positive from Coverity.
Attachment #8897863 - Flags: review?(till)
Comment on attachment 8897863 [details] [diff] [review]
bug1389904.patch

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

r=me
Attachment #8897863 - Flags: review?(till) → review+
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6ec1b10f8474
Change variable type back to char16_t to avoid false-positives from coverity. r=till
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/6ec1b10f8474
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.