Closed Bug 601129 Opened 14 years ago Closed 3 years ago

sixgill annotations for JS

Categories

(Core :: JavaScript Engine, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED INVALID

People

(Reporter: bhackett1024, Assigned: bhackett1024)

Details

Attachments

(2 files)

It would be good to start adding sixgill annotations (http://sixgill.org) to JS.  These annotations fix false positives and make it easy to catch new bugs as they come in.  Reports can be viewed at:

http://dm-sixgill01.mozilla.org

Which is currently behind LDAP (might change soon).  All JS annotations will be attached to this bug.  This attachment does two things:

A) Adds annotation defines to jsutil.h (jsstaticcheck.h isn't included everywhere).  These are also in xpcom/glue/nsDebug.h, which isn't included by JS.  The defines in both headers are modified so that both can be included without issue.

B) Adds 15 annotations to JS for read overflows on Values.  These cut out 571 false positives, mostly related to JS natives and accesses on the JS stack.  There are 48 warnings for such reads left in JS, which need more annotations (will be easier after bug 584917 lands).

Who should review these?
(In reply to comment #0)
> Who should review these?

I heard jorendorff is all about assertions, I'd send it his way. :-)
Attachment #480113 - Flags: review?(jorendorff)
Comment on attachment 480113 [details] [diff] [review]
header fixes and annotations

jsutil.h is included from jsapi.h. I don't think we want macro names like
STATIC_ASSERT to be in our public headers. Please put them in jsstaticcheck.h
and include that from jsprvtd.h (which will cover dozens of .cpp files) and
whatever other files need it.

Otherwise this looks good.

>+        STATIC_ASSUME(!p || ubound((char*)p) >= size + incr);              \

Style nit: put a space in "(char *)" please.

>+STATIC_PRECONDITION_ASSUME(ubound(vp) >= argc + 2)
> JS_ALWAYS_INLINE bool
> CallJSNative(JSContext *cx, js::Native native, uintN argc, js::Value *vp)

Wouldn't it be nice if instead of assuming this, we could require the callers
to vouch for it. But OK.

r=me.
Attachment #480113 - Flags: review?(jorendorff) → review+
http://hg.mozilla.org/tracemonkey/rev/c55fcc4e8588

I'm hoping that eventually CallJSNative can have a checked precondition, the problem now is that using a checked precondition will just cause the tool to get stuck on complex invariants describing the stack in the interpreter.  The assume vs. assert annotations provide a nice way of marking the boundary between code that can and can't be checked: we can determine the correctness of every JS native, modulo bugs in the interpreter or JITs themselves.  That annotation turned up overflows in the natives, and (provided things are maintained in the future) I'm confident no similar bugs will ship with future versions of Firefox.
http://hg.mozilla.org/mozilla-central/rev/c55fcc4e8588
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Attached patch annotationsSplinter Review
These annotations fix a bunch of warnings in jsemit and jstypedarray, and allow checking for object slot accesses (getSlot, setSlot, etc.)
Assignee: general → bhackett1024
Status: RESOLVED → REOPENED
Attachment #483979 - Flags: review?(jorendorff)
Resolution: FIXED → ---
Attachment #483979 - Flags: review?(jorendorff) → review+
(In reply to comment #6)
> Created attachment 483979 [details] [diff] [review]
> annotations
> 
> These annotations fix a bunch of warnings in jsemit and jstypedarray, and allow
> checking for object slot accesses (getSlot, setSlot, etc.)

Was the annotations patch also landed?
Status: REOPENED → RESOLVED
Closed: 14 years ago3 years ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.