Merge branch 'tp_dictoffset'

Makefile

@@ -869,6 +869,10 @@ runpy_%: %.py ext_python
 	PYTHONPATH=test/test_extension/build/lib.linux-x86_64-2.7 python $<
 $(call make_search,runpy_%)
 
+check_%: %.py ext_python ext_pyston
+	$(MAKE) check_dbg ARGS="$(patsubst %.py,%,$(notdir $<)) -K"
+$(call make_search,check_%)
+
 dbgpy_%: %.py ext_pythondbg
 	export PYTHON_VERSION=$$(python2.7-dbg -V 2>&1 | awk '{print $$2}'); PYTHONPATH=test/test_extension/build/lib.linux-x86_64-2.7-pydebug $(GDB) --ex "dir $(DEPS_DIR)/python-src/python2.7-$$PYTHON_VERSION/debian" $(GDB_CMDS) --args python2.7-dbg $<
 $(call make_search,dbgpy_%)

src/capi/typeobject.cpp

@@ -1762,9 +1762,6 @@ extern "C" int PyType_Ready(PyTypeObject* cls) noexcept {
     cls->gc_visit = &conservativeGCHandler;
     cls->is_user_defined = true;
 
-    // TODO not sure how we can handle extension types that manually
-    // specify a dict...
-    RELEASE_ASSERT(cls->tp_dictoffset == 0, "");
     // this should get automatically initialized to 0 on this path:
     assert(cls->attrs_offset == 0);
 

src/codegen/compvars.cpp

@@ -1274,8 +1274,13 @@ public:
         return c == cls;
     }
 
+    bool canStaticallyResolveGetattrs() {
+        return (cls->is_constant && !cls->instancesHaveHCAttrs() && !cls->instancesHaveDictAttrs()
+                && cls->hasGenericGetattr());
+    }
+
     CompilerType* getattrType(const std::string* attr, bool cls_only) override {
-        if (cls->is_constant && !cls->instancesHaveAttrs() && cls->hasGenericGetattr()) {
+        if (canStaticallyResolveGetattrs()) {
             Box* rtattr = cls->getattr(*attr);
             if (rtattr == NULL)
                 return UNDEF;
@@ -1300,7 +1305,7 @@ public:
     CompilerVariable* getattr(IREmitter& emitter, const OpInfo& info, ConcreteCompilerVariable* var,
                               const std::string* attr, bool cls_only) override {
         // printf("%s.getattr %s\n", debugName().c_str(), attr->c_str());
-        if (cls->is_constant && !cls->instancesHaveAttrs() && cls->hasGenericGetattr()) {
+        if (canStaticallyResolveGetattrs()) {
             Box* rtattr = cls->getattr(*attr);
             if (rtattr == NULL) {
                 llvm::CallSite call = emitter.createCall2(info.unw_info, g.funcs.raiseAttributeErrorStr,
@@ -1346,7 +1351,7 @@ public:
                                                   const std::vector<CompilerVariable*>& args,
                                                   const std::vector<const std::string*>* keyword_names,
                                                   bool raise_on_missing = true) {
-        if (!cls->is_constant || cls->instancesHaveAttrs() || !cls->hasGenericGetattr())
+        if (!canStaticallyResolveGetattrs())
             return NULL;
 
         Box* rtattr = cls->getattr(*attr);

src/core/types.h

@@ -381,6 +381,7 @@ public:
     HCAttrs() : hcls(root_hcls), attr_list(nullptr) {}
 };
 
+class BoxedDict;
 class BoxedString;
 
 class Box {
@@ -396,7 +397,8 @@ public:
 
     llvm::iterator_range<BoxIterator> pyElements();
 
-    HCAttrs* getAttrsPtr();
+    HCAttrs* getHCAttrsPtr();
+    BoxedDict* getDict();
 
     void setattr(const std::string& attr, Box* val, SetattrRewriteArgs* rewrite_args);
     void giveAttr(const std::string& attr, Box* val) {

src/runtime/builtin_modules/builtins.cpp

@@ -91,12 +91,15 @@ extern "C" Box* dir(Box* obj) {
     for (auto const& kv : obj->cls->attrs.hcls->attr_offsets) {
         listAppend(result, boxString(kv.first));
     }
-    if (obj->cls->instancesHaveAttrs()) {
-        HCAttrs* attrs = obj->getAttrsPtr();
+    if (obj->cls->instancesHaveHCAttrs()) {
+        HCAttrs* attrs = obj->getHCAttrsPtr();
         for (auto const& kv : attrs->hcls->attr_offsets) {
             listAppend(result, boxString(kv.first));
         }
     }
+    if (obj->cls->instancesHaveDictAttrs()) {
+        Py_FatalError("unimplemented");
+    }
     return result;
 }
 

src/runtime/objmodel.cpp

@@ -64,6 +64,13 @@ static const std::string _call_str("__call__"), _new_str("__new__"), _init_str("
 static const std::string _getattr_str("__getattr__");
 static const std::string _getattribute_str("__getattribute__");
 
+#if 0
+void REWRITE_ABORTED(const char* reason) {
+}
+#else
+#define REWRITE_ABORTED(reason) ((void)(reason))
+#endif
+
 struct GetattrRewriteArgs {
     Rewriter* rewriter;
     RewriterVar* obj;
@@ -494,15 +501,34 @@ HiddenClass* HiddenClass::delAttrToMakeHC(const std::string& attr) {
     return cur;
 }
 
-HCAttrs* Box::getAttrsPtr() {
-    assert(cls->instancesHaveAttrs());
+HCAttrs* Box::getHCAttrsPtr() {
+    assert(cls->instancesHaveHCAttrs());
 
     char* p = reinterpret_cast<char*>(this);
     p += cls->attrs_offset;
     return reinterpret_cast<HCAttrs*>(p);
 }
 
+BoxedDict* Box::getDict() {
+    assert(cls->instancesHaveDictAttrs());
+
+    char* p = reinterpret_cast<char*>(this);
+    p += cls->tp_dictoffset;
+
+    BoxedDict** d_ptr = reinterpret_cast<BoxedDict**>(p);
+    BoxedDict* d = *d_ptr;
+    if (!d) {
+        d = *d_ptr = new BoxedDict();
+    }
+
+    assert(d->cls == dict_cls);
+    return d;
+}
+
 Box* Box::getattr(const std::string& attr, GetattrRewriteArgs* rewrite_args) {
+    if (rewrite_args)
+        rewrite_args->obj->addAttrGuard(BOX_CLS_OFFSET, (intptr_t)cls);
+
     // Have to guard on the memory layout of this object.
     // Right now, guard on the specific Python-class, which in turn
     // specifies the C structure.
@@ -512,41 +538,55 @@ Box* Box::getattr(const std::string& attr, GetattrRewriteArgs* rewrite_args) {
     // Only matters if we end up getting multiple classes with the same
     // structure (ex user class) and the same hidden classes, because
     // otherwise the guard will fail anyway.;
-    if (rewrite_args) {
-        rewrite_args->obj->addAttrGuard(BOX_CLS_OFFSET, (intptr_t)cls);
-        rewrite_args->out_success = true;
-    }
+    if (cls->instancesHaveHCAttrs()) {
+        if (rewrite_args)
+            rewrite_args->out_success = true;
 
-    if (!cls->instancesHaveAttrs()) {
-        return NULL;
-    }
+        HCAttrs* attrs = getHCAttrsPtr();
+        HiddenClass* hcls = attrs->hcls;
 
-    HCAttrs* attrs = getAttrsPtr();
-    HiddenClass* hcls = attrs->hcls;
+        if (rewrite_args) {
+            if (!rewrite_args->obj_hcls_guarded)
+                rewrite_args->obj->addAttrGuard(cls->attrs_offset + HCATTRS_HCLS_OFFSET, (intptr_t)hcls);
+        }
 
-    if (rewrite_args) {
-        if (!rewrite_args->obj_hcls_guarded)
-            rewrite_args->obj->addAttrGuard(cls->attrs_offset + HCATTRS_HCLS_OFFSET, (intptr_t)hcls);
-    }
+        int offset = hcls->getOffset(attr);
+        if (offset == -1) {
+            return NULL;
+        }
+
+        if (rewrite_args) {
+            // TODO using the output register as the temporary makes register allocation easier
+            // since we don't need to clobber a register, but does it make the code slower?
+            RewriterVar* r_attrs
+                = rewrite_args->obj->getAttr(cls->attrs_offset + HCATTRS_ATTRS_OFFSET, Location::any());
+            rewrite_args->out_rtn = r_attrs->getAttr(offset * sizeof(Box*) + ATTRLIST_ATTRS_OFFSET, Location::any());
+        }
 
-    int offset = hcls->getOffset(attr);
-    if (offset == -1) {
-        return NULL;
+        Box* rtn = attrs->attr_list->attrs[offset];
+        return rtn;
     }
 
-    if (rewrite_args) {
-        // TODO using the output register as the temporary makes register allocation easier
-        // since we don't need to clobber a register, but does it make the code slower?
-        RewriterVar* r_attrs = rewrite_args->obj->getAttr(cls->attrs_offset + HCATTRS_ATTRS_OFFSET, Location::any());
-        rewrite_args->out_rtn = r_attrs->getAttr(offset * sizeof(Box*) + ATTRLIST_ATTRS_OFFSET, Location::any());
+    if (cls->instancesHaveDictAttrs()) {
+        if (rewrite_args)
+            REWRITE_ABORTED("");
+
+        BoxedDict* d = getDict();
+
+        Box* key = boxString(attr);
+        auto it = d->d.find(key);
+        if (it == d->d.end())
+            return NULL;
+        return it->second;
     }
 
-    Box* rtn = attrs->attr_list->attrs[offset];
-    return rtn;
+    if (rewrite_args)
+        rewrite_args->out_success = true;
+
+    return NULL;
 }
 
 void Box::setattr(const std::string& attr, Box* val, SetattrRewriteArgs* rewrite_args) {
-    assert(cls->instancesHaveAttrs());
     assert(gc::isValidGCObject(val));
 
     // Have to guard on the memory layout of this object.
@@ -561,84 +601,96 @@ void Box::setattr(const std::string& attr, Box* val, SetattrRewriteArgs* rewrite
     if (rewrite_args)
         rewrite_args->obj->addAttrGuard(BOX_CLS_OFFSET, (intptr_t)cls);
 
-
     static const std::string none_str("None");
 
     RELEASE_ASSERT(attr != none_str || this == builtins_module, "can't assign to None");
 
-    HCAttrs* attrs = getAttrsPtr();
-    HiddenClass* hcls = attrs->hcls;
-    int numattrs = hcls->attr_offsets.size();
+    if (cls->instancesHaveHCAttrs()) {
+        HCAttrs* attrs = getHCAttrsPtr();
+        HiddenClass* hcls = attrs->hcls;
+        int numattrs = hcls->attr_offsets.size();
 
-    int offset = hcls->getOffset(attr);
+        int offset = hcls->getOffset(attr);
 
-    if (rewrite_args) {
-        rewrite_args->obj->addAttrGuard(cls->attrs_offset + HCATTRS_HCLS_OFFSET, (intptr_t)hcls);
-        // rewrite_args->rewriter->addDecision(offset == -1 ? 1 : 0);
-    }
+        if (rewrite_args) {
+            rewrite_args->obj->addAttrGuard(cls->attrs_offset + HCATTRS_HCLS_OFFSET, (intptr_t)hcls);
+            // rewrite_args->rewriter->addDecision(offset == -1 ? 1 : 0);
+        }
 
-    if (offset >= 0) {
-        assert(offset < numattrs);
-        Box* prev = attrs->attr_list->attrs[offset];
-        attrs->attr_list->attrs[offset] = val;
+        if (offset >= 0) {
+            assert(offset < numattrs);
+            Box* prev = attrs->attr_list->attrs[offset];
+            attrs->attr_list->attrs[offset] = val;
 
-        if (rewrite_args) {
+            if (rewrite_args) {
 
-            RewriterVar* r_hattrs
-                = rewrite_args->obj->getAttr(cls->attrs_offset + HCATTRS_ATTRS_OFFSET, Location::any());
+                RewriterVar* r_hattrs
+                    = rewrite_args->obj->getAttr(cls->attrs_offset + HCATTRS_ATTRS_OFFSET, Location::any());
 
-            r_hattrs->setAttr(offset * sizeof(Box*) + ATTRLIST_ATTRS_OFFSET, rewrite_args->attrval);
+                r_hattrs->setAttr(offset * sizeof(Box*) + ATTRLIST_ATTRS_OFFSET, rewrite_args->attrval);
 
-            rewrite_args->out_success = true;
-        }
+                rewrite_args->out_success = true;
+            }
 
-        return;
-    }
+            return;
+        }
 
-    assert(offset == -1);
-    HiddenClass* new_hcls = hcls->getOrMakeChild(attr);
+        assert(offset == -1);
+        HiddenClass* new_hcls = hcls->getOrMakeChild(attr);
 
-    // TODO need to make sure we don't need to rearrange the attributes
-    assert(new_hcls->attr_offsets[attr] == numattrs);
+        // TODO need to make sure we don't need to rearrange the attributes
+        assert(new_hcls->attr_offsets[attr] == numattrs);
 #ifndef NDEBUG
-    for (const auto& p : hcls->attr_offsets) {
-        assert(new_hcls->attr_offsets[p.first] == p.second);
-    }
+        for (const auto& p : hcls->attr_offsets) {
+            assert(new_hcls->attr_offsets[p.first] == p.second);
+        }
 #endif
 
-    RewriterVar* r_new_array2 = NULL;
-    int new_size = sizeof(HCAttrs::AttrList) + sizeof(Box*) * (numattrs + 1);
-    if (numattrs == 0) {
-        attrs->attr_list = (HCAttrs::AttrList*)gc_alloc(new_size, gc::GCKind::UNTRACKED);
-        if (rewrite_args) {
-            RewriterVar* r_newsize = rewrite_args->rewriter->loadConst(new_size, Location::forArg(0));
-            RewriterVar* r_kind = rewrite_args->rewriter->loadConst((int)gc::GCKind::UNTRACKED, Location::forArg(1));
-            r_new_array2 = rewrite_args->rewriter->call(false, (void*)gc::gc_alloc, r_newsize, r_kind);
+        RewriterVar* r_new_array2 = NULL;
+        int new_size = sizeof(HCAttrs::AttrList) + sizeof(Box*) * (numattrs + 1);
+        if (numattrs == 0) {
+            attrs->attr_list = (HCAttrs::AttrList*)gc_alloc(new_size, gc::GCKind::UNTRACKED);
+            if (rewrite_args) {
+                RewriterVar* r_newsize = rewrite_args->rewriter->loadConst(new_size, Location::forArg(0));
+                RewriterVar* r_kind
+                    = rewrite_args->rewriter->loadConst((int)gc::GCKind::UNTRACKED, Location::forArg(1));
+                r_new_array2 = rewrite_args->rewriter->call(false, (void*)gc::gc_alloc, r_newsize, r_kind);
+            }
+        } else {
+            attrs->attr_list = (HCAttrs::AttrList*)gc::gc_realloc(attrs->attr_list, new_size);
+            if (rewrite_args) {
+                RewriterVar* r_oldarray
+                    = rewrite_args->obj->getAttr(cls->attrs_offset + HCATTRS_ATTRS_OFFSET, Location::forArg(0));
+                RewriterVar* r_newsize = rewrite_args->rewriter->loadConst(new_size, Location::forArg(1));
+                r_new_array2 = rewrite_args->rewriter->call(false, (void*)gc::gc_realloc, r_oldarray, r_newsize);
+            }
         }
-    } else {
-        attrs->attr_list = (HCAttrs::AttrList*)gc::gc_realloc(attrs->attr_list, new_size);
+        // Don't set the new hcls until after we do the allocation for the new attr_list;
+        // that allocation can cause a collection, and we want the collector to always
+        // see a consistent state between the hcls and the attr_list
+        attrs->hcls = new_hcls;
+
         if (rewrite_args) {
-            RewriterVar* r_oldarray
-                = rewrite_args->obj->getAttr(cls->attrs_offset + HCATTRS_ATTRS_OFFSET, Location::forArg(0));
-            RewriterVar* r_newsize = rewrite_args->rewriter->loadConst(new_size, Location::forArg(1));
-            r_new_array2 = rewrite_args->rewriter->call(false, (void*)gc::gc_realloc, r_oldarray, r_newsize);
-        }
-    }
-    // Don't set the new hcls until after we do the allocation for the new attr_list;
-    // that allocation can cause a collection, and we want the collector to always
-    // see a consistent state between the hcls and the attr_list
-    attrs->hcls = new_hcls;
+            r_new_array2->setAttr(numattrs * sizeof(Box*) + ATTRLIST_ATTRS_OFFSET, rewrite_args->attrval);
+            rewrite_args->obj->setAttr(cls->attrs_offset + HCATTRS_ATTRS_OFFSET, r_new_array2);
 
-    if (rewrite_args) {
-        r_new_array2->setAttr(numattrs * sizeof(Box*) + ATTRLIST_ATTRS_OFFSET, rewrite_args->attrval);
-        rewrite_args->obj->setAttr(cls->attrs_offset + HCATTRS_ATTRS_OFFSET, r_new_array2);
+            RewriterVar* r_hcls = rewrite_args->rewriter->loadConst((intptr_t)new_hcls);
+            rewrite_args->obj->setAttr(cls->attrs_offset + HCATTRS_HCLS_OFFSET, r_hcls);
 
-        RewriterVar* r_hcls = rewrite_args->rewriter->loadConst((intptr_t)new_hcls);
-        rewrite_args->obj->setAttr(cls->attrs_offset + HCATTRS_HCLS_OFFSET, r_hcls);
+            rewrite_args->out_success = true;
+        }
+        attrs->attr_list->attrs[numattrs] = val;
+        return;
+    }
 
-        rewrite_args->out_success = true;
+    if (cls->instancesHaveDictAttrs()) {
+        BoxedDict* d = getDict();
+        d->d[boxString(attr)] = val;
+        return;
     }
-    attrs->attr_list->attrs[numattrs] = val;
+
+    // Unreachable
+    abort();
 }
 
 Box* typeLookup(BoxedClass* cls, const std::string& attr, GetattrRewriteArgs* rewrite_args) {
@@ -951,6 +1003,7 @@ Box* dataDescriptorInstanceSpecialCases(GetattrRewriteArgs* rewrite_args, const
                 }
 
                 rewrite_args = NULL;
+                REWRITE_ABORTED("");
                 char* rtn = reinterpret_cast<char*>((char*)obj + member_desc->offset);
                 return boxString(std::string(rtn));
             }
@@ -962,6 +1015,7 @@ Box* dataDescriptorInstanceSpecialCases(GetattrRewriteArgs* rewrite_args, const
 
     else if (descr->cls == property_cls) {
         rewrite_args = NULL; // TODO
+        REWRITE_ABORTED("");
 
         BoxedProperty* prop = static_cast<BoxedProperty*>(descr);
         if (prop->prop_get == NULL || prop->prop_get == None) {
@@ -1184,8 +1238,10 @@ Box* getattrInternalGeneral(Box* obj, const std::string& attr, GetattrRewriteArg
                 if (_set_) {
                     // Have to abort because we're about to call now, but there will be before more
                     // guards between this call and the next...
-                    if (for_call)
+                    if (for_call) {
                         rewrite_args = NULL;
+                        REWRITE_ABORTED("");
+                    }
 
                     Box* res;
                     if (rewrite_args) {
@@ -1285,8 +1341,10 @@ Box* getattrInternalGeneral(Box* obj, const std::string& attr, GetattrRewriteArg
                 if (local_get) {
                     Box* res;
 
-                    if (for_call)
+                    if (for_call) {
                         rewrite_args = NULL;
+                        REWRITE_ABORTED("");
+                    }
 
                     if (rewrite_args) {
                         CallRewriteArgs crewrite_args(rewrite_args->rewriter, r_get, rewrite_args->destination);
@@ -1330,8 +1388,10 @@ Box* getattrInternalGeneral(Box* obj, const std::string& attr, GetattrRewriteArg
         // the result.
         if (_get_) {
             // this could happen for the callattr path...
-            if (for_call)
+            if (for_call) {
                 rewrite_args = NULL;
+                REWRITE_ABORTED("");
+            }
 
             Box* res;
             if (rewrite_args) {
@@ -1366,6 +1426,7 @@ Box* getattrInternalGeneral(Box* obj, const std::string& attr, GetattrRewriteArg
         // Don't need to pass icentry args, since we special-case __getattribute__ and __getattr__ to use
         // invalidation rather than guards
         rewrite_args = NULL;
+        REWRITE_ABORTED("");
         Box* getattr = typeLookup(obj->cls, "__getattr__", NULL);
         if (getattr) {
             Box* boxstr = boxString(attr);
@@ -1403,7 +1464,8 @@ extern "C" Box* getattr(Box* obj, const char* attr) {
     }
 
     if (strcmp(attr, "__dict__") == 0) {
-        if (obj->cls->instancesHaveAttrs())
+        // TODO this is wrong, should be added at the class level as a getset
+        if (obj->cls->instancesHaveHCAttrs())
             return makeAttrWrapper(obj);
     }
 
@@ -1509,6 +1571,8 @@ void setattrInternal(Box* obj, const std::string& attr, Box* val, SetattrRewrite
         BoxedClass* self = static_cast<BoxedClass*>(obj);
 
         if (attr == _getattr_str || attr == _getattribute_str) {
+            if (rewrite_args)
+                REWRITE_ABORTED("");
             // Will have to embed the clear in the IC, so just disable the patching for now:
             rewrite_args = NULL;
 
@@ -1521,8 +1585,10 @@ void setattrInternal(Box* obj, const std::string& attr, Box* val, SetattrRewrite
             raiseExcHelper(TypeError, "readonly attribute");
 
         bool touched_slot = update_slot(self, attr);
-        if (touched_slot)
+        if (touched_slot) {
             rewrite_args = NULL;
+            REWRITE_ABORTED("");
+        }
     }
 }
 
@@ -1532,7 +1598,7 @@ extern "C" void setattr(Box* obj, const char* attr, Box* attr_val) {
     static StatCounter slowpath_setattr("slowpath_setattr");
     slowpath_setattr.log();
 
-    if (!obj->cls->instancesHaveAttrs()) {
+    if (!obj->cls->instancesHaveHCAttrs() && !obj->cls->instancesHaveDictAttrs()) {
         raiseAttributeError(obj, attr);
     }
 
@@ -2013,6 +2079,7 @@ extern "C" Box* callattrInternal(Box* obj, const std::string* attr, LookupScope
         Box* rtn;
         if (val->cls != function_cls && val->cls != instancemethod_cls) {
             rewrite_args = NULL;
+            REWRITE_ABORTED("");
         }
 
         if (rewrite_args) {
@@ -2239,11 +2306,13 @@ Box* callFunc(BoxedFunction* func, CallRewriteArgs* rewrite_args, ArgPassSpec ar
 
     if (argspec.has_starargs || argspec.has_kwargs || f->takes_kwargs || func->isGenerator) {
         rewrite_args = NULL;
+        REWRITE_ABORTED("");
     }
 
     // These could be handled:
     if (argspec.num_keywords) {
         rewrite_args = NULL;
+        REWRITE_ABORTED("");
     }
 
     // TODO Should we guard on the CLFunction or the BoxedFunction?
@@ -2328,10 +2397,12 @@ Box* callFunc(BoxedFunction* func, CallRewriteArgs* rewrite_args, ArgPassSpec ar
     std::vector<Box*, StlCompatAllocator<Box*>> unused_positional;
     for (int i = positional_to_positional; i < argspec.num_args; i++) {
         rewrite_args = NULL;
+        REWRITE_ABORTED("");
         unused_positional.push_back(getArg(i, arg1, arg2, arg3, args));
     }
     for (int i = varargs_to_positional; i < varargs.size(); i++) {
         rewrite_args = NULL;
+        REWRITE_ABORTED("");
         unused_positional.push_back(varargs[i]);
     }
 
@@ -2785,6 +2856,7 @@ extern "C" Box* binopInternal(Box* lhs, Box* rhs, int op_type, bool inplace, Bin
     if (rewrite_args) {
         assert(rewrite_args->out_success == false);
         rewrite_args = NULL;
+        REWRITE_ABORTED("");
     }
 
     std::string rop_name = getReverseOpName(op_type);
@@ -2978,6 +3050,7 @@ Box* compareInternal(Box* lhs, Box* rhs, int op_type, CompareRewriteArgs* rewrit
     if (rewrite_args) {
         assert(rewrite_args->out_success == false);
         rewrite_args = NULL;
+        REWRITE_ABORTED("");
     }
 
     std::string rop_name = getReverseOpName(op_type);
@@ -3182,25 +3255,34 @@ extern "C" void delitem(Box* target, Box* slice) {
 }
 
 void Box::delattr(const std::string& attr, DelattrRewriteArgs* rewrite_args) {
-    // as soon as the hcls changes, the guard on hidden class won't pass.
-    HCAttrs* attrs = getAttrsPtr();
-    HiddenClass* hcls = attrs->hcls;
-    HiddenClass* new_hcls = hcls->delAttrToMakeHC(attr);
-
-    // The order of attributes is pertained as delAttrToMakeHC constructs
-    // the new HiddenClass by invoking getOrMakeChild in the prevous order
-    // of remaining attributes
-    int num_attrs = hcls->attr_offsets.size();
-    int offset = hcls->getOffset(attr);
-    assert(offset >= 0);
-    Box** start = attrs->attr_list->attrs;
-    memmove(start + offset, start + offset + 1, (num_attrs - offset - 1) * sizeof(Box*));
+    if (cls->instancesHaveHCAttrs()) {
+        // as soon as the hcls changes, the guard on hidden class won't pass.
+        HCAttrs* attrs = getHCAttrsPtr();
+        HiddenClass* hcls = attrs->hcls;
+        HiddenClass* new_hcls = hcls->delAttrToMakeHC(attr);
+
+        // The order of attributes is pertained as delAttrToMakeHC constructs
+        // the new HiddenClass by invoking getOrMakeChild in the prevous order
+        // of remaining attributes
+        int num_attrs = hcls->attr_offsets.size();
+        int offset = hcls->getOffset(attr);
+        assert(offset >= 0);
+        Box** start = attrs->attr_list->attrs;
+        memmove(start + offset, start + offset + 1, (num_attrs - offset - 1) * sizeof(Box*));
+
+        attrs->hcls = new_hcls;
+
+        // guarantee the size of the attr_list equals the number of attrs
+        int new_size = sizeof(HCAttrs::AttrList) + sizeof(Box*) * (num_attrs - 1);
+        attrs->attr_list = (HCAttrs::AttrList*)gc::gc_realloc(attrs->attr_list, new_size);
+        return;
+    }
 
-    attrs->hcls = new_hcls;
+    if (cls->instancesHaveDictAttrs()) {
+        Py_FatalError("unimplemented");
+    }
 
-    // guarantee the size of the attr_list equals the number of attrs
-    int new_size = sizeof(HCAttrs::AttrList) + sizeof(Box*) * (num_attrs - 1);
-    attrs->attr_list = (HCAttrs::AttrList*)gc::gc_realloc(attrs->attr_list, new_size);
+    abort();
 }
 
 extern "C" void delattr_internal(Box* obj, const std::string& attr, bool allow_custom,
@@ -3332,13 +3414,14 @@ Box* typeNew(Box* _cls, Box* arg1, Box* arg2, Box** _args) {
 
     BoxedClass* made;
 
-    if (base->instancesHaveAttrs()) {
+    if (base->instancesHaveDictAttrs() || base->instancesHaveHCAttrs()) {
         made = new (cls) BoxedHeapClass(base, NULL, base->attrs_offset, base->tp_basicsize, true);
     } else {
         assert(base->tp_basicsize % sizeof(void*) == 0);
         made = new (cls) BoxedHeapClass(base, NULL, base->tp_basicsize, base->tp_basicsize + sizeof(HCAttrs), true);
     }
 
+    made->tp_dictoffset = base->tp_dictoffset;
     made->giveAttr("__module__", boxString(getCurrentModule()->name()));
     made->giveAttr("__doc__", None);
 
@@ -3373,6 +3456,7 @@ Box* typeCallInternal(BoxedFunction* f, CallRewriteArgs* rewrite_args, ArgPassSp
     // TODO shouldn't have to redo this argument handling here...
     if (argspec.has_starargs) {
         rewrite_args = NULL;
+        REWRITE_ABORTED("");
 
         Box* starargs;
         if (argspec.num_args == 0)
@@ -3452,6 +3536,7 @@ Box* typeCallInternal(BoxedFunction* f, CallRewriteArgs* rewrite_args, ArgPassSp
             if (descr_r) {
                 new_attr = descr_r;
                 rewrite_args = NULL;
+                REWRITE_ABORTED("");
             }
         }
     } else {
@@ -3516,6 +3601,7 @@ Box* typeCallInternal(BoxedFunction* f, CallRewriteArgs* rewrite_args, ArgPassSp
             // ASSERT(cls->is_user_defined || cls == type_cls, "Does '%s' have a well-behaved __new__?  if so, add to
             // allowable_news, otherwise add to the blacklist in this assert", cls->tp_name);
             rewrite_args = NULL;
+            REWRITE_ABORTED("");
         }
     }
 
@@ -3800,7 +3886,7 @@ extern "C" Box* importStar(Box* _from_module, BoxedModule* to_module) {
         return None;
     }
 
-    HCAttrs* module_attrs = from_module->getAttrsPtr();
+    HCAttrs* module_attrs = from_module->getHCAttrsPtr();
     for (auto& p : module_attrs->hcls->attr_offsets) {
         if (p.first[0] == '_')
             continue;

src/runtime/types.cpp

@@ -349,8 +349,8 @@ extern "C" void boxGCHandler(GCVisitor* v, Box* b) {
     if (b->cls) {
         v->visit(b->cls);
 
-        if (b->cls->instancesHaveAttrs()) {
-            HCAttrs* attrs = b->getAttrsPtr();
+        if (b->cls->instancesHaveHCAttrs()) {
+            HCAttrs* attrs = b->getHCAttrsPtr();
 
             v->visit(attrs->hcls);
             int nattrs = attrs->hcls->attr_offsets.size();
@@ -361,6 +361,10 @@ extern "C" void boxGCHandler(GCVisitor* v, Box* b) {
                 v->visitRange((void**)&attr_list->attrs[0], (void**)&attr_list->attrs[nattrs]);
             }
         }
+
+        if (b->cls->instancesHaveDictAttrs()) {
+            RELEASE_ASSERT(0, "Shouldn't all of these objects be conservatively scanned?");
+        }
     } else {
         assert(type_cls == NULL || b == type_cls);
     }
@@ -776,7 +780,7 @@ private:
     Box* b;
 
 public:
-    AttrWrapper(Box* b) : b(b) {}
+    AttrWrapper(Box* b) : b(b) { assert(b->cls->instancesHaveHCAttrs()); }
 
     DEFAULT_CLASS(attrwrapper_cls);
 
@@ -829,7 +833,7 @@ public:
         std::ostringstream os("");
         os << "attrwrapper({";
 
-        HCAttrs* attrs = self->b->getAttrsPtr();
+        HCAttrs* attrs = self->b->getHCAttrsPtr();
         bool first = true;
         for (const auto& p : attrs->hcls->attr_offsets) {
             if (!first)
@@ -859,7 +863,7 @@ public:
 
         BoxedList* rtn = new BoxedList();
 
-        HCAttrs* attrs = self->b->getAttrsPtr();
+        HCAttrs* attrs = self->b->getHCAttrsPtr();
         for (const auto& p : attrs->hcls->attr_offsets) {
             BoxedTuple* t = new BoxedTuple({ boxString(p.first), attrs->attr_list->attrs[p.second] });
             listAppend(rtn, t);
@@ -869,7 +873,7 @@ public:
 };
 
 Box* makeAttrWrapper(Box* b) {
-    assert(b->cls->instancesHaveAttrs());
+    assert(b->cls->instancesHaveHCAttrs());
     return new AttrWrapper(b);
 }
 
@@ -908,8 +912,8 @@ Box* objectStr(Box* obj) {
 // Added as parameter because it should typically be available
 inline void initUserAttrs(Box* obj, BoxedClass* cls) {
     assert(obj->cls == cls);
-    if (cls->attrs_offset) {
-        HCAttrs* attrs = obj->getAttrsPtr();
+    if (cls->instancesHaveHCAttrs()) {
+        HCAttrs* attrs = obj->getHCAttrsPtr();
         attrs = new ((void*)attrs) HCAttrs();
     }
 }

src/runtime/types.h

@@ -199,7 +199,8 @@ public:
     // Analogous to tp_dictoffset
     const int attrs_offset;
 
-    bool instancesHaveAttrs() { return attrs_offset != 0; }
+    bool instancesHaveHCAttrs() { return attrs_offset != 0; }
+    bool instancesHaveDictAttrs() { return tp_dictoffset != 0; }
 
     // Whether this class object is constant or not, ie whether or not class-level
     // attributes can be changed or added.

test/test_extension/slots_test.c

 #include <Python.h>
+#include <stddef.h> /* For offsetof */
 
 typedef struct {
-    PyObject_HEAD
+    PyObject_HEAD;
+
+    PyObject* dict;
 
     int n;
 } slots_tester_object;
@@ -252,7 +255,7 @@ static PyTypeObject slots_tester_map= {
     0,                                  /* tp_dict */
     0,                                  /* tp_descr_get */
     0,                                  /* tp_descr_set */
-    0,                                  /* tp_dictoffset */
+    offsetof(slots_tester_object, dict), /* tp_dictoffset */
     0,                                  /* tp_init */
     0,                                  /* tp_alloc */
     slots_tester_new,                   /* tp_new */

test/tests/capi_slots.py

@@ -88,3 +88,27 @@ try:
         pass
 except TypeError, e:
     print e
+
+try:
+    slots_test.SlotsTesterSeq(5).foo = 1
+except AttributeError, e:
+    print e
+
+try:
+    print slots_test.SlotsTesterSeq(5).__dict__
+except AttributeError, e:
+    print e
+
+c = C3(5)
+c.foo = 1
+print c.foo
+print c.__dict__.items()
+
+s = slots_test.SlotsTesterMap(6)
+s.bar = 2
+print s.bar
+print hasattr(s, "bar"), hasattr(s, "foo")
+try:
+    print s.__dict__
+except AttributeError, e:
+    print e