Closed
Bug 601129
Opened 14 years ago
Closed 3 years ago
sixgill annotations for JS
Categories
(Core :: JavaScript Engine, enhancement)
Core
JavaScript Engine
Tracking
()
RESOLVED
INVALID
People
(Reporter: bhackett1024, Assigned: bhackett1024)
Details
Attachments
(2 files)
12.15 KB,
patch
|
jorendorff
:
review+
|
Details | Diff | Splinter Review |
10.02 KB,
patch
|
jorendorff
:
review+
|
Details | Diff | Splinter Review |
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?
Assignee | ||
Comment 1•14 years ago
|
||
Comment 2•14 years ago
|
||
(In reply to comment #0) > Who should review these? I heard jorendorff is all about assertions, I'd send it his way. :-)
Assignee | ||
Updated•14 years ago
|
Attachment #480113 -
Flags: review?(jorendorff)
Comment 3•14 years ago
|
||
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+
Assignee | ||
Comment 4•14 years ago
|
||
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.
Comment 5•14 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/c55fcc4e8588
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 6•14 years ago
|
||
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 → ---
Updated•14 years ago
|
Attachment #483979 -
Flags: review?(jorendorff) → review+
Comment 7•13 years ago
|
||
(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?
Updated•3 years ago
|
Status: REOPENED → RESOLVED
Closed: 14 years ago → 3 years ago
Resolution: --- → INVALID
You need to log in
before you can comment on or make changes to this bug.
Description
•