Bug 532568: when setting upvars, write to outer trace native stack if needed, r=dvander

js/src/jsrecursion.cpp

@@ -625,7 +625,7 @@ TraceRecorder::downRecursion()
     lirbuf->rp = lir->ins2(LIR_piadd, lirbuf->rp, lir->insImmWord(sizeof(FrameInfo*)));
     lir->insStorei(lirbuf->rp, lirbuf->state, offsetof(InterpState, rp));
     --callDepth;
-    clearFrameSlotsFromTracker(nativeFrameTracker);
+    clearCurrentFrameSlotsFromTracker(nativeFrameTracker);
 
     /*
      * If the callee and caller have identical call sites, this is a down-

js/src/jstracer.cpp

@@ -3325,6 +3325,11 @@ struct ArgClosureTraits
     // See also UpvarArgTraits.
     static inline uint32 adj_slot(JSStackFrame* fp, uint32 slot) { return 2 + slot; }
 
+    // Generate the adj_slot computation in LIR.
+    static inline LIns* adj_slot_lir(LirWriter* lir, LIns* fp_ins, unsigned slot) {
+        return lir->insImm(2 + slot);
+    }
+
     // Get the right frame slots to use our slot index with.
     // See also UpvarArgTraits.
     static inline jsval* slots(JSStackFrame* fp) { return fp->argv; }
@@ -3360,6 +3365,11 @@ struct VarClosureTraits
     // See also UpvarVarTraits.
     static inline uint32 adj_slot(JSStackFrame* fp, uint32 slot) { return 3 + fp->argc + slot; }
 
+    static inline LIns* adj_slot_lir(LirWriter* lir, LIns* fp_ins, unsigned slot) {
+        LIns *argc_ins = lir->insLoad(LIR_ld, fp_ins, offsetof(JSStackFrame, argc));
+        return lir->ins2(LIR_add, lir->insImm(3 + slot), argc_ins);
+    }
+
     // See also UpvarVarTraits.
     static inline jsval* slots(JSStackFrame* fp) { return fp->slots; }
     static inline jsval* slots(JSObject* obj) {
@@ -3999,6 +4009,7 @@ TraceRecorder::determineSlotType(jsval* vp)
         } else {
             m = importTypeMap[nativeStackSlot(vp)];
         }
+        JS_ASSERT(m != TT_IGNORE);
     } else if (JSVAL_IS_OBJECT(*vp)) {
         if (JSVAL_IS_NULL(*vp))
             m = TT_NULL;
@@ -5166,11 +5177,13 @@ TraceRecorder::emitTreeCall(TreeFragment* inner, VMSideExit* exit, LIns* inner_s
 #endif
 
     /*
-     * Flush values from the tracker which could have been invalidated by the
-     * inner tree. This means variables local to this frame and global slots.
-     * It's safe to keep the offset cache around, we just want to force reloads.
+     * Clear anything from the tracker that the inner tree could have written.
+     * This includes the current frame (which has variables that are local in
+     * the inner tree), the entry frame (which can be written to when upvars
+     * are set), and the globals.
      */
-    clearFrameSlotsFromTracker(tracker);
+    clearEntryFrameSlotsFromTracker(tracker);
+    clearCurrentFrameSlotsFromTracker(tracker);
     SlotList& gslots = *tree->globalSlots;
     for (unsigned i = 0; i < gslots.length(); i++) {
         unsigned slot = gslots[i];
@@ -5178,43 +5191,17 @@ TraceRecorder::emitTreeCall(TreeFragment* inner, VMSideExit* exit, LIns* inner_s
         tracker.set(vp, NULL);
     }
 
-    /*
-     * Begin the complicated process of building a usable typemap by looking
-     * at all stack slots.
-     */
-    TypeMap stackTypeMap(NULL);
-    stackTypeMap.setLength(NativeStackSlots(cx, callDepth));
-    CaptureTypesVisitor visitor(cx, stackTypeMap.data());
-    VisitStackSlots(visitor, cx, callDepth);
-    JS_ASSERT(stackTypeMap.length() >= exit->numStackSlots);
-
-    /*
-     * The import map layout looks like:
-     * -------------
-     * Initial Frame   (callDepth=0)
-     * ...
-     * ...
-     * ...
-     * Current Frame   (callDepth=N)
-     * -------------
-     *
-     * Anything in between the innermost and outermost frames must be in the
-     * tracker at this point, so it's kind of pointless to make sure it's
-     * in the typemap. It'd be a pain to have separate typemaps for each
-     * "hole" though, so make one big one knowing that the holes won't be
-     * read.
-     *
-     * NOTE: It's important that setLength() doesn't mess up stuff below the
-     * current frame. It has to shrink safely.
-     */
-    importTypeMap.setLength(stackTypeMap.length());
-    unsigned startOfInnerFrame = stackTypeMap.length() - exit->numStackSlots;
+    /* Set stack slots from the innermost frame. */
+    importTypeMap.setLength(NativeStackSlots(cx, callDepth));
 #ifdef DEBUG
-    for (unsigned i = importStackSlots; i < startOfInnerFrame; i++)
-        stackTypeMap[i] = TT_IGNORE;
+    for (unsigned i = importStackSlots; i < importTypeMap.length(); i++)
+        importTypeMap[i] = TT_IGNORE;
 #endif
+    unsigned startOfInnerFrame = importTypeMap.length() - exit->numStackSlots;
     for (unsigned i = 0; i < exit->numStackSlots; i++)
         importTypeMap[startOfInnerFrame + i] = exit->stackTypeMap()[i];
+    importStackSlots = importTypeMap.length();
+    JS_ASSERT(importStackSlots == NativeStackSlots(cx, callDepth));
 
     /*
      * Bug 502604 - It is illegal to extend from the outer typemap without
@@ -5222,7 +5209,6 @@ TraceRecorder::emitTreeCall(TreeFragment* inner, VMSideExit* exit, LIns* inner_s
      */
     BuildGlobalTypeMapFromInnerTree(importTypeMap, exit);
 
-    importStackSlots = stackTypeMap.length();
     importGlobalSlots = importTypeMap.length() - importStackSlots;
     JS_ASSERT(importGlobalSlots == tree->globalSlots->length());
 
@@ -6142,6 +6128,19 @@ TraceRecorder::attemptTreeCall(TreeFragment* f, uintN& inlineCallCount)
 
     JSContext *localCx = cx;
 
+    // Refresh the import type map so the tracker can reimport values after the
+    // call with their correct types. The inner tree must not change the type of
+    // any variable in a frame above the current one (i.e., upvars). 
+    //
+    // Note that DetermineTypesVisitor may call determineSlotType, which may
+    // read from the (current, stale) import type map, but this is safe here.
+    // The reason is that determineSlotType will read the import type map only
+    // if there is not a tracker instruction for that value, which means that
+    // value has not been written yet, so that type map entry is up to date.
+    importTypeMap.setLength(NativeStackSlots(cx, callDepth));
+    DetermineTypesVisitor visitor(*this, importTypeMap.data());
+    VisitStackSlots(visitor, cx, callDepth);
+
     VMSideExit* innermostNestedGuard = NULL;
     VMSideExit* lr = ExecuteTree(cx, f, inlineCallCount, &innermostNestedGuard);
 
@@ -9802,8 +9801,11 @@ TraceRecorder::guardNotGlobalObject(JSObject* obj, LIns* obj_ins)
     return RECORD_CONTINUE;
 }
 
+// Helper for clearXEntryFrameSlotsFromTracker.
+// Clear out slots of the given frame in the NativeFrameTracker. All argument slots
+// are cleared. |nslots| local slots are cleared.
 JS_REQUIRES_STACK void
-TraceRecorder::clearFrameSlotsFromTracker(Tracker& which)
+TraceRecorder::clearFrameSlotsFromTracker(Tracker& which, JSStackFrame* fp, unsigned nslots)
 {
     /*
      * Clear out all slots of this frame in the nativeFrameTracker. Different
@@ -9812,7 +9814,6 @@ TraceRecorder::clearFrameSlotsFromTracker(Tracker& which)
      * we have to make sure we map those in to the cache with the right
      * offsets.
      */
-    JSStackFrame* fp = cx->fp;
     jsval* vp;
     jsval* vpstop;
 
@@ -9829,11 +9830,36 @@ TraceRecorder::clearFrameSlotsFromTracker(Tracker& which)
         which.set(&fp->argsobj, (LIns*)0);
     }
     vp = &fp->slots[0];
-    vpstop = &fp->slots[fp->script->nslots];
+    vpstop = &fp->slots[nslots];
     while (vp < vpstop)
         which.set(vp++, (LIns*)0);
 }
 
+JS_REQUIRES_STACK JSStackFrame*
+TraceRecorder::entryFrame() const
+{
+    JSStackFrame *fp = cx->fp;
+    for (unsigned i = 0; i < callDepth; ++i)
+        fp = fp->down;
+    return fp;
+}
+
+JS_REQUIRES_STACK void
+TraceRecorder::clearEntryFrameSlotsFromTracker(Tracker& which)
+{
+    JSStackFrame *fp = entryFrame();
+
+    // Clear only slots that are not also used by the next frame up.
+    clearFrameSlotsFromTracker(which, fp, fp->script->nfixed);
+}
+
+JS_REQUIRES_STACK void
+TraceRecorder::clearCurrentFrameSlotsFromTracker(Tracker& which)
+{
+    // Clear out all local slots.
+    clearFrameSlotsFromTracker(which, cx->fp, cx->fp->script->nslots);
+}
+
 /*
  * If we have created an |arguments| object for the frame, we must copy the
  * argument values into the object as properties in case it is used after
@@ -10065,7 +10091,7 @@ TraceRecorder::record_JSOP_RETURN()
     debug_only_printf(LC_TMTracer,
                       "returning from %s\n",
                       js_AtomToPrintableString(cx, cx->fp->fun->atom));
-    clearFrameSlotsFromTracker(nativeFrameTracker);
+    clearCurrentFrameSlotsFromTracker(nativeFrameTracker);
 
     return ARECORD_CONTINUE;
 }
@@ -11494,6 +11520,52 @@ TraceRecorder::setCallProp(JSObject *callobj, LIns *callobj_ins, JSScopeProperty
     else
         RETURN_STOP("can't trace special CallClass setter");
 
+    // Even though the frame is out of range, later we might be called as an
+    // inner trace such that the target variable is defined in the outer trace
+    // entry frame. In that case, we must store to the native stack area for
+    // that frame.
+
+    LIns *fp_ins = lir->insLoad(LIR_ldp, cx_ins, offsetof(JSContext, fp));
+    LIns *fpcallobj_ins = lir->insLoad(LIR_ldp, fp_ins, offsetof(JSStackFrame, callobj));
+    LIns *br1 = lir->insBranch(LIR_jf, lir->ins2(LIR_peq, fpcallobj_ins, callobj_ins), NULL);
+
+    // Case 1: storing to native stack area.
+
+    // Compute native stack slot and address offset we are storing to.
+    unsigned slot = uint16(sprop->shortid);
+    LIns *slot_ins;
+    if (sprop->setter == SetCallArg)
+        slot_ins = ArgClosureTraits::adj_slot_lir(lir, fp_ins, slot);
+    else
+        slot_ins = VarClosureTraits::adj_slot_lir(lir, fp_ins, slot);
+    LIns *offset_ins = lir->ins2(LIR_mul, slot_ins, INS_CONST(sizeof(double)));
+
+    // Guard that we are not changing the type of the slot we are storing to.
+    LIns *callstackBase_ins = lir->insLoad(LIR_ldp, lirbuf->state, 
+                                           offsetof(InterpState, callstackBase));
+    LIns *frameInfo_ins = lir->insLoad(LIR_ldp, callstackBase_ins, 0);
+    LIns *typemap_ins = lir->ins2(LIR_addp, frameInfo_ins, INS_CONSTWORD(sizeof(FrameInfo)));
+    LIns *type_ins = lir->insLoad(LIR_ldcb, 
+                                  lir->ins2(LIR_addp, typemap_ins, lir->ins_u2p(slot_ins)), 0);
+    JSTraceType type = getCoercedType(v);
+    if (type == TT_INT32 && !isPromoteInt(v_ins))
+        type = TT_DOUBLE;
+    guard(true,
+          addName(lir->ins2(LIR_eq, type_ins, lir->insImm(type)),
+                  "guard(type-stable set upvar)"),
+          BRANCH_EXIT);
+
+    // Store to the native stack slot.
+    LIns *stackBase_ins = lir->insLoad(LIR_ldp, lirbuf->state, 
+                                       offsetof(InterpState, stackBase));
+    LIns *storeValue_ins = isPromoteInt(v_ins) ? demote(lir, v_ins) : v_ins;
+    lir->insStorei(storeValue_ins, 
+                   lir->ins2(LIR_addp, stackBase_ins, lir->ins_u2p(offset_ins)), 0);
+    LIns *br2 = lir->insBranch(LIR_j, NULL, NULL);
+
+    // Case 2: calling builtin.
+    LIns *label1 = lir->ins0(LIR_label);
+    br1->setTarget(label1);
     LIns* args[] = {
         box_jsval(v, v_ins),
         INS_CONST(SPROP_USERID(sprop)),
@@ -11502,6 +11574,10 @@ TraceRecorder::setCallProp(JSObject *callobj, LIns *callobj_ins, JSScopeProperty
     };
     LIns* call_ins = lir->insCall(ci, args);
     guard(false, addName(lir->ins_eq0(call_ins), "guard(set upvar)"), STATUS_EXIT);
+
+    LIns *label2 = lir->ins0(LIR_label);
+    br2->setTarget(label2);
+    
     return RECORD_CONTINUE;
 }
 
@@ -14418,7 +14494,7 @@ TraceRecorder::record_JSOP_STOP()
     } else {
         rval_ins = INS_CONST(JSVAL_TO_SPECIAL(JSVAL_VOID));
     }
-    clearFrameSlotsFromTracker(nativeFrameTracker);
+    clearCurrentFrameSlotsFromTracker(nativeFrameTracker);
     return ARECORD_CONTINUE;
 }
 

js/src/jstracer.h

@@ -1289,7 +1289,10 @@ class TraceRecorder
                                                                              ExitType exitType);
     JS_REQUIRES_STACK RecordingStatus guardNotGlobalObject(JSObject* obj,
                                                              nanojit::LIns* obj_ins);
-    void clearFrameSlotsFromTracker(Tracker& which);
+    JS_REQUIRES_STACK JSStackFrame* entryFrame() const;
+    JS_REQUIRES_STACK void clearEntryFrameSlotsFromTracker(Tracker& which);
+    JS_REQUIRES_STACK void clearCurrentFrameSlotsFromTracker(Tracker& which);
+    JS_REQUIRES_STACK void clearFrameSlotsFromTracker(Tracker& which, JSStackFrame* fp, unsigned nslots);
     JS_REQUIRES_STACK void putArguments();
     JS_REQUIRES_STACK RecordingStatus guardCallee(jsval& callee);
     JS_REQUIRES_STACK JSStackFrame      *guardArguments(JSObject *obj, nanojit::LIns* obj_ins,

js/src/trace-test/tests/basic/bug532568-2.js

@@ -0,0 +1,20 @@
+var before = '';
+var after = '';
+
+function g(b) {
+  for (var i = 0; i < 10; ++i) {
+  }
+}
+
+function f(xa_arg) {
+  var xa = xa_arg;
+  for (var i = 0; i < 5; ++i) {
+    var j = i + xa[i];
+    before += j + ',';
+    g(); 
+    after += j + ',';
+  }
+}
+
+f([ 0, 1, 2, 3, 4 ]);
+assertEq(before, after);

js/src/trace-test/tests/basic/bug532568.js

@@ -0,0 +1,19 @@
+var actual = "";
+
+function f() {
+  var x = 10;
+  
+  var g = function(p) {
+    for (var i = 0; i < 10; ++i)
+      x = p + i;
+  }
+  
+  for (var i = 0; i < 10; ++i) {
+    g(100 * i + x);
+    actual += x + ',';
+  }
+}
+
+f();
+
+assertEq(actual, "19,128,337,646,1055,1564,2173,2882,3691,4600,");