Fix issues found by clang's scan-build

RESOLVED FIXED in mozilla11

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: evilpie, Assigned: evilpie)

Tracking

unspecified
mozilla11
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Comment hidden (empty)
(Assignee)

Comment 1

6 years ago
http://javascript-reverse.tumblr.com/post/13917377820/how-to-use-scan-build-to-analyze-spidermonkey

The actual report: http://dl.dropbox.com/u/11402872/scan-build-2011-12-08-1/index.html
(Assignee)

Comment 2

6 years ago
Created attachment 582056 [details] [diff] [review]
v1

This fixes most of the actual issues, except:

>Dead store	Dead initialization	methodjit /InvokeHelpers.cpp	116
bhackett or luke are going to change the function or even remove it?
>Dead store	Dead assignment	jsopcode.cpp	2200
Oh well, never mind
>Dead code	Idempotent operation	frontend /BytecodeEmitter.cpp	3184	
This just works

All the 'Dereference of null pointer' look fishy to me, but i wouldn't mind if somebody would double check.
Assignee: general → evilpies
Status: NEW → ASSIGNED
Attachment #582056 - Flags: review?(jwalden+bmo)
Comment on attachment 582056 [details] [diff] [review]
v1

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

I didn't look much at the other warnings, but some definitely look fixable with some this-function-doesn't-return annotations (assuming what I saw that looked like no-return functions actually were).  We should add MOZ_NO_RETURN to mfbt at some point for this.

::: js/src/jsinfer.cpp
@@ +4178,5 @@
>           * Don't track for parents which add call objects or are generators,
>           * don't resolve NAME accesses into the parent.
>           */
> +        if (!detached && (nesting->parent->analysis()->addsScopeObjects() ||
> +            JSOp(*nesting->parent->code) == JSOP_GENERATOR)) 

The previous wrapping was more readable.  This one suggests three equal-level terms evaluated sequentially.  That said, I don't think the previous wrapping was all that great, either.  How about this:

if (!detached) {
    /*
     * Don't track for...
     */
    if (nesting->parent->analysis()->addsScopeObjects() || JSOp(*nesting->parent()->code) == JSOP_GENERATOR)
        DetachNestingParent(script);
}

::: js/src/jsinterp.cpp
@@ +2099,1 @@
>              len = JSOP_CALL_LENGTH;

I'd somewhat prefer if this were moved next to the DO_NEXT_OP(len) below -- mind moving it?

::: js/src/jsxml.cpp
@@ +3050,2 @@
>      JS_ASSERT(list->xml_class == JSXML_CLASS_LIST);
> +    uint32 i = list->xml_kids.length;

uint32_t

@@ +3053,4 @@
>      if (xml->xml_class == JSXML_CLASS_LIST) {
>          list->xml_target = xml->xml_target;
>          list->xml_targetprop = xml->xml_targetprop;
> +        uint32 n = JSXML_LENGTH(xml);

uint32_t

@@ +3061,1 @@
>              if (kid)

Make this |if (JSXML *kid = ...)|.
Attachment #582056 - Flags: review?(jwalden+bmo) → review+
(Assignee)

Comment 4

6 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/caeef8ca5d94
https://hg.mozilla.org/mozilla-central/rev/caeef8ca5d94https://hg.mozilla.org/mozilla-central/rev/caeef8ca5d94
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
You need to log in before you can comment on or make changes to this bug.