If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

TraceRecorder::record_JSOP_GETELEM does some shifty checking

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: mrbkap, Assigned: gal)

Tracking

({fixed1.9.1})

Trunk
x86
Linux
fixed1.9.1
Points:
---
Bug Flags:
wanted1.9.1 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 attachment)

(Reporter)

Description

9 years ago
In particular:
- It does a record-time check for dense array
- It checks for indexes less than 0, even though TraceRecorder::elem handles those.
(Reporter)

Comment 1

9 years ago
Created attachment 377335 [details] [diff] [review]
Proposed fix

I *think* this is the way to do branch exits -- is this the right way to think about this?
Attachment #377335 - Flags: review?(gal)
(Assignee)

Comment 2

9 years ago
Comment on attachment 377335 [details] [diff] [review]
Proposed fix

>Bug 492912 - Clean up the way that TraceRecorder::elem interacts with the rest of the world.
>
>diff --git a/js/src/jstracer.cpp b/js/src/jstracer.cpp
>--- a/js/src/jstracer.cpp
>+++ b/js/src/jstracer.cpp
>@@ -5829,17 +5829,23 @@ TraceRecorder::incProp(jsint incr, bool 
> JS_REQUIRES_STACK JSRecordingStatus
> TraceRecorder::incElem(jsint incr, bool pre)
> {
>     jsval& r = stackval(-1);
>     jsval& l = stackval(-2);
>     jsval* vp;
>     LIns* v_ins;
>     LIns* addr_ins;
>-    CHECK_STATUS(elem(l, r, vp, v_ins, addr_ins));
>+
>+    if (!JSVAL_IS_OBJECT(l) || !JSVAL_IS_INT(r) ||
>+        !guardDenseArray(JSVAL_TO_OBJECT(l), get(&l))) {
>+        return JSRS_STOP;
>+    }
>+
>+    CHECK_STATUS(denseArrayElem(l, r, vp, v_ins, addr_ins));
>     if (!addr_ins) // if we read a hole, abort
>         return JSRS_STOP;
>     CHECK_STATUS(inc(*vp, v_ins, incr, pre));
>     box_jsval(*vp, v_ins);
>     lir->insStorei(v_ins, addr_ins, 0);
>     return JSRS_CONTINUE;
> }
> 
>@@ -8134,27 +8140,26 @@ TraceRecorder::record_JSOP_GETELEM()
> 
>         // The object is not guaranteed to be a dense array at this point, so it might be the
>         // global object, which we have to guard against.
>         CHECK_STATUS(guardNotGlobalObject(obj, obj_ins));
> 
>         return call_imacro(call ? callelem_imacros.callprop : getelem_imacros.getprop);
>     }
> 
>-    // Invalid dense array index or not a dense array.
>-    if (JSVAL_TO_INT(idx) < 0 || !OBJ_IS_DENSE_ARRAY(cx, obj)) {
>+    if (!guardDenseArray(obj, obj_ins, BRANCH_EXIT)) {
>         CHECK_STATUS(guardNotGlobalObject(obj, obj_ins));

We could also only guard on the fast path since the slow path can do both. But this way we can specialize later. On the other hand slow arrays never go fast again.

> 
>         return call_imacro(call ? callelem_imacros.callelem : getelem_imacros.getelem);
>     }
> 
>-    // Fast path for dense arrays accessed with a non-negative integer index.
>+    // Fast path for dense arrays accessed with a integer index.
>     jsval* vp;
>     LIns* addr_ins;
>-    CHECK_STATUS(elem(lval, idx, vp, v_ins, addr_ins));
>+    CHECK_STATUS(denseArrayElem(lval, idx, vp, v_ins, addr_ins));
>     set(&lval, v_ins);
>     if (call)
>         set(&idx, obj_ins);
>     return JSRS_CONTINUE;
> }
> 
> /* Functions used by JSOP_SETELEM */
> 
>@@ -8920,31 +8925,25 @@ TraceRecorder::prop(JSObject* obj, LIns*
> 
>     v_ins = stobj_get_slot(obj_ins, slot, dslots_ins);
>     unbox_jsval(STOBJ_GET_SLOT(obj, slot), v_ins, snapshot(BRANCH_EXIT));
> 
>     return JSRS_CONTINUE;
> }
> 
> JS_REQUIRES_STACK JSRecordingStatus
>-TraceRecorder::elem(jsval& oval, jsval& ival, jsval*& vp, LIns*& v_ins, LIns*& addr_ins)
>-{
>-    /* no guards for type checks, trace specialized this already */
>-    if (JSVAL_IS_PRIMITIVE(oval) || !JSVAL_IS_INT(ival))
>-        return JSRS_STOP;
>+TraceRecorder::denseArrayElem(jsval& oval, jsval& ival, jsval*& vp, LIns*& v_ins, LIns*& addr_ins)

denseArrayElem .... ent? Can as well spell it out now.

>+{
>+    JS_ASSERT(JSVAL_IS_OBJECT(oval) && JSVAL_IS_INT(ival));
> 
>     JSObject* obj = JSVAL_TO_OBJECT(oval);
>     LIns* obj_ins = get(&oval);
>     jsint idx = JSVAL_TO_INT(ival);
>     LIns* idx_ins = makeNumberInt32(get(&ival));
> 
>-    /* make sure the object is actually a dense array */
>-    if (!guardDenseArray(obj, obj_ins))
>-        return JSRS_STOP;
>-
>     VMSideExit* exit = snapshot(BRANCH_EXIT);
> 
>     /* check that the index is within bounds */
>     LIns* dslots_ins = lir->insLoad(LIR_ldp, obj_ins, offsetof(JSObject, dslots));
>     jsuint capacity = js_DenseArrayCapacity(obj);
>     bool within = (jsuint(idx) < jsuint(obj->fslots[JSSLOT_ARRAY_LENGTH]) && jsuint(idx) < capacity);
>     if (!within) {
>         /* If idx < 0, stay on trace (and read value as undefined, since this is a dense array). */
>diff --git a/js/src/jstracer.h b/js/src/jstracer.h

Tryserver it for sure.







































This extra padding was added intentionally for shaver ;)
Attachment #377335 - Flags: review?(gal) → review+
(Reporter)

Comment 3

9 years ago
http://hg.mozilla.org/tracemonkey/rev/c0cb2d11b999
Whiteboard: fixed-in-tracemonkey

Comment 4

9 years ago
http://hg.mozilla.org/mozilla-central/rev/c0cb2d11b999
Flags: wanted1.9.1+

Comment 5

9 years ago
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/f151e2e0647e
Keywords: fixed1.9.1
Again, blake has a hard blocker.  Is this work done in progress of his blockers?
(Assignee)

Comment 7

9 years ago
This was a quick hack a couple days ago and its landed. I will take over the bug in case there are any followup regressions.
Assignee: mrbkap → gal
Depends on: 517721

Updated

8 years ago
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.