Merge pull request #747 from kmod/getattr

Improve __getattr__ handling

from_cpython/Include/object.h

@@ -457,7 +457,7 @@ struct _typeobject {
     char _ics[32];
     void* _gcvisit_func;
     int _attrs_offset;
-    bool _flags[6];
+    bool _flags[7];
     void* _tpp_descr_get;
     void* _tpp_hasnext;
     void* _tpp_call;

src/capi/abstract.cpp

@@ -17,6 +17,7 @@
 
 #include "Python.h"
 
+#include "capi/typeobject.h"
 #include "capi/types.h"
 #include "core/ast.h"
 #include "core/threading.h"
@@ -405,12 +406,22 @@ static int recursive_isinstance(PyObject* inst, PyObject* cls) noexcept {
         retval = PyObject_TypeCheck(inst, (PyTypeObject*)cls);
         if (retval == 0) {
             PyObject* c = NULL;
-            if (inst->cls->has___class__ || inst->cls->tp_getattr != object_cls->tp_getattr
-                || inst->cls->tp_getattro != object_cls->tp_getattro) {
+
+            if (!inst->cls->has_getattribute) {
+                assert(inst->cls->tp_getattr == object_cls->tp_getattr);
+                assert(inst->cls->tp_getattro == object_cls->tp_getattro
+                       || inst->cls->tp_getattro == slot_tp_getattr_hook);
+            }
+            // We don't need to worry about __getattr__, since the default __class__ will always resolve.
+            bool has_custom_class = inst->cls->has___class__ || inst->cls->has_getattribute;
+            if (!has_custom_class) {
+                assert(PyObject_GetAttr(inst, __class__) == inst->cls);
+            } else {
                 c = PyObject_GetAttr(inst, __class__);
                 if (!c)
                     PyErr_Clear();
             }
+
             if (c) {
                 if (c != (PyObject*)(inst->cls) && PyType_Check(c))
                     retval = PyType_IsSubtype((PyTypeObject*)c, (PyTypeObject*)cls);

src/capi/typeobject.cpp

@@ -922,7 +922,17 @@ static PyObject* call_attribute(PyObject* self, PyObject* attr, PyObject* name)
     return res;
 }
 
-static PyObject* slot_tp_getattr_hook(PyObject* self, PyObject* name) noexcept {
+/* Pyston change: static */ PyObject* slot_tp_getattr_hook(PyObject* self, PyObject* name) noexcept {
+    try {
+        assert(name->cls == str_cls);
+        return slotTpGetattrHookInternal(self, (BoxedString*)name, NULL);
+    } catch (ExcInfo e) {
+        setCAPIException(e);
+        return NULL;
+    }
+}
+
+Box* slotTpGetattrHookInternal(Box* self, BoxedString* name, GetattrRewriteArgs* rewrite_args) {
     STAT_TIMER(t0, "us_timer_slot_tpgetattrhook", SLOT_AVOIDABILITY(self));
 
     PyObject* getattr, *getattribute, * res = NULL;
@@ -933,30 +943,145 @@ static PyObject* slot_tp_getattr_hook(PyObject* self, PyObject* name) noexcept {
          _PyType_Lookup and create the method only when needed, with
          call_attribute. */
     static BoxedString* _getattr_str = internStringImmortal("__getattr__");
+
+    // Don't need to do this in the rewritten version; if a __getattr__ later gets removed:
+    // - if we ever get to the "call __getattr__" portion of the rewrite, the guards will
+    //   fail and we will end up back here
+    // - if we never get to the "call __getattr__" portion and the "calling __getattribute__"
+    //   portion still has its guards pass, then that section is still behaviorally correct, and
+    //   I think should be close to as fast as the normal rewritten version we would generate.
     getattr = typeLookup(self->cls, _getattr_str, NULL);
+
     if (getattr == NULL) {
+        assert(!rewrite_args || !rewrite_args->out_success);
+
         /* No __getattr__ hook: use a simpler dispatcher */
         self->cls->tp_getattro = slot_tp_getattro;
         return slot_tp_getattro(self, name);
     }
+
     /* speed hack: we could use lookup_maybe, but that would resolve the
          method fully for each attribute lookup for classes with
          __getattr__, even when self has the default __getattribute__
          method. So we use _PyType_Lookup and create the method only when
          needed, with call_attribute. */
     static BoxedString* _getattribute_str = internStringImmortal("__getattribute__");
-    getattribute = typeLookup(self->cls, _getattribute_str, NULL);
+
+    RewriterVar* r_getattribute = NULL;
+    if (rewrite_args) {
+        RewriterVar* r_obj_cls = rewrite_args->obj->getAttr(offsetof(Box, cls), Location::any());
+        GetattrRewriteArgs grewrite_args(rewrite_args->rewriter, r_obj_cls, Location::any());
+
+        getattribute = typeLookup(self->cls, _getattribute_str, &grewrite_args);
+        if (!grewrite_args.out_success)
+            rewrite_args = NULL;
+        else if (getattribute)
+            r_getattribute = grewrite_args.out_rtn;
+    } else {
+        getattribute = typeLookup(self->cls, _getattribute_str, NULL);
+    }
+    // Not sure why CPython checks if getattribute is NULL since I don't think that should happen.
+    // Is there some legacy way of creating types that don't inherit from object?  Anyway, I think we
+    // have the right behavior even if getattribute was somehow NULL, but add an assert because that
+    // case would still be very surprising to me:
+    assert(getattribute);
+
     if (getattribute == NULL
         || (Py_TYPE(getattribute) == wrapperdescr_cls
             && ((BoxedWrapperDescriptor*)getattribute)->wrapped == (void*)PyObject_GenericGetAttr)) {
-        res = PyObject_GenericGetAttr(self, name);
+
+        assert(PyString_CHECK_INTERNED(name));
+        if (rewrite_args) {
+            // Fetching getattribute should have done the appropriate guarding on whether or not
+            // getattribute exists.
+            if (getattribute)
+                r_getattribute->addGuard((intptr_t)getattribute);
+
+            GetattrRewriteArgs grewrite_args(rewrite_args->rewriter, rewrite_args->obj, rewrite_args->destination);
+            try {
+                res = getattrInternalGeneric(self, name, &grewrite_args, false, false, NULL, NULL);
+            } catch (ExcInfo e) {
+                if (!e.matches(AttributeError))
+                    throw e;
+
+                grewrite_args.out_success = false;
+                res = NULL;
+            }
+
+            if (!grewrite_args.out_success)
+                rewrite_args = NULL;
+            else if (res)
+                rewrite_args->out_rtn = grewrite_args.out_rtn;
+        } else {
+            try {
+                res = getattrInternalGeneric(self, name, NULL, false, false, NULL, NULL);
+            } catch (ExcInfo e) {
+                if (!e.matches(AttributeError))
+                    throw e;
+                res = NULL;
+            }
+        }
     } else {
+        rewrite_args = NULL;
+
         res = call_attribute(self, getattribute, name);
+        if (res == NULL) {
+            if (PyErr_ExceptionMatches(PyExc_AttributeError))
+                PyErr_Clear();
+            else
+                throwCAPIException();
+        }
     }
-    if (res == NULL && PyErr_ExceptionMatches(PyExc_AttributeError)) {
-        PyErr_Clear();
-        res = call_attribute(self, getattr, name);
+
+    // At this point, CPython would have three cases: res is non-NULL and no exception was thrown,
+    // or res is NULL and an exception was thrown and either it was an AttributeError (in which case
+    // we call __getattr__) or it wan't (in which case it propagates).
+    //
+    // We handled it differently: if a non-AttributeError was thrown, we already would have propagated
+    // it.  So there are only two cases: res is non-NULL if the attribute exists, or it is NULL if it
+    // doesn't exist.
+
+    if (res) {
+        if (rewrite_args)
+            rewrite_args->out_success = true;
+        return res;
+    }
+
+    assert(!PyErr_Occurred());
+
+    CallattrFlags callattr_flags = {.cls_only = true, .null_on_nonexistent = false, .argspec = ArgPassSpec(1) };
+    if (rewrite_args) {
+        // I was thinking at first that we could try to catch any AttributeErrors here and still
+        // write out valid rewrite, but
+        // - we need to let the original AttributeError propagate and not generate a new, potentially-different one
+        // - we have no way of signalling that "we didn't get an attribute this time but that may be different
+        //   in future executions through the IC".
+        // I think this should only end up mattering anyway if the getattr site throws every single time.
+        CallRewriteArgs crewrite_args(rewrite_args->rewriter, rewrite_args->obj, rewrite_args->destination);
+        assert(PyString_CHECK_INTERNED(name) == SSTATE_INTERNED_IMMORTAL);
+        crewrite_args.arg1 = rewrite_args->rewriter->loadConst((intptr_t)name, Location::forArg(1));
+
+        res = callattrInternal(self, _getattr_str, LookupScope::CLASS_ONLY, &crewrite_args, ArgPassSpec(1), name, NULL,
+                               NULL, NULL, NULL);
+        assert(res);
+
+        if (!crewrite_args.out_success)
+            rewrite_args = NULL;
+        else
+            rewrite_args->out_rtn = crewrite_args.out_rtn;
+    } else {
+        // TODO: we already fetched the getattr attribute, it would be faster to call it rather than do
+        // a second callattr.  My guess though is that the gains would be small, so I would prefer to keep
+        // the rewrite_args and non-rewrite_args case the same.
+        // Actually, we might have gotten to the point that doing a runtimeCall on an instancemethod is as
+        // fast as a callattr, but that hasn't typically been the case.
+        res = callattrInternal(self, _getattr_str, LookupScope::CLASS_ONLY, NULL, ArgPassSpec(1), name, NULL, NULL,
+                               NULL, NULL);
+        assert(res);
     }
+
+    if (rewrite_args)
+        rewrite_args->out_success = true;
     return res;
 }
 
@@ -1490,6 +1615,7 @@ static slotdef slotdefs[]
         TPSLOT("__del__", tp_del, slot_tp_del, NULL, ""),
         FLSLOT("__class__", has___class__, NULL, NULL, "", PyWrapperFlag_BOOL),
         FLSLOT("__instancecheck__", has_instancecheck, NULL, NULL, "", PyWrapperFlag_BOOL),
+        FLSLOT("__getattribute__", has_getattribute, NULL, NULL, "", PyWrapperFlag_BOOL),
         TPPSLOT("__hasnext__", tpp_hasnext, slotTppHasnext, wrapInquirypred, "hasnext"),
 
         BINSLOT("__add__", nb_add, slot_nb_add, "+"),                             // [force clang-format to line break]
@@ -1682,6 +1808,13 @@ static const slotdef* update_one_slot(BoxedClass* type, const slotdef* p) noexce
                     descr = NULL;
             }
 
+            static BoxedString* getattribute_str = internStringImmortal("__getattribute__");
+            if (p->name_strobj == getattribute_str) {
+                if (descr && descr->cls == wrapperdescr_cls
+                    && ((BoxedWrapperDescriptor*)descr)->wrapped == PyObject_GenericGetAttr)
+                    descr = NULL;
+            }
+
             *(bool*)ptr = (bool)descr;
             return p + 1;
         }

src/capi/typeobject.h

@@ -48,6 +48,10 @@ PyObject* slot_tp_new(PyTypeObject* self, PyObject* args, PyObject* kwds) noexce
 PyObject* slot_mp_subscript(PyObject* self, PyObject* arg1) noexcept;
 int slot_sq_contains(PyObject* self, PyObject* value) noexcept;
 Py_ssize_t slot_sq_length(PyObject* self) noexcept;
+PyObject* slot_tp_getattr_hook(PyObject* self, PyObject* name) noexcept;
+
+class GetattrRewriteArgs;
+Box* slotTpGetattrHookInternal(Box* self, BoxedString* attr, GetattrRewriteArgs* rewrite_args);
 }
 
 #endif

src/runtime/objmodel.cpp

@@ -748,7 +748,14 @@ static StatCounter box_getattr_slowpath("slowpath_box_getattr");
 Box* Box::getattr(BoxedString* attr, GetattrRewriteArgs* rewrite_args) {
     assert(attr->interned_state != SSTATE_NOT_INTERNED);
 
-    if (rewrite_args && !rewrite_args->obj_cls_guarded)
+    // We have to guard on the class in order to know the object's layout,
+    // ie to know which kinds of attributes the object has and where they
+    // live in the object's layout.
+    // TODO we could try guarding on those fields directly rather than on
+    // the class itself (which implies all of them).  That might require
+    // creating a single field that encompasses the relevant other fields
+    // so that it can still be a single guard rather than multiple.
+    if (rewrite_args && !rewrite_args->obj_shape_guarded)
         rewrite_args->obj->addAttrGuard(offsetof(Box, cls), (intptr_t)cls);
 
 #if 0
@@ -1053,9 +1060,9 @@ Box* typeLookup(BoxedClass* cls, BoxedString* attr, GetattrRewriteArgs* rewrite_
                 assert(rewrite_args->obj == obj_saved);
             } else {
                 rewrite_args->obj = rewrite_args->rewriter->loadConst((intptr_t)base, Location::any());
-                if (static_cast<BoxedClass*>(base)->is_constant) {
-                    rewrite_args->obj_cls_guarded = true;
-                }
+                // We are passing a constant object, and objects are not allowed to change shape
+                // (at least the kind of "shape" that Box::getattr is referring to)
+                rewrite_args->obj_shape_guarded = true;
             }
             val = base->getattr(attr, rewrite_args);
             assert(rewrite_args->out_success);
@@ -1469,9 +1476,19 @@ Box* getattrInternalEx(Box* obj, BoxedString* attr, GetattrRewriteArgs* rewrite_
 
     if (!cls_only) {
         BoxedClass* cls = obj->cls;
+
+        // We could also use the old invalidation-based approach here:
+        if (rewrite_args)
+            rewrite_args->obj->getAttr(offsetof(Box, cls))
+                ->addAttrGuard(offsetof(BoxedClass, tp_getattro), (uint64_t)obj->cls->tp_getattro);
+
         if (obj->cls->tp_getattro && obj->cls->tp_getattro != PyObject_GenericGetAttr) {
             STAT_TIMER(t0, "us_timer_slowpath_tpgetattro", 10);
 
+            if (obj->cls->tp_getattro == slot_tp_getattr_hook) {
+                return slotTpGetattrHookInternal(obj, attr, rewrite_args);
+            }
+
             Box* r = obj->cls->tp_getattro(obj, attr);
             if (!r)
                 throwCAPIException();
@@ -1481,10 +1498,6 @@ Box* getattrInternalEx(Box* obj, BoxedString* attr, GetattrRewriteArgs* rewrite_
             // around the fact that we don't currently scan ICs for GC references, but eventually
             // we should just add that.
             if (rewrite_args && attr->interned_state == SSTATE_INTERNED_IMMORTAL) {
-                // In theory we could also just guard that the tp_getattro slot is non-null and then call
-                // into it, instead of guarding that it is the same as it is here.
-                rewrite_args->obj->getAttr(offsetof(Box, cls))
-                    ->addAttrGuard(offsetof(BoxedClass, tp_getattro), (uint64_t)obj->cls->tp_getattro);
                 auto r_box = rewrite_args->rewriter->loadConst((intptr_t)attr);
                 auto r_rtn = rewrite_args->rewriter->call(true, (void*)obj->cls->tp_getattro, rewrite_args->obj, r_box);
                 rewrite_args->rewriter->call(true, (void*)checkAndThrowCAPIException);
@@ -1495,6 +1508,11 @@ Box* getattrInternalEx(Box* obj, BoxedString* attr, GetattrRewriteArgs* rewrite_
             return r;
         }
 
+        // We could also use the old invalidation-based approach here:
+        if (rewrite_args)
+            rewrite_args->obj->getAttr(offsetof(Box, cls))
+                ->addAttrGuard(offsetof(BoxedClass, tp_getattr), (uint64_t)obj->cls->tp_getattr);
+
         if (obj->cls->tp_getattr) {
             STAT_TIMER(t0, "us_timer_slowpath_tpgetattr", 10);
 
@@ -1504,13 +1522,6 @@ Box* getattrInternalEx(Box* obj, BoxedString* attr, GetattrRewriteArgs* rewrite_
                 throwCAPIException();
             return r;
         }
-
-        // We could also use the old invalidation-based approach here:
-        if (rewrite_args) {
-            auto r_cls = rewrite_args->obj->getAttr(offsetof(Box, cls));
-            r_cls->addAttrGuard(offsetof(BoxedClass, tp_getattr), (uint64_t)obj->cls->tp_getattr);
-            r_cls->addAttrGuard(offsetof(BoxedClass, tp_getattro), (uint64_t)obj->cls->tp_getattro);
-        }
     }
 
     return getattrInternalGeneric(obj, attr, rewrite_args, cls_only, for_call, bind_obj_out, r_bind_obj_out);
@@ -3780,7 +3791,7 @@ Box* runtimeCallInternal(Box* obj, CallRewriteArgs* rewrite_args, ArgPassSpec ar
 
         RewriterVar* r_im_func;
         if (rewrite_args) {
-            r_im_func = rewrite_args->obj->getAttr(offsetof(BoxedInstanceMethod, obj), Location::any());
+            r_im_func = rewrite_args->obj->getAttr(offsetof(BoxedInstanceMethod, func), Location::any());
         }
 
         if (rewrite_args && !rewrite_args->func_guarded) {
@@ -5283,7 +5294,7 @@ extern "C" Box* getGlobal(Box* globals, BoxedString* name) {
         if (rewriter.get()) {
             RewriterVar* builtins = rewriter->loadConst((intptr_t)builtins_module, Location::any());
             GetattrRewriteArgs rewrite_args(rewriter.get(), builtins, rewriter->getReturnDestination());
-            rewrite_args.obj_cls_guarded = true; // always builtin module
+            rewrite_args.obj_shape_guarded = true; // always builtin module
             rtn = builtins_module->getattr(name, &rewrite_args);
 
             if (!rtn || !rewrite_args.out_success) {

src/runtime/rewrite_args.h

@@ -28,7 +28,7 @@ struct GetattrRewriteArgs {
     RewriterVar* out_rtn;
 
     bool obj_hcls_guarded;
-    bool obj_cls_guarded;
+    bool obj_shape_guarded; // "shape" as in whether there are hcls attrs and where they live
 
     GetattrRewriteArgs(Rewriter* rewriter, RewriterVar* obj, Location destination)
         : rewriter(rewriter),
@@ -37,7 +37,7 @@ struct GetattrRewriteArgs {
           out_success(false),
           out_rtn(NULL),
           obj_hcls_guarded(false),
-          obj_cls_guarded(false) {}
+          obj_shape_guarded(false) {}
 };
 
 struct SetattrRewriteArgs {

src/runtime/types.h

@@ -232,6 +232,7 @@ public:
 
     bool has___class__; // Has a custom __class__ attribute (ie different from object's __class__ descriptor)
     bool has_instancecheck;
+    bool has_getattribute;
 
     typedef bool (*pyston_inquiry)(Box*);
 

test/tests/55.py

@@ -13,13 +13,13 @@ class C(object):
     __metaclass__ = M
 
     def __getattribute__(self, attr):
-        print "C.__getattribute__"
+        print "C.__getattribute__", attr
         if attr == "n":
             return 1
         return object.__getattribute__(self, attr)
 
     def __getattr__(self, attr):
-        print "C.__getattr__"
+        print "C.__getattr__", attr
         if attr == "m":
             return 2
         return object.__getattr__(self, attr)

test/tests/isinstance.py

@@ -91,3 +91,35 @@ print "checking superclass instancecheck:"
 print isinstance(1, C)
 M.__instancecheck__ =  m_instancecheck
 print isinstance(1, C)
+print
+
+class C(object):
+    @property
+    def __class__(self):
+        print "C.__class__ descriptor"
+        raise AttributeError()
+
+    def __getattr__(self, attr):
+        print "C.__getattr__", attr
+        return int
+
+print "Testing __class__ from __getattr__"
+c = C()
+print c.__class__
+print isinstance(c, int)
+print
+
+class C(object):
+    def __getattribute__(self, attr):
+        print "C.__getattribute__", attr
+        raise AttributeError()
+
+    def __getattr__(self, attr):
+        print "C.__getattr__", attr
+        return int
+
+print "Testing __class__ from __getattr__+__getattribute__"
+c = C()
+print c.__class__
+print isinstance(c, int)
+print