Merge pull request #592 from undingen/generator_gc

free generator even if the don't exit

src/codegen/ast_interpreter.cpp

@@ -53,6 +53,31 @@ namespace pyston {
 
 namespace {
 
+static BoxedClass* astinterpreter_cls;
+
+class ASTInterpreter;
+
+// Map from stack frame pointers for frames corresponding to ASTInterpreter::execute() to the ASTInterpreter handling
+// them. Used to look up information about that frame. This is used for getting tracebacks, for CPython introspection
+// (sys._getframe & co), and for GC scanning.
+static std::unordered_map<void*, ASTInterpreter*> s_interpreterMap;
+static_assert(THREADING_USE_GIL, "have to make the interpreter map thread safe!");
+
+class RegisterHelper {
+private:
+    void* frame_addr;
+    ASTInterpreter* interpreter;
+
+public:
+    RegisterHelper(ASTInterpreter* interpreter, void* frame_addr);
+    ~RegisterHelper();
+
+    static void deregister(void* frame_addr) {
+        assert(s_interpreterMap.count(frame_addr));
+        s_interpreterMap.erase(frame_addr);
+    }
+};
+
 union Value {
     bool b;
     int64_t n;
@@ -68,7 +93,7 @@ union Value {
     }
 };
 
-class ASTInterpreter {
+class ASTInterpreter : public Box {
 public:
     typedef ContiguousMap<InternedString, Box*> SymMap;
 
@@ -149,8 +174,11 @@ private:
 
     // This is either a module or a dict
     Box* globals;
+    void* frame_addr; // used to clear entry inside the s_interpreterMap on destruction
 
 public:
+    DEFAULT_CLASS_SIMPLE(astinterpreter_cls);
+
     AST_stmt* getCurrentStatement() {
         assert(current_inst);
         return current_inst;
@@ -175,7 +203,16 @@ public:
     void setFrameInfo(const FrameInfo* frame_info);
     void setGlobals(Box* globals);
 
-    void gcVisit(GCVisitor* visitor);
+    static void gcHandler(GCVisitor* visitor, Box* box);
+    static void simpleDestructor(Box* box) {
+        ASTInterpreter* inter = (ASTInterpreter*)box;
+        assert(inter->cls == astinterpreter_cls);
+        if (inter->frame_addr)
+            RegisterHelper::deregister(inter->frame_addr);
+        inter->~ASTInterpreter();
+    }
+
+    friend class RegisterHelper;
 };
 
 void ASTInterpreter::addSymbol(InternedString name, Box* value, bool allow_duplicates) {
@@ -215,17 +252,17 @@ void ASTInterpreter::setGlobals(Box* globals) {
     this->globals = globals;
 }
 
-void ASTInterpreter::gcVisit(GCVisitor* visitor) {
-    visitor->visitRange((void* const*)&sym_table.vector()[0], (void* const*)&sym_table.vector()[sym_table.size()]);
-    if (passed_closure)
-        visitor->visit(passed_closure);
-    if (created_closure)
-        visitor->visit(created_closure);
-    if (generator)
-        visitor->visit(generator);
-    if (frame_info.boxedLocals)
-        visitor->visit(frame_info.boxedLocals);
-    visitor->visit(globals);
+void ASTInterpreter::gcHandler(GCVisitor* visitor, Box* box) {
+    boxGCHandler(visitor, box);
+
+    ASTInterpreter* interp = (ASTInterpreter*)box;
+    auto&& vec = interp->sym_table.vector();
+    visitor->visitRange((void* const*)&vec[0], (void* const*)&vec[interp->sym_table.size()]);
+    visitor->visit(interp->passed_closure);
+    visitor->visit(interp->created_closure);
+    visitor->visit(interp->generator);
+    visitor->visit(interp->globals);
+    interp->frame_info.gcVisit(visitor);
 }
 
 ASTInterpreter::ASTInterpreter(CompiledFunction* compiled_function)
@@ -240,7 +277,8 @@ ASTInterpreter::ASTInterpreter(CompiledFunction* compiled_function)
       created_closure(0),
       generator(0),
       edgecount(0),
-      frame_info(ExcInfo(NULL, NULL, NULL)) {
+      frame_info(ExcInfo(NULL, NULL, NULL)),
+      frame_addr(0) {
 
     CLFunction* f = compiled_function->clfunc;
     if (!source_info->cfg)
@@ -279,25 +317,16 @@ void ASTInterpreter::initArguments(int nargs, BoxedClosure* _closure, BoxedGener
     }
 }
 
-// Map from stack frame pointers for frames corresponding to ASTInterpreter::execute() to the ASTInterpreter handling
-// them. Used to look up information about that frame. This is used for getting tracebacks, for CPython introspection
-// (sys._getframe & co), and for GC scanning.
-static std::unordered_map<void*, ASTInterpreter*> s_interpreterMap;
-static_assert(THREADING_USE_GIL, "have to make the interpreter map thread safe!");
-
-class RegisterHelper {
-private:
-    void* frame_addr;
+RegisterHelper::RegisterHelper(ASTInterpreter* interpreter, void* frame_addr)
+    : frame_addr(frame_addr), interpreter(interpreter) {
+    interpreter->frame_addr = frame_addr;
+    s_interpreterMap[frame_addr] = interpreter;
+}
 
-public:
-    RegisterHelper(ASTInterpreter* interpreter, void* frame_addr) : frame_addr(frame_addr) {
-        s_interpreterMap[frame_addr] = interpreter;
-    }
-    ~RegisterHelper() {
-        assert(s_interpreterMap.count(frame_addr));
-        s_interpreterMap.erase(frame_addr);
-    }
-};
+RegisterHelper::~RegisterHelper() {
+    interpreter->frame_addr = nullptr;
+    deregister(frame_addr);
+}
 
 Value ASTInterpreter::execute(ASTInterpreter& interpreter, CFGBlock* start_block, AST_stmt* start_at) {
     STAT_TIMER(t0, "us_timer_astinterpreter_execute");
@@ -382,7 +411,7 @@ void ASTInterpreter::doStore(AST_expr* node, Value value) {
         doStore(name->id, value);
     } else if (node->type == AST_TYPE::Attribute) {
         AST_Attribute* attr = (AST_Attribute*)node;
-        setattr(visit_expr(attr->value).o, attr->attr.c_str(), value.o);
+        pyston::setattr(visit_expr(attr->value).o, attr->attr.c_str(), value.o);
     } else if (node->type == AST_TYPE::Tuple) {
         AST_Tuple* tuple = (AST_Tuple*)node;
         Box** array = unpackIntoArray(value.o, tuple->elts.size());
@@ -897,7 +926,7 @@ Value ASTInterpreter::visit_delete(AST_Delete* node) {
             }
             case AST_TYPE::Attribute: {
                 AST_Attribute* attr = (AST_Attribute*)target_;
-                delattr(visit_expr(attr->value).o, attr->attr.c_str());
+                pyston::delattr(visit_expr(attr->value).o, attr->attr.c_str());
                 break;
             }
             case AST_TYPE::Name: {
@@ -1237,7 +1266,7 @@ Value ASTInterpreter::visit_tuple(AST_Tuple* node) {
 }
 
 Value ASTInterpreter::visit_attribute(AST_Attribute* node) {
-    return getattr(visit_expr(node->value).o, node->attr.c_str());
+    return pyston::getattr(visit_expr(node->value).o, node->attr.c_str());
 }
 }
 
@@ -1261,23 +1290,23 @@ Box* astInterpretFunction(CompiledFunction* cf, int nargs, Box* closure, Box* ge
     }
 
     ++cf->times_called;
-    ASTInterpreter interpreter(cf);
+    ASTInterpreter* interpreter = new ASTInterpreter(cf);
 
     ScopeInfo* scope_info = cf->clfunc->source->getScopeInfo();
     SourceInfo* source_info = cf->clfunc->source.get();
     if (unlikely(scope_info->usesNameLookup())) {
-        interpreter.setBoxedLocals(new BoxedDict());
+        interpreter->setBoxedLocals(new BoxedDict());
     }
 
     assert((!globals) == cf->clfunc->source->scoping->areGlobalsFromModule());
     if (globals) {
-        interpreter.setGlobals(globals);
+        interpreter->setGlobals(globals);
     } else {
-        interpreter.setGlobals(source_info->parent_module);
+        interpreter->setGlobals(source_info->parent_module);
     }
 
-    interpreter.initArguments(nargs, (BoxedClosure*)closure, (BoxedGenerator*)generator, arg1, arg2, arg3, args);
-    Value v = ASTInterpreter::execute(interpreter);
+    interpreter->initArguments(nargs, (BoxedClosure*)closure, (BoxedGenerator*)generator, arg1, arg2, arg3, args);
+    Value v = ASTInterpreter::execute(*interpreter);
 
     return v.o ? v.o : None;
 }
@@ -1285,18 +1314,18 @@ Box* astInterpretFunction(CompiledFunction* cf, int nargs, Box* closure, Box* ge
 Box* astInterpretFunctionEval(CompiledFunction* cf, Box* globals, Box* boxedLocals) {
     ++cf->times_called;
 
-    ASTInterpreter interpreter(cf);
-    interpreter.initArguments(0, NULL, NULL, NULL, NULL, NULL, NULL);
-    interpreter.setBoxedLocals(boxedLocals);
+    ASTInterpreter* interpreter = new ASTInterpreter(cf);
+    interpreter->initArguments(0, NULL, NULL, NULL, NULL, NULL, NULL);
+    interpreter->setBoxedLocals(boxedLocals);
 
     ScopeInfo* scope_info = cf->clfunc->source->getScopeInfo();
     SourceInfo* source_info = cf->clfunc->source.get();
 
     assert(!cf->clfunc->source->scoping->areGlobalsFromModule());
     assert(globals);
-    interpreter.setGlobals(globals);
+    interpreter->setGlobals(globals);
 
-    Value v = ASTInterpreter::execute(interpreter);
+    Value v = ASTInterpreter::execute(*interpreter);
 
     return v.o ? v.o : None;
 }
@@ -1309,29 +1338,29 @@ Box* astInterpretFrom(CompiledFunction* cf, AST_expr* after_expr, AST_stmt* encl
     assert(after_expr);
     assert(expr_val);
 
-    ASTInterpreter interpreter(cf);
+    ASTInterpreter* interpreter = new ASTInterpreter(cf);
 
     ScopeInfo* scope_info = cf->clfunc->source->getScopeInfo();
     SourceInfo* source_info = cf->clfunc->source.get();
     assert(cf->clfunc->source->scoping->areGlobalsFromModule());
-    interpreter.setGlobals(source_info->parent_module);
+    interpreter->setGlobals(source_info->parent_module);
 
     for (const auto& p : frame_state.locals->d) {
         assert(p.first->cls == str_cls);
         auto name = static_cast<BoxedString*>(p.first)->s();
         if (name == PASSED_GENERATOR_NAME) {
-            interpreter.setGenerator(p.second);
+            interpreter->setGenerator(p.second);
         } else if (name == PASSED_CLOSURE_NAME) {
-            interpreter.setPassedClosure(p.second);
+            interpreter->setPassedClosure(p.second);
         } else if (name == CREATED_CLOSURE_NAME) {
-            interpreter.setCreatedClosure(p.second);
+            interpreter->setCreatedClosure(p.second);
         } else {
             InternedString interned = cf->clfunc->source->getInternedStrings().get(name);
-            interpreter.addSymbol(interned, p.second, false);
+            interpreter->addSymbol(interned, p.second, false);
         }
     }
 
-    interpreter.setFrameInfo(frame_state.frame_info);
+    interpreter->setFrameInfo(frame_state.frame_info);
 
     CFGBlock* start_block = NULL;
     AST_stmt* starting_statement = NULL;
@@ -1343,7 +1372,7 @@ Box* astInterpretFrom(CompiledFunction* cf, AST_expr* after_expr, AST_stmt* encl
             assert(asgn->targets[0]->type == AST_TYPE::Name);
             auto name = ast_cast<AST_Name>(asgn->targets[0]);
             assert(name->id.str()[0] == '#');
-            interpreter.addSymbol(name->id, expr_val, true);
+            interpreter->addSymbol(name->id, expr_val, true);
             break;
         } else if (enclosing_stmt->type == AST_TYPE::Expr) {
             auto expr = ast_cast<AST_Expr>(enclosing_stmt);
@@ -1381,7 +1410,7 @@ Box* astInterpretFrom(CompiledFunction* cf, AST_expr* after_expr, AST_stmt* encl
         assert(starting_statement);
     }
 
-    Value v = ASTInterpreter::execute(interpreter, start_block, starting_statement);
+    Value v = ASTInterpreter::execute(*interpreter, start_block, starting_statement);
 
     return v.o ? v.o : None;
 }
@@ -1430,9 +1459,10 @@ BoxedClosure* passedClosureForInterpretedFrame(void* frame_ptr) {
     return interpreter->getPassedClosure();
 }
 
-void gatherInterpreterRoots(GCVisitor* visitor) {
-    for (const auto& p : s_interpreterMap) {
-        p.second->gcVisit(visitor);
-    }
+void setupInterpreter() {
+    astinterpreter_cls = BoxedHeapClass::create(type_cls, object_cls, ASTInterpreter::gcHandler, 0, 0,
+                                                sizeof(ASTInterpreter), false, "astinterpreter");
+    astinterpreter_cls->simple_destructor = ASTInterpreter::simpleDestructor;
+    astinterpreter_cls->freeze();
 }
 }

src/codegen/ast_interpreter.h

@@ -33,6 +33,7 @@ struct LineInfo;
 
 extern const void* interpreter_instr_addr;
 
+void setupInterpreter();
 Box* astInterpretFunction(CompiledFunction* f, int nargs, Box* closure, Box* generator, Box* globals, Box* arg1,
                           Box* arg2, Box* arg3, Box** args);
 Box* astInterpretFunctionEval(CompiledFunction* cf, Box* globals, Box* boxedLocals);
@@ -46,7 +47,6 @@ struct FrameInfo;
 FrameInfo* getFrameInfoForInterpretedFrame(void* frame_ptr);
 BoxedClosure* passedClosureForInterpretedFrame(void* frame_ptr);
 
-void gatherInterpreterRoots(gc::GCVisitor* visitor);
 BoxedDict* localsForInterpretedFrame(void* frame_ptr, bool only_user_visible);
 }
 

src/codegen/irgen/irgenerator.cpp

@@ -117,8 +117,7 @@ static llvm::Value* getBoxedLocalsGep(llvm::IRBuilder<true>& builder, llvm::Valu
 
 static llvm::Value* getExcinfoGep(llvm::IRBuilder<true>& builder, llvm::Value* v) {
     static_assert(offsetof(FrameInfo, exc) == 0, "");
-    static_assert(offsetof(ExcInfo, type) == 0, "");
-    return builder.CreateConstInBoundsGEP2_32(builder.CreateConstInBoundsGEP2_32(v, 0, 0), 0, 0);
+    return builder.CreateConstInBoundsGEP2_32(v, 0, 0);
 }
 
 static llvm::Value* getFrameObjGep(llvm::IRBuilder<true>& builder, llvm::Value* v) {
@@ -180,8 +179,16 @@ llvm::Value* IRGenState::getFrameInfoVar() {
             // The "normal" case
 
             // frame_info.exc.type = NULL
-            builder.CreateStore(getNullPtr(g.llvm_value_type_ptr), getExcinfoGep(builder, al));
-
+            // frame_info.exc.value = NULL
+            // frame_info.exc.traceback = NULL
+            llvm::Constant* null_value = getNullPtr(g.llvm_value_type_ptr);
+            llvm::Value* exc_info = getExcinfoGep(builder, al);
+            builder.CreateStore(
+                null_value, builder.CreateConstInBoundsGEP2_32(exc_info, 0, offsetof(ExcInfo, type) / sizeof(Box*)));
+            builder.CreateStore(
+                null_value, builder.CreateConstInBoundsGEP2_32(exc_info, 0, offsetof(ExcInfo, value) / sizeof(Box*)));
+            builder.CreateStore(null_value, builder.CreateConstInBoundsGEP2_32(exc_info, 0, offsetof(ExcInfo, traceback)
+                                                                                                / sizeof(Box*)));
             // frame_info.boxedLocals = NULL
             llvm::Value* boxed_locals_gep = getBoxedLocalsGep(builder, al);
             builder.CreateStore(getNullPtr(g.llvm_value_type_ptr), boxed_locals_gep);

src/core/types.h

@@ -706,6 +706,8 @@ struct FrameInfo {
     BoxedFrame* frame_obj;
 
     FrameInfo(ExcInfo exc) : exc(exc), boxedLocals(NULL), frame_obj(0) {}
+
+    void gcVisit(GCVisitor* visitor);
 };
 
 struct CallattrFlags {

src/gc/collector.cpp

@@ -277,7 +277,6 @@ void markPhase() {
     GCVisitor visitor(&stack);
 
     threading::visitAllStacks(&visitor);
-    gatherInterpreterRoots(&visitor);
 
     for (auto h : *getRootHandles()) {
         visitor.visit(h->value);

src/runtime/types.cpp

@@ -24,6 +24,7 @@
 
 #include "capi/typeobject.h"
 #include "capi/types.h"
+#include "codegen/ast_interpreter.h"
 #include "codegen/unwinding.h"
 #include "core/options.h"
 #include "core/stats.h"
@@ -91,6 +92,14 @@ bool IN_SHUTDOWN = false;
 #define SLICE_STOP_OFFSET ((char*)&(((BoxedSlice*)0x01)->stop) - (char*)0x1)
 #define SLICE_STEP_OFFSET ((char*)&(((BoxedSlice*)0x01)->step) - (char*)0x1)
 
+void FrameInfo::gcVisit(GCVisitor* visitor) {
+    visitor->visit(boxedLocals);
+    visitor->visit(exc.traceback);
+    visitor->visit(exc.type);
+    visitor->visit(exc.value);
+    visitor->visit(frame_obj);
+}
+
 // Analogue of PyType_GenericAlloc (default tp_alloc), but should only be used for Pyston classes!
 extern "C" PyObject* PystonType_GenericAlloc(BoxedClass* cls, Py_ssize_t nitems) noexcept {
     assert(cls);
@@ -2637,6 +2646,7 @@ void setupRuntime() {
 
     closure_cls->freeze();
 
+    setupInterpreter();
     setupCAPI();
 
     // Can't set up object methods until we set up CAPI support:

test/tests/generator_collection_finally.py

+# expected: fail
+# We currently don't call finalizers when destroying a generator.
+def G():
+    try:
+        yield 0
+        yield 1
+        print "end"
+    except Exception as e:
+        print e
+    finally:
+        print "finally"
+
+def foo():
+    g = G()
+    print g.next()
+    print g.next()
+foo()

test/tests/generator_collection_running.py

+# This test checks if generators which get started but haven't yet stopped (=not raisen a StopIteration exc, etc)
+# get freed when there aren't any references to the generators left.
+
+import gc
+import weakref
+
+class C(object):
+    val = 42
+
+def G():
+    l = range(100)
+    yield weakref.ref(C())
+    while True:
+        yield 1
+
+def get_weakrefs(num=5):
+    wr = []
+    for i in range(num):
+        g = G()
+        w = g.next()
+        wr.append(w)
+    return wr
+
+def recurse(f, n):
+     if n:
+        return recurse(f, n-1)
+     return f()
+
+wr = recurse(get_weakrefs, 100)
+gc.collect()
+for w in wr:
+    try:
+        print w.__hash__()
+        print w().val
+    except TypeError as e:
+        print e