492912 - TraceRecorder::record_JSOP_GETELEM does some shifty checking

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Blake Kaplan (:mrbkap) (inactive)]

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

Comment 1

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Proposed fix

I *think* this is the way to do branch exits -- is this the right way to think about this?

Comment 2

[Andreas Gal :gal]

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 ;)

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

http://hg.mozilla.org/tracemonkey/rev/c0cb2d11b999

Comment 4

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/c0cb2d11b999

Comment 5

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/f151e2e0647e

Comment 6

[Damon Sicore (:damons)]

Again, blake has a hard blocker.  Is this work done in progress of his blockers?

Comment 7

[Andreas Gal :gal]

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.