Fix deinitFrame on deopt

Our convention is that on deopt, the callee is responsible for calling deinitFrame().
This is tricky, since for OSR our convention is the opposite, that the caller calls deinitFrame().

This means that if we OSR and then deopt(), the top and bottom frames both think they should call
deinitFrame() (since one is the caller of an OSR and the other is the callee of a deopt).

This commit fixes + extends the "disable deinitFrame for this frame" approach we kind of had.
For performance, deinitFrame() stays the same, but any site that might have its deinitFrame disabled
(namely, in the interpreter), it should call deinitFrameMaybe() instead.

src/codegen/ast_interpreter.cpp

@@ -247,9 +247,16 @@ ASTInterpreter::ASTInterpreter(FunctionMetadata* md, Box** vregs, int num_vregs,
     if (deopt_frame_info) {
         // copy over all fields and clear the deopt frame info
         frame_info = *deopt_frame_info;
+
+        // Well, don't actually copy over the vregs.  We'll deal with them separately
+        // (using the locals dict), so just clear out and decref the old ones:
+        frame_info.vregs = NULL;
+        frame_info.num_vregs = 0;
         for (int i = 0; i < deopt_frame_info->num_vregs; ++i)
             Py_XDECREF(deopt_frame_info->vregs[i]);
-        memset(deopt_frame_info, 0, sizeof(*deopt_frame_info));
+
+        // We are taking responsibility for calling deinit:
+        deopt_frame_info->disableDeinit(&this->frame_info);
     }
 
     frame_info.vregs = vregs;
@@ -1853,7 +1860,7 @@ const void* interpreter_instr_addr = (void*)&executeInnerAndSetupFrame;
 extern "C" Box* executeInnerFromASM(ASTInterpreter& interpreter, CFGBlock* start_block, AST_stmt* start_at) {
     initFrame(interpreter.getFrameInfo());
     Box* rtn = ASTInterpreter::executeInner(interpreter, start_block, start_at);
-    deinitFrame(interpreter.getFrameInfo());
+    deinitFrameMaybe(interpreter.getFrameInfo());
     return rtn;
 }
 
@@ -2086,7 +2093,7 @@ extern "C" Box* astInterpretDeoptFromASM(FunctionMetadata* md, AST_expr* after_e
     // We need to remove the old python frame created in the LLVM tier otherwise we would have a duplicate frame because
     // the interpreter will set the new state before executing the first statement.
     RELEASE_ASSERT(cur_thread_state.frame_info == frame_state.frame_info, "");
-    cur_thread_state.frame_info = frame_state.frame_info->back;
+    cur_thread_state.frame_info = interpreter.getFrameInfo()->back;
 
     Box* v = ASTInterpreter::execute(interpreter, start_block, starting_statement);
     return v ? v : incref(None);

src/codegen/irgen/irgenerator.cpp

@@ -841,6 +841,9 @@ private:
         curblock = deopt_bb;
         emitter.getBuilder()->SetInsertPoint(curblock);
         llvm::Value* v = emitter.createDeopt(current_statement, (AST_expr*)node, node_value);
+        // TODO: this might not be necessary since it should never fire?
+        if (!irstate->getCurFunction()->entry_descriptor)
+            emitter.getBuilder()->CreateCall(g.funcs.deinitFrameMaybe, irstate->getFrameInfoVar());
         llvm::Instruction* ret_inst = emitter.getBuilder()->CreateRet(v);
         irstate->getRefcounts()->refConsumed(v, ret_inst);
 
@@ -2426,6 +2429,8 @@ private:
             emitter.getBuilder()->CreateCall(l_free, malloc_save);
         }
 
+        if (!irstate->getCurFunction()->entry_descriptor)
+            emitter.getBuilder()->CreateCall(g.funcs.deinitFrame, irstate->getFrameInfoVar());
         emitter.getBuilder()->CreateRet(rtn);
 
         emitter.getBuilder()->SetInsertPoint(starting_block);
@@ -3006,7 +3011,8 @@ public:
                     emitter.getBuilder()->CreateCall(g.funcs.reraiseCapiExcAsCxx);
                     emitter.getBuilder()->CreateUnreachable();
                 } else {
-                    emitter.getBuilder()->CreateCall(g.funcs.deinitFrame, irstate->getFrameInfoVar());
+                    if (!irstate->getCurFunction()->entry_descriptor)
+                        emitter.getBuilder()->CreateCall(g.funcs.deinitFrame, irstate->getFrameInfoVar());
                     emitter.getBuilder()->CreateRet(getNullPtr(g.llvm_value_type_ptr));
                 }
             } else {
@@ -3095,7 +3101,8 @@ public:
             irstate->getRefcounts()->refConsumed(exc_type, call_inst);
             irstate->getRefcounts()->refConsumed(exc_value, call_inst);
             irstate->getRefcounts()->refConsumed(exc_traceback, call_inst);
-            emitter.getBuilder()->CreateCall(g.funcs.deinitFrame, irstate->getFrameInfoVar());
+            if (!irstate->getCurFunction()->entry_descriptor)
+                emitter.getBuilder()->CreateCall(g.funcs.deinitFrame, irstate->getFrameInfoVar());
             emitter.getBuilder()->CreateRet(getNullPtr(g.llvm_value_type_ptr));
         } else {
             // auto call_inst = emitter.createCall3(UnwindInfo(unw_info.current_stmt, NO_CXX_INTERCEPTION),

src/codegen/irgen/refcounts.cpp

@@ -651,8 +651,12 @@ void RefcountTracker::addRefcounts(IRGenState* irstate) {
         while (s->elements().size() > 0 && llvm::isa<llvm::StructType>(s->elements()[0]))
             s = llvm::cast<llvm::StructType>(s->elements()[0]);
 
+        if (isa<ConstantPointerNull>(v))
+            return;
+
         bool ok_type = false;
-        if (s->elements().size() >= 2 && s->elements()[0] == g.i64 && s->elements()[1] == g.llvm_class_type_ptr) {
+        if (s->elements().size() >= 2 + REFCOUNT_IDX && s->elements()[REFCOUNT_IDX] == g.i64
+            && s->elements()[REFCOUNT_IDX + 1] == g.llvm_class_type_ptr) {
             // printf("This looks likes a class\n");
             ok_type = true;
         }

src/codegen/runtime_hooks.cpp

@@ -198,6 +198,7 @@ void initGlobalFuncs(GlobalState& g) {
     GET(createSet);
     GET(initFrame);
     GET(deinitFrame);
+    GET(deinitFrameMaybe);
     GET(makePendingCalls);
     GET(setFrameExcInfo);
 

src/codegen/runtime_hooks.h

@@ -34,8 +34,8 @@ struct GlobalFuncs {
 
     llvm::Value* boxInt, *unboxInt, *boxFloat, *unboxFloat, *createFunctionFromMetadata, *getFunctionMetadata,
         *boxInstanceMethod, *boxBool, *unboxBool, *createTuple, *createDict, *createList, *createSlice,
-        *createUserClass, *createClosure, *createGenerator, *createSet, *initFrame, *deinitFrame, *makePendingCalls,
-        *setFrameExcInfo;
+        *createUserClass, *createClosure, *createGenerator, *createSet, *initFrame, *deinitFrame, *deinitFrameMaybe,
+        *makePendingCalls, *setFrameExcInfo;
     llvm::Value* getattr, *getattr_capi, *setattr, *delattr, *delitem, *delGlobal, *nonzero, *binop, *compare,
         *augbinop, *unboxedLen, *getitem, *getitem_capi, *getclsattr, *getGlobal, *setitem, *unaryop, *import,
         *importFrom, *importStar, *repr, *exceptionMatches, *yield, *getiterHelper, *hasnext, *setGlobal, *apply_slice;

src/core/types.h

@@ -1001,6 +1001,12 @@ struct FrameInfo {
 
     BORROWED(Box*) updateBoxedLocals();
 
+    static FrameInfo* const NO_DEINIT;
+
+    // Calling disableDeinit makes future deinitFrameMaybe() frames not call deinitFrame().
+    // For use by deopt(), which takes over deinit responsibility for its caller.
+    void disableDeinit(FrameInfo* replacement_frame);
+
     FrameInfo(ExcInfo exc)
         : exc(exc),
           boxedLocals(NULL),

src/runtime/frame.cpp

@@ -229,7 +229,37 @@ extern "C" void initFrame(FrameInfo* frame_info) {
     cur_thread_state.frame_info = frame_info;
 }
 
+FrameInfo* const FrameInfo::NO_DEINIT = (FrameInfo*)-2; // not -1 to not match memset(-1)
+
+void FrameInfo::disableDeinit(FrameInfo* replacement_frame) {
+    assert(replacement_frame->back == this->back);
+    assert(replacement_frame->frame_obj == this->frame_obj);
+
+    if (this->frame_obj) {
+        assert(this->frame_obj->frame_info == this);
+        this->frame_obj->frame_info = replacement_frame;
+    }
+
+#ifndef NDEBUG
+    // First, make sure this doesn't get used for anything else:
+    memset(this, -1, sizeof(*this));
+#endif
+
+    // Kinda hacky but maybe worth it to not store any extra bits:
+    back = NO_DEINIT;
+}
+
+extern "C" void deinitFrameMaybe(FrameInfo* frame_info) {
+    // Note: this has to match FrameInfo::disableDeinit
+    if (frame_info->back != FrameInfo::NO_DEINIT)
+        deinitFrame(frame_info);
+}
+
 extern "C" void deinitFrame(FrameInfo* frame_info) {
+    // This can fire if we have a call to deinitFrame() that should be to deinitFrameMaybe() instead
+    assert(frame_info->back != FrameInfo::NO_DEINIT);
+
+    assert(cur_thread_state.frame_info == frame_info);
     cur_thread_state.frame_info = frame_info->back;
     BoxedFrame* frame = frame_info->frame_obj;
     if (frame) {
@@ -239,6 +269,7 @@ extern "C" void deinitFrame(FrameInfo* frame_info) {
 
     assert(frame_info->vregs || frame_info->num_vregs == 0);
     int num_vregs = frame_info->num_vregs;
+    assert(num_vregs >= 0);
 
     decrefArray<true>(frame_info->vregs, num_vregs);
 

src/runtime/inline/link_forcer.cpp

@@ -73,6 +73,7 @@ void force() {
     FORCE(decodeUTF8StringPtr);
     FORCE(initFrame);
     FORCE(deinitFrame);
+    FORCE(deinitFrameMaybe);
     FORCE(makePendingCalls);
     FORCE(setFrameExcInfo);
 

src/runtime/types.h

@@ -1370,6 +1370,7 @@ BORROWED(Box*) getFrame(FrameInfo* frame_info);
 BORROWED(Box*) getFrame(int depth);
 void frameInvalidateBack(BoxedFrame* frame);
 extern "C" void deinitFrame(FrameInfo* frame_info);
+extern "C" void deinitFrameMaybe(FrameInfo* frame_info);
 int frameinfo_traverse(FrameInfo* frame_info, visitproc visit, void* arg) noexcept;
 extern "C" void initFrame(FrameInfo* frame_info);
 extern "C" void setFrameExcInfo(FrameInfo* frame_info, STOLEN(Box*) type, STOLEN(Box*) value, STOLEN(Box*) tb);