I think I understand what I was doing 6 weeks ago... let's see if I'm right

src/asm_writing/rewriter.cpp

@@ -1205,6 +1205,8 @@ void RewriterVar::bumpUse() {
         }
 
         this->locations.clear();
+    } else {
+        assert(vrefcount > 0);
     }
 }
 

src/asm_writing/rewriter.h

@@ -251,8 +251,15 @@ public:
         assert(vrefcount > 0);
         vrefcount--;
 
-        if (reftype == RefType::OWNED && vrefcount == 0)
-            decref();
+        if (vrefcount == 0) {
+            // Assert that there are no more uses of the this at the machine-code level.
+            // This is kind of weird to cross this abstraction boundary like this, but
+            // I think it's a good safety net.
+            assert(next_use == uses.size());
+
+            if (reftype == RefType::OWNED)
+                decref();
+        }
         return this;
     }
 
@@ -262,10 +269,17 @@ public:
         vrefcount--;
 
         if (reftype == RefType::OWNED && vrefcount == 0) {
-            // handoff, do nothing
+            // handoff, do nothing;
+            // we skip this incref because we skipped the decref in decvref (by touching vrefcount manually)
         } else {
             incref();
         }
+
+        if (vrefcount == 0) {
+            // See the comment in decvref.
+            // This is similar, except that we want there to be exactly one more use of this variable:
+            // whatever consumes the vref.  We want that to happen after the materializeVref call.
+        }
         return this;
     }
 
@@ -274,16 +288,17 @@ public:
 
     bool isConstant() { return is_constant; }
 
+protected:
+    void incref();
+    void decref();
+    void xdecref();
+
 private:
     Rewriter* rewriter;
 
     llvm::SmallVector<Location, 4> locations;
     bool isInLocation(Location l);
 
-    void incref();
-    void decref();
-    void xdecref();
-
     // uses is a vector of the indices into the Rewriter::actions vector
     // indicated the actions that use this variable.
     // During the assembly-emitting phase, next_use is used to keep track of the next

src/codegen/baseline_jit.cpp

@@ -328,8 +328,8 @@ RewriterVar* JitFragmentWriter::emitGetGlobal(Box* global, BoxedString* s) {
     }
 
     RewriterVar* args[] = { NULL, NULL };
-    args[0] = imm(global)->asBorrowed()->decvref();
-    args[1] = imm(s)->asBorrowed()->decvref();
+    args[0] = imm(global);
+    args[1] = imm(s);
     return emitPPCall((void*)getGlobal, args, 2, 512)->setType(RefType::OWNED);
 }
 
@@ -550,7 +550,7 @@ void JitFragmentWriter::emitSetItemName(BoxedString* s, RewriterVar* v) {
     emitSetItem(emitGetBoxedLocals(), imm(s), v);
 }
 
-void JitFragmentWriter::emitSetLocal(InternedString s, int vreg, bool set_closure, RewriterVar* v) {
+void JitFragmentWriter::emitSetLocal(InternedString s, int vreg, bool set_closure, STOLEN(RewriterVar*) v) {
     assert(vreg >= 0);
     if (set_closure) {
         assert(0 && "check refcounting");
@@ -566,7 +566,7 @@ void JitFragmentWriter::emitSetLocal(InternedString s, int vreg, bool set_closur
         vregs_array->setAttr(8 * vreg, v);
         // XXX: this either needs to be an xdecref or we should check liveness analysis.
         prev->decvref();
-        //prev->xdecref();
+        //prev->xdecvref();
     }
 }
 

src/codegen/baseline_jit.h

@@ -264,7 +264,7 @@ public:
     void emitSetGlobal(Box* global, BoxedString* s, RewriterVar* v);
     void emitSetItemName(BoxedString* s, RewriterVar* v);
     void emitSetItem(RewriterVar* target, RewriterVar* slice, RewriterVar* value);
-    void emitSetLocal(InternedString s, int vreg, bool set_closure, RewriterVar* v);
+    void emitSetLocal(InternedString s, int vreg, bool set_closure, STOLEN(RewriterVar*) v);
     void emitSideExit(STOLEN(RewriterVar*) v, Box* cmp_value, CFGBlock* next_block);
     void emitUncacheExcInfo();
 

src/runtime/objmodel.cpp

@@ -3564,6 +3564,22 @@ void rearrangeArgumentsInternal(ParamReceiveSpec paramspec, const ParamNames* pa
                 Py_INCREF(oargs[i]);
             }
         }
+        if (rewrite_args) {
+            // TODO should probably rethink the whole vrefcounting thing here.
+            // which ones actually need new vrefs?
+            if (num_output_args >= 1)
+                rewrite_args->arg1->incvref();
+            if (num_output_args >= 2)
+                rewrite_args->arg2->incvref();
+            if (num_output_args >= 3)
+                rewrite_args->arg3->incvref();
+            if (num_output_args >= 3) {
+                RELEASE_ASSERT(0, "not sure what to do here\n");
+                for (int i = 0; i < num_output_args - 3; i++) {
+                    rewrite_args->args->getAttr(i * sizeof(Box*))->incref();
+                }
+            }
+        }
         return;
     }
 
@@ -4729,8 +4745,6 @@ Box* binopInternal(Box* lhs, Box* rhs, int op_type, bool inplace, BinopRewriteAr
     if (rewrite_args) {
         CallattrRewriteArgs srewrite_args(rewrite_args->rewriter, rewrite_args->lhs, rewrite_args->destination);
         srewrite_args.arg1 = rewrite_args->rhs;
-        rewrite_args->lhs->incvref();
-        rewrite_args->rhs->incvref();
         lrtn = callattrInternal1<CXX, REWRITABLE>(lhs, op_name, CLASS_ONLY, &srewrite_args, ArgPassSpec(1), rhs);
 
         if (!srewrite_args.isSuccessful()) {
@@ -4754,8 +4768,6 @@ Box* binopInternal(Box* lhs, Box* rhs, int op_type, bool inplace, BinopRewriteAr
     if (lrtn) {
         if (lrtn != NotImplemented) {
             if (rewrite_args) {
-                rewrite_args->lhs->decvref();
-                rewrite_args->rhs->decvref();
                 assert(rewrite_args->out_rtn);
                 rewrite_args->out_success = true;
             }