fix materializeRef -> stealRef

src/asm_writing/rewriter.cpp

@@ -1505,7 +1505,8 @@ bool Rewriter::finishAssembly(int continue_offset) {
 void Rewriter::commitReturning(RewriterVar* var) {
     STAT_TIMER(t0, "us_timer_rewriter", 10);
 
-    assert(var->vrefcount == 0);
+    assert(var->vrefcount == 1);
+    var->stealRef();
 
     addAction([=]() {
         assembler->comment("commitReturning");
@@ -1513,6 +1514,8 @@ void Rewriter::commitReturning(RewriterVar* var) {
         var->bumpUse();
     }, { var }, ActionType::NORMAL);
 
+    var->decvref();
+
     commit();
 }
 

src/asm_writing/rewriter.h

@@ -270,19 +270,36 @@ public:
         return this;
     }
 
-    RewriterVar* materializeVref() {
+    // Steal a ref (not a vref) from this object.
+    // If there aren't any owned refs available, one will be created.
+    // Doesn't change the vrefcount, but the vref that stealRef acts on
+    // becomes a borrowed vref.  One can not use this RewriterVar after stealRef
+    // except for the operation that will actually steal the ref.
+    // If that is necessary, an extra vref should be taken out beforehand.
+    //
+    // Typical usage should look like:
+    //   var->stealRef();
+    //   functionThatStealsRef(var);
+    //   var->decvref();
+    //
+    // If you need to use var after functionThatStealsRef, the whole snippet
+    // needs to be surrounded in an incvref/decvref.
+    //
+    // TODO: I still think I can find a better API for this.  Maybe something like
+    // var->materializeRefFor([&](){ functionThatStealsRef(var); });
+    //
+    RewriterVar* stealRef() {
         assert(reftype == RefType::OWNED || reftype == RefType::BORROWED);
         assert(vrefcount > 0);
-        vrefcount--;
 
-        if (reftype == RefType::OWNED && vrefcount == 0) {
+        if (reftype == RefType::OWNED && vrefcount == 1) {
             // handoff, do nothing;
-            // we skip this incref because we skipped the decref in decvref (by touching vrefcount manually)
+            reftype = RefType::BORROWED;
         } else {
             incref();
         }
 
-        if (vrefcount == 0) {
+        if (vrefcount == 1) {
             // 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.

src/codegen/baseline_jit.cpp

@@ -339,6 +339,8 @@ RewriterVar* JitFragmentWriter::emitGetItem(AST_expr* node, RewriterVar* value,
 
 RewriterVar* JitFragmentWriter::emitGetLocal(InternedString s, int vreg) {
     assert(vreg >= 0);
+    // TODO Can we use BORROWED here? Not sure if there are cases when we can't rely on borrowing the ref
+    // from the vregs array.  Safer like this.
     RewriterVar* val_var = vregs_array->getAttr(vreg * 8)->setType(RefType::OWNED);
     addAction([=]() { _emitGetLocal(val_var, s.c_str()); }, { val_var }, ActionType::NORMAL);
     return val_var;
@@ -512,9 +514,9 @@ void JitFragmentWriter::emitReturn(RewriterVar* v) {
             v.second->decvref();
     }
 
-    v->materializeVref();
-
+    v->stealRef();
     addAction([=]() { _emitReturn(v); }, { v }, ActionType::NORMAL);
+    v->decvref();
 }
 
 void JitFragmentWriter::emitSetAttr(AST_expr* node, RewriterVar* obj, BoxedString* s, RewriterVar* attr) {
@@ -563,9 +565,12 @@ void JitFragmentWriter::emitSetLocal(InternedString s, int vreg, bool set_closur
              v);
     } else {
         RewriterVar* prev = vregs_array->getAttr(8 * vreg)->setType(RefType::OWNED);
-        v->materializeVref();
+        v->stealRef();
         vregs_array->setAttr(8 * vreg, v);
+        v->decvref();
         // XXX: this either needs to be an xdecref or we should check liveness analysis.
+        // keeping it like this for some performance testing but we need to fix this (will segfault
+        // on non-osr entries to the bjit)
         prev->decvref();
         //prev->xdecvref();
     }

src/runtime/objmodel.cpp

@@ -1022,9 +1022,10 @@ void Box::setattr(BoxedString* attr, Box* val, SetattrRewriteArgs* rewrite_args)
                     r_hattrs->getAttr(offset * sizeof(Box*) + offsetof(HCAttrs::AttrList, attrs))
                         ->setType(RefType::OWNED)
                         ->decvref();
-                    rewrite_args->attrval->materializeVref();
+                    rewrite_args->attrval->stealRef();
                     r_hattrs->setAttr(offset * sizeof(Box*) + offsetof(HCAttrs::AttrList, attrs),
                                       rewrite_args->attrval);
+                    rewrite_args->attrval->decvref();
 
                     rewrite_args->out_success = true;
                 }
@@ -4859,7 +4860,6 @@ extern "C" Box* binop(Box* lhs, Box* rhs, int op_type) {
         } else {
             rewriter->getArg(0)->decvref();
             rewriter->getArg(1)->decvref();
-            rewrite_args.out_rtn->materializeVref();
             rewriter->commitReturning(rewrite_args.out_rtn);
         }
     } else {
@@ -6406,7 +6406,6 @@ extern "C" Box* getGlobal(Box* globals, BoxedString* name) {
                 if (r) {
                     if (rewriter.get()) {
                         RewriterVar* r_rtn = rewrite_args.getReturn(ReturnConvention::HAS_RETURN);
-                        r_rtn->materializeVref();
                         rewriter->commitReturning(r_rtn);
                     }