Closed Bug 492912 Opened 16 years ago Closed 15 years ago

TraceRecorder::record_JSOP_GETELEM does some shifty checking

Categories

(Core :: JavaScript Engine, defect)

x86
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: mrbkap, Assigned: gal)

References

Details

(Keywords: fixed1.9.1, Whiteboard: fixed-in-tracemonkey)

Attachments

(1 file)

In particular: - It does a record-time check for dense array - It checks for indexes less than 0, even though TraceRecorder::elem handles those.
Attached patch Proposed fixSplinter Review
I *think* this is the way to do branch exits -- is this the right way to think about this?
Attachment #377335 - Flags: review?(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 ;)
Attachment #377335 - Flags: review?(gal) → review+
Again, blake has a hard blocker. Is this work done in progress of his blockers?
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
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: