Fixed regression with recursive scripts missing a return (bug 552196, r=gal).
authorDavid Anderson <danderson@mozilla.com>
Fri, 26 Mar 2010 09:58:33 -0500
changeset 40375 9a0a4c64da0a2168981d3fefb4e5c0c21952eefe
parent 40374 23442b4a6e2090c89c2b2745925303f0a4731f15
child 40376 4a32bcd70c494120045b4c7fd1f4ba64a96245aa
push id12610
push userrsayre@mozilla.com
push dateMon, 05 Apr 2010 17:26:41 +0000
treeherdermozilla-central@1942c0b4e101 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersgal
bugs552196
milestone1.9.3a3pre
Fixed regression with recursive scripts missing a return (bug 552196, r=gal).
js/src/jsrecursion.cpp
js/src/trace-test/tests/basic/bug552196.js
--- a/js/src/jsrecursion.cpp
+++ b/js/src/jsrecursion.cpp
@@ -304,19 +304,18 @@ TraceRecorder::upRecursion()
     /*
      * Now it's time to try and close the loop. Get a special exit that points
      * at the down frame, after the return has been propagated up.
      */
     exit = downSnapshot(fi);
 
     LIns* rval_ins;
     if (*cx->fp->regs->pc == JSOP_RETURN) {
-        rval_ins = (!anchor || anchor->exitType != RECURSIVE_SLURP_FAIL_EXIT) ?
-                   get(&stackval(-1)) :
-                   NULL;
+        JS_ASSERT(!anchor || anchor->exitType != RECURSIVE_SLURP_FAIL_EXIT);
+        rval_ins = get(&stackval(-1));
         JS_ASSERT(rval_ins);
     } else {
         rval_ins = INS_VOID();
     }
 
     TraceType returnType = exit->stackTypeMap()[downPostSlots];
     if (returnType == TT_INT32) {
         JS_ASSERT(*cx->fp->regs->pc == JSOP_RETURN);
@@ -350,16 +349,37 @@ class SlurpInfo
 {
 public:
     unsigned curSlot;
     TraceType* typeMap;
     VMSideExit* exit;
     unsigned slurpFailSlot;
 };
 
+/*
+ * The three types of anchors that can cause this type of trace to be built are:
+ *   RECURSIVE_SLURP_MISMATCH_EXIT
+ *   RECURSIVE_SLURP_FAIL_EXIT
+ *   RECURSIVE_EMPTY_RP_EXIT
+ *
+ * EMPTY_RP means that recursion is unwinding, but there are no more frames.
+ * This triggers a "slurp trace" to be built. A slurp trace does three things:
+ *   1) Checks to see if cx->fp returns to the same point the recursive trace
+ *      is trying to unwind to.
+ *   2) Pops the inline frame cx->fp, such that cx->fp is now cx->fp->down.
+ *   3) Converts the new top-frame slots/sp into the tracer frame.
+ *
+ * SLURP_MISMATCH means that while trying to convert an interpreter frame,
+ * it is owned by the same script, but does not return to the same pc. At this
+ * point the frame has not been popped yet.
+ *
+ * SLURP_FAIL means that the interpreter frame has been popped, the return
+ * value has been written to the native stack, but there was a type mismatch
+ * while unboxing the interpreter slots.
+ */
 JS_REQUIRES_STACK AbortableRecordingStatus
 TraceRecorder::slurpDownFrames(jsbytecode* return_pc)
 {
     /* Missing - no go */
     if (cx->fp->argc != cx->fp->fun->nargs)
         RETURN_STOP_A("argc != nargs");
 
     LIns* argv_ins;
@@ -496,22 +516,34 @@ TraceRecorder::slurpDownFrames(jsbytecod
      * In all other cases, the return value lives in the tracker and it can be
      * grabbed safely.
      */
     LIns* rval_ins;
     intptr_t offset = exit->sp_adj - sizeof(double);
     TraceType returnType = exit->stackTypeMap()[downPostSlots];
 
     if (!anchor || anchor->exitType != RECURSIVE_SLURP_FAIL_EXIT) {
-        rval_ins = get(&stackval(-1));
-        if (returnType == TT_INT32) {
-            JS_ASSERT(determineSlotType(&stackval(-1)) == TT_INT32);
-            JS_ASSERT(isPromoteInt(rval_ins));
-            rval_ins = demote(lir, rval_ins);
+        /*
+         * It is safe to read cx->fp->regs->pc here because the frame hasn't
+         * been popped yet. We're guaranteed to have a return or stop.
+         */
+        JSOp op = JSOp(*cx->fp->regs->pc);
+        JS_ASSERT(op == JSOP_RETURN || op == JSOP_STOP);
+
+        if (op == JSOP_RETURN) {
+            rval_ins = get(&stackval(-1));
+            if (returnType == TT_INT32) {
+                JS_ASSERT(determineSlotType(&stackval(-1)) == TT_INT32);
+                JS_ASSERT(isPromoteInt(rval_ins));
+                rval_ins = demote(lir, rval_ins);
+            }
+        } else {
+            rval_ins = INS_VOID();
         }
+
         /*
          * The return value must be written out early, before slurping can fail,
          * otherwise it will not be available when there's a type mismatch.
          */
         lir->insStorei(rval_ins, lirbuf->sp, offset, ACC_STACK);
     } else {
         switch (returnType)
         {
new file mode 100644
--- /dev/null
+++ b/js/src/trace-test/tests/basic/bug552196.js
@@ -0,0 +1,12 @@
+(Function("\
+  for (a = 0; a < 5; a++)\n\
+  (function f(b) {\n\
+    if (b > 0) {\n\
+      f(b - 1)\n\
+    }\n\
+  })\n\
+  (3)\n\
+"))()
+
+/* Don't assert. */
+