Allow attrwrappers with deallocd underlying objects

ie something like "print C().__dict__"

The attrwrapper objects don't (arnd probably shouldn't) keep their underlying
objects alive.  Previous to this commit they would just try to access their
freed underlying object and crash.

With this commit, object deallocation will check if there is an attrwrapper,
and if so convert it to be privately-backed.

src/core/types.h

@@ -649,8 +649,11 @@ public:
     HCAttrs(HiddenClass* hcls = NULL) : hcls(hcls), attr_list(nullptr) {}
 
     int traverse(visitproc visit, void* arg) noexcept;
-    void clear() noexcept;
-    void moduleClear() noexcept; // slightly different order of clearing attributes, meant for modules
+
+    void _clearRaw() noexcept;              // Raw clear -- clears out and decrefs all the attrs.
+                                            // Meant for implementing other clear-like functions
+    void clearForDealloc() noexcept;        // meant for normal object deallocation.  converts the attrwrapper
+    void moduleClear() noexcept;            // Meant for _PyModule_Clear.  doesn't clear all attributes.
 };
 static_assert(sizeof(HCAttrs) == sizeof(struct _hcattrs), "");
 
@@ -715,7 +718,7 @@ public:
     }
     void giveAttr(STOLEN(BoxedString*) attr, STOLEN(Box*) val);
 
-    void clearAttrs();
+    void clearAttrsForDealloc();
 
     void giveAttrDescriptor(const char* attr, Box* (*get)(Box*, void*),
             void (*set)(Box*, Box*, void*));

src/runtime/classobj.cpp

@@ -1751,7 +1751,7 @@ void BoxedInstance::dealloc(Box* b) noexcept {
         }
 
         Py_DECREF(inst->inst_cls);
-        inst->attrs.clear();
+        inst->attrs.clearForDealloc();
         PyObject_GC_Del(inst);
     } else {
         Py_ssize_t refcnt = inst->ob_refcnt;
@@ -1801,7 +1801,7 @@ void BoxedClassobj::dealloc(Box* b) noexcept {
     if (cl->weakreflist)
         PyObject_ClearWeakRefs(cl);
 
-    cl->clearAttrs();
+    cl->clearAttrsForDealloc();
 
     cl->cls->tp_free(cl);
 }

src/runtime/objmodel.cpp

@@ -490,7 +490,7 @@ static void subtype_dealloc(Box* self) noexcept {
 
     // Pyston addition: same for hcattrs
     if (type->attrs_offset && !base->attrs_offset) {
-        self->getHCAttrsPtr()->clear();
+        self->getHCAttrsPtr()->clearForDealloc();
     }
 
     /* Extract the type again; tp_del may have changed it */
@@ -866,7 +866,7 @@ static int subtype_clear(PyObject* self) noexcept {
     }
 
     if (type->attrs_offset != base->attrs_offset) {
-        self->getHCAttrsPtr()->clear();
+        self->getHCAttrsPtr()->clearForDealloc();
     }
 
     if (baseclear)
@@ -1233,25 +1233,56 @@ static HCAttrs::AttrList* reallocAttrs(HCAttrs::AttrList* attrs, int old_nattrs,
     return rtn;
 }
 
-void HCAttrs::clear() noexcept {
-    HiddenClass* hcls = this->hcls;
+void Box::setDictBacked(STOLEN(Box*) val) {
+    // this checks for: v.__dict__ = v.__dict__
+    if (val->cls == attrwrapper_cls && unwrapAttrWrapper(val) == this) {
+        Py_DECREF(val);
+        return;
+    }
 
-    if (!hcls)
+    assert(this->cls->instancesHaveHCAttrs());
+    HCAttrs* hcattrs = this->getHCAttrsPtr();
+    RELEASE_ASSERT(PyDict_Check(val) || val->cls == attrwrapper_cls, "");
+
+    if (hcattrs->hcls->type == HiddenClass::DICT_BACKED) {
+        auto old_dict = hcattrs->attr_list->attrs[0];
+        hcattrs->attr_list->attrs[0] = val;
+        Py_DECREF(old_dict);
         return;
+    }
 
-    if (unlikely(hcls->type == HiddenClass::DICT_BACKED)) {
-        Box* d = this->attr_list->attrs[0];
+    // If there is an old attrwrapper it is not allowed to wrap the instance anymore instead it has to switch to a
+    // private dictonary.
+    // e.g.:
+    //     a = v.__dict__
+    //     v.__dict__ = {} # 'a' must switch now from wrapping 'v' to a the private dict.
+    int offset = hcattrs->hcls->getAttrwrapperOffset();
+    if (offset != -1) {
+        Box* wrapper = hcattrs->attr_list->attrs[offset];
+        RELEASE_ASSERT(wrapper->cls == attrwrapper_cls, "");
+        convertAttrwrapperToPrivateDict(wrapper);
+    }
 
-        // Skips the attrlist freelist
-        PyObject_FREE(this->attr_list);
-        this->attr_list = NULL;
+    // assign the dict to the attribute list and switch to the dict backed strategy
+    // Skips the attrlist freelist
+    auto new_attr_list = (HCAttrs::AttrList*)PyObject_MALLOC(sizeof(HCAttrs::AttrList) + sizeof(Box*));
+    new_attr_list->attrs[0] = val;
 
-        Py_DECREF(d);
+    auto old_attr_list = hcattrs->attr_list;
+    int old_attr_list_size = hcattrs->hcls->attributeArraySize();
 
-        return;
-    }
+    hcattrs->hcls = HiddenClass::dict_backed;
+    hcattrs->attr_list = new_attr_list;
 
-    assert(hcls->type == HiddenClass::NORMAL || hcls->type == HiddenClass::SINGLETON);
+    decrefArray(old_attr_list->attrs, old_attr_list_size);
+    freeAttrs(old_attr_list, old_attr_list_size);
+}
+
+void HCAttrs::_clearRaw() noexcept {
+    HiddenClass* hcls = this->hcls;
+
+    if (!hcls)
+        return;
 
     auto old_attr_list = this->attr_list;
     auto old_attr_list_size = hcls->attributeArraySize();
@@ -1261,8 +1292,30 @@ void HCAttrs::clear() noexcept {
     if (old_attr_list) {
         decrefArray(old_attr_list->attrs, old_attr_list_size);
 
-        freeAttrs(old_attr_list, old_attr_list_size);
+        // DICT_BACKED attrs don't use the freelist:
+        if (hcls->type == HiddenClass::DICT_BACKED)
+            PyObject_FREE(old_attr_list);
+        else
+            freeAttrs(old_attr_list, old_attr_list_size);
+    }
+}
+
+void HCAttrs::clearForDealloc() noexcept {
+    HiddenClass* hcls = this->hcls;
+
+    if (!hcls)
+        return;
+
+    if (hcls->type == HiddenClass::NORMAL || hcls->type == HiddenClass::SINGLETON) {
+        int offset = hcls->getAttrwrapperOffset();
+        if (offset != -1) {
+            Box* attrwrapper = this->attr_list->attrs[offset];
+            if (attrwrapper->ob_refcnt != 1)
+                convertAttrwrapperToPrivateDict(attrwrapper);
+        }
     }
+
+    _clearRaw();
 }
 
 void HCAttrs::moduleClear() noexcept {

src/runtime/types.cpp

@@ -405,7 +405,7 @@ static void functionDtor(Box* b) {
     self->dependent_ics.invalidateAll();
     self->dependent_ics.~ICInvalidator();
 
-    self->clearAttrs();
+    self->clearAttrsForDealloc();
 
     Py_DECREF(self->doc);
     Py_XDECREF(self->modname);
@@ -2166,10 +2166,17 @@ private:
         if (isDictBacked())
             return;
 
+        // TODO: this means that future accesses to __dict__ will return something other than
+        // this attrwrapper.  We should store the attrwrapper in the attributes array.
+
         HCAttrs* attrs = this->b->getHCAttrsPtr();
         assert(attrs->hcls->type != HiddenClass::DICT_BACKED);
         BoxedDict* d = (BoxedDict*)AttrWrapper::copy(this);
-        b->clearAttrs();
+
+        b->getHCAttrsPtr()->_clearRaw();
+        assert(this->b);
+        assert(!private_dict);
+
         HCAttrs* hcattrs = b->getHCAttrsPtr();
         // Skips the attrlist freelist:
         auto new_attr_list = (HCAttrs::AttrList*)PyObject_MALLOC(sizeof(HCAttrs::AttrList) + sizeof(Box*));
@@ -2211,7 +2218,6 @@ public:
         RELEASE_ASSERT(b, "");
         private_dict = (BoxedDict*)AttrWrapper::copy(this);
         assert(PyDict_CheckExact(private_dict));
-        b->clearAttrs();
         b = NULL;
     }
 
@@ -2610,7 +2616,7 @@ public:
 
         HCAttrs* attrs = self->b->getHCAttrsPtr();
         RELEASE_ASSERT(attrs->hcls->type == HiddenClass::NORMAL || attrs->hcls->type == HiddenClass::SINGLETON, "");
-        attrs->clear();
+        attrs->_clearRaw();
 
         // Add the existing attrwrapper object (ie self) back as the attrwrapper:
         self->b->appendNewHCAttr(self, NULL);
@@ -2730,6 +2736,11 @@ public:
     friend class AttrWrapperIter;
 };
 
+void convertAttrwrapperToPrivateDict(Box* b) {
+    assert(b->cls == attrwrapper_cls);
+    static_cast<AttrWrapper*>(b)->convertToPrivateDict();
+};
+
 AttrWrapperIter::AttrWrapperIter(AttrWrapper* aw) {
     hcls = aw->b->getHCAttrsPtr()->hcls;
     assert(hcls);
@@ -2822,38 +2833,6 @@ void attrwrapperSet(Box* b, Box* k, Box* v) {
 }
 
 
-void Box::setDictBacked(STOLEN(Box*) val) {
-    // this checks for: v.__dict__ = v.__dict__
-    if (val->cls == attrwrapper_cls && unwrapAttrWrapper(val) == this) {
-        Py_DECREF(val);
-        return;
-    }
-
-    assert(this->cls->instancesHaveHCAttrs());
-    HCAttrs* hcattrs = this->getHCAttrsPtr();
-    RELEASE_ASSERT(PyDict_Check(val) || val->cls == attrwrapper_cls, "");
-
-    // If there is an old attrwrapper it is not allowed to wrap the instance anymore instead it has to switch to a
-    // private dictonary.
-    // e.g.:
-    //     a = v.__dict__
-    //     v.__dict__ = {} # 'a' must switch now from wrapping 'v' to a the private dict.
-    int offset = hcattrs->hcls->type != HiddenClass::DICT_BACKED ? hcattrs->hcls->getAttrwrapperOffset() : -1;
-    if (offset != -1) {
-        AttrWrapper* wrapper = (AttrWrapper*)hcattrs->attr_list->attrs[offset];
-        RELEASE_ASSERT(wrapper->cls == attrwrapper_cls, "");
-        wrapper->convertToPrivateDict();
-    }
-
-    // assign the dict to the attribute list and switch to the dict backed strategy
-    // Skips the attrlist freelist
-    auto new_attr_list = (HCAttrs::AttrList*)PyObject_MALLOC(sizeof(HCAttrs::AttrList) + sizeof(Box*));
-    new_attr_list->attrs[0] = val;
-
-    hcattrs->hcls = HiddenClass::dict_backed;
-    hcattrs->attr_list = new_attr_list;
-}
-
 Box* attrwrapperKeys(Box* b) {
     return AttrWrapper::keys(b);
 }
@@ -3509,7 +3488,7 @@ extern "C" int PyObject_DelHcAttrString(PyObject* obj, const char* attr) PYSTON_
 }
 
 extern "C" int PyObject_ClearHcAttrs(HCAttrs* attrs) noexcept {
-    attrs->clear();
+    attrs->clearForDealloc();
     return 0;
 }
 
@@ -3729,16 +3708,16 @@ static Box* getsetDelete(Box* self, Box* obj) {
     return getsetSet(self, obj, NULL);
 }
 
-void Box::clearAttrs() {
+void Box::clearAttrsForDealloc() {
     if (cls->instancesHaveHCAttrs()) {
         HCAttrs* attrs = getHCAttrsPtr();
-        attrs->clear();
+        attrs->clearForDealloc();
         return;
     }
 
     if (cls->instancesHaveDictAttrs()) {
-        BoxedDict* d = getDict();
-        PyDict_Clear(d);
+        BoxedDict** d = getDictPtr();
+        Py_CLEAR(*d);
         return;
     }
 }
@@ -3839,7 +3818,7 @@ extern "C" void _PyModule_Clear(PyObject* b) noexcept {
 
 int BoxedModule::clear(Box* b) noexcept {
     _PyModule_Clear(b);
-    b->clearAttrs();
+    b->clearAttrsForDealloc();
 
     return 0;
 }
@@ -3857,7 +3836,7 @@ void BoxedSlice::dealloc(Box* b) noexcept {
 void BoxedInstanceMethod::dealloc(Box* b) noexcept {
     BoxedInstanceMethod* im = static_cast<BoxedInstanceMethod*>(b);
 
-    im->clearAttrs();
+    im->clearAttrsForDealloc();
 
     _PyObject_GC_UNTRACK(im);
     if (im->im_weakreflist != NULL)
@@ -3896,7 +3875,7 @@ void BoxedClass::dealloc(Box* b) noexcept {
 
     if (PyObject_IS_GC(type))
         _PyObject_GC_UNTRACK(type);
-    type->clearAttrs();
+    type->clearAttrsForDealloc();
 
     Py_XDECREF(type->tp_dict);
     Py_XDECREF(type->tp_bases);
@@ -4003,7 +3982,7 @@ static int type_clear(PyTypeObject* type) {
     PyType_Modified(type);
     if (type->tp_dict)
         PyDict_Clear(type->tp_dict);
-    type->attrs.clear();
+    type->attrs.clearForDealloc();
     Py_CLEAR(type->tp_dict);
     Py_CLEAR(type->tp_mro);
 
@@ -4816,7 +4795,7 @@ extern "C" void Py_Finalize() noexcept {
 
     for (auto b : classes) {
         if (!PyObject_IS_GC(b)) {
-            b->clearAttrs();
+            b->getHCAttrsPtr()->_clearRaw();
             Py_CLEAR(b->tp_mro);
         }
         Py_DECREF(b);

src/runtime/types.h

@@ -1333,6 +1333,7 @@ public:
 Box* objectSetattr(Box* obj, Box* attr, Box* value);
 
 BORROWED(Box*) unwrapAttrWrapper(Box* b);
+void convertAttrwrapperToPrivateDict(Box* b);
 Box* attrwrapperKeys(Box* b);
 void attrwrapperDel(Box* b, llvm::StringRef attr);
 void attrwrapperClear(Box* b);

test/tests/dict_setting.py

-# expected: reffail
 try:
     object().__dict__ = 1
 except AttributeError as e:

test/tests/special_getattrs.py

-# expected: reffail
 def show(obj):
     print obj.__class__
     for b in obj.__class__.__mro__: