Merge pull request #1328 from kmod/addguard_gcref

Fix uses of addGuard(python_object)

src/asm_writing/icinfo.cpp

@@ -78,10 +78,13 @@ void ICSlotInfo::clear(bool should_invalidate) {
         ic->invalidate(this);
     used = false;
 
-    for (auto p : gc_references) {
+    // Have to be careful here: DECREF can end up recursively clearing this slot
+    std::vector<void*> saved_gc_references;
+    std::swap(saved_gc_references, gc_references);
+    for (auto p : saved_gc_references) {
         Py_DECREF(p);
     }
-    gc_references.clear();
+    saved_gc_references.clear();
 
     for (auto&& invalidator : invalidators) {
         invalidator->remove(this);
@@ -353,10 +356,8 @@ ICInfo::~ICInfo() {
     deregisterGCTrackedICInfo(this);
 
     for (auto& slot : slots) {
-        for (auto invalidator : slot.invalidators) {
-            assert(invalidator->dependents.count(&slot));
-            invalidator->dependents.erase(&slot);
-        }
+        // Calling a full clear() might be overkill here, but probably better safe than sorry:
+        slot.clear(false);
     }
 }
 

src/asm_writing/rewriter.cpp

@@ -1435,8 +1435,7 @@ void Rewriter::commit() {
 
     for (auto p : gc_references) {
         if (Py_REFCNT(p) == 1) {
-            // we hold the only ref to this object
-            assert(0 && "untested");
+            // we hold the only ref to this object, there's no way this could succeed in the future
 
             this->abort();
             return;

src/capi/typeobject.cpp

@@ -972,8 +972,10 @@ Box* slotTpGetattrHookInternal(Box* self, BoxedString* name, GetattrRewriteArgs*
         if (rewrite_args) {
             // Fetching getattribute should have done the appropriate guarding on whether or not
             // getattribute exists.
-            if (getattribute)
+            if (getattribute) {
                 r_getattribute->addGuard((intptr_t)getattribute);
+                rewrite_args->rewriter->addGCReference(getattribute);
+            }
 
             GetattrRewriteArgs grewrite_args(rewrite_args->rewriter, rewrite_args->obj, rewrite_args->destination);
             try {

src/runtime/capi.cpp

@@ -1572,6 +1572,7 @@ Box* BoxedCApiFunction::tppCall(Box* _self, CallRewriteArgs* rewrite_args, ArgPa
 
     if (rewrite_args) {
         rewrite_args->obj->addGuard((intptr_t)self);
+        rewrite_args->rewriter->addGCReference(self);
     }
 
     int flags = self->method_def->ml_flags;

src/runtime/classobj.cpp

@@ -564,6 +564,7 @@ void instanceSetattroInternal(Box* _inst, Box* _attr, STOLEN(Box*) value, Setatt
     if (rewrite_args) {
         RewriterVar* inst_r = rewrite_args->obj->getAttr(offsetof(BoxedInstance, inst_cls));
         inst_r->addGuard((uint64_t)inst->inst_cls);
+        rewrite_args->rewriter->addGCReference(inst->inst_cls);
         GetattrRewriteArgs grewrite_args(rewrite_args->rewriter, inst_r,
                                          rewrite_args->rewriter->getReturnDestination());
         Box* setattr = classLookup<REWRITABLE>(inst->inst_cls, setattr_str, &grewrite_args);

src/runtime/objmodel.cpp

@@ -3755,6 +3755,7 @@ Box* callattrInternal(Box* obj, BoxedString* attr, LookupScope scope, CallattrRe
 
         if (rewrite_args) {
             r_val->addGuard((int64_t)val);
+            rewrite_args->rewriter->addGCReference(val);
             rewrite_args->obj = r_val;
             rewrite_args->func_guarded = true;
         }
@@ -5255,6 +5256,7 @@ Box* runtimeCallInternal(Box* obj, CallRewriteArgs* rewrite_args, ArgPassSpec ar
 
         if (rewrite_args && !rewrite_args->func_guarded) {
             r_im_func->addGuard((intptr_t)im->func);
+            rewrite_args->rewriter->addGCReference(im->func);
             rewrite_args->func_guarded = true;
         }
 
@@ -5567,8 +5569,11 @@ Box* binopInternal(Box* lhs, Box* rhs, int op_type, BinopRewriteArgs* rewrite_ar
 
         RewriterVar* r_lhs_cls = r_lhs->getAttr(offsetof(Box, cls))->setType(RefType::BORROWED);
         r_lhs_cls->addGuard((intptr_t)lhs->cls);
+        rewrite_args->rewriter->addGCReference(lhs->cls);
+
         RewriterVar* r_rhs_cls = r_rhs->getAttr(offsetof(Box, cls))->setType(RefType::BORROWED);
         r_rhs_cls->addGuard((intptr_t)rhs->cls);
+        rewrite_args->rewriter->addGCReference(rhs->cls);
 
         r_lhs_cls->addAttrGuard(offsetof(BoxedClass, tp_mro), (intptr_t)lhs->cls->tp_mro);
         r_rhs_cls->addAttrGuard(offsetof(BoxedClass, tp_mro), (intptr_t)rhs->cls->tp_mro);
@@ -6786,6 +6791,7 @@ extern "C" Box* createBoxedIterWrapperIfNeeded(Box* o) {
         } else if (r) {
             RewriterVar* rtn = rewrite_args.getReturn(ReturnConvention::HAS_RETURN);
             rtn->addGuard((uint64_t)r);
+            rewrite_args.rewriter->addGCReference(r);
             rewriter->commitReturning(r_o);
             return incref(o);
         } else /* if (!r) */ {

src/runtime/types.cpp

@@ -823,6 +823,7 @@ static Box* typeCallInner(CallRewriteArgs* rewrite_args, ArgPassSpec argspec, Bo
 
     if (rewrite_args) {
         rewrite_args->arg1->addGuard((intptr_t)cls);
+        rewrite_args->rewriter->addGCReference(cls);
     }
 
     // Special-case unicode for now, maybe there's something about this that can eventually be generalized:
@@ -1083,6 +1084,7 @@ static Box* typeCallInner(CallRewriteArgs* rewrite_args, ArgPassSpec argspec, Bo
                 if (init_attr) {
                     r_init = grewrite_args.getReturn(ReturnConvention::HAS_RETURN);
                     r_init->addGuard((intptr_t)init_attr);
+                    rewrite_args->rewriter->addGCReference(init_attr);
                 } else {
                     grewrite_args.assertReturnConvention(ReturnConvention::NO_RETURN);
                 }
@@ -3953,6 +3955,10 @@ static int type_traverse(PyTypeObject* type, visitproc visit, void* arg) {
     Py_VISIT(type->tp_bases);
     Py_VISIT(type->tp_base);
 
+    // TODO: should have something like this to traverse GC references in the type runtime ICs:
+    // if (type->hasnext_ic)
+    // Py_TRAVERSE(*type->hasnext_ic);
+
     /* There's no need to visit type->tp_subclasses or
        ((PyHeapTypeObject *)type)->ht_slots, because they can't be involved
        in cycles; tp_subclasses is a list of weak references,
@@ -4707,6 +4713,7 @@ extern "C" void Py_Finalize() noexcept {
     // initialized = 0;
 
     PyType_ClearCache();
+    clearAllICs();
     PyGC_Collect();
 
     PyImport_Cleanup();
@@ -4717,6 +4724,8 @@ extern "C" void Py_Finalize() noexcept {
 
     // May need to run multiple collections to collect everything:
     while (true) {
+        clearAllICs();
+
         int freed = 0;
         freed += PyGC_Collect();
 
@@ -4736,7 +4745,6 @@ extern "C" void Py_Finalize() noexcept {
     }
     constant_locations.clear();
 
-    clearAllICs();
     PyType_ClearCache();
     PyOS_FiniInterrupts();
     _PyCodecRegistry_Deinit();
@@ -4750,6 +4758,7 @@ extern "C" void Py_Finalize() noexcept {
     constants.clear();
 
     clearAllICs();
+    PyGC_Collect();
 
     for (auto b : late_constants) {
         Py_DECREF(b);

src/runtime/util.cpp

@@ -251,7 +251,7 @@ extern "C" void dumpEx(void* p, int levels) {
             FunctionMetadata* md = f->md;
             if (md->source) {
                 printf("User-defined function '%s'\n", md->source->getName()->c_str());
-                printf("Defined at %s:%d\n", md->source->getFn()->c_str(), md->source->getBody()[0]->lineno);
+                printf("Defined at %s:%d\n", md->source->getFn()->c_str(), md->source->ast->lineno);
 
                 if (md->source->cfg && levels > 0) {
                     md->source->cfg->print();

test/tests/elementree_test.py

@@ -39,8 +39,18 @@ def test(ET):
         name = country.get('name')
         print name, rank
 
+    it = root.iter
+    i = it()
+    for e in it():
+        assert not isinstance(e, str)
+
 import xml.etree.ElementTree as Python_ET
 import xml.etree.cElementTree as CAPI_ET
 
-test(Python_ET)
-test(CAPI_ET)
+for i in xrange(20):
+    print
+    print i
+    test(Python_ET)
+    test(CAPI_ET)
+    test(CAPI_ET)
+    test(CAPI_ET)