Merge pull request #1102 from kmod/getattr_exception_rewriting

Add a ReturnConvention for getattrs that can C++ throw

src/capi/typeobject.cpp

@@ -1013,6 +1013,8 @@ Box* slotTpGetattrHookInternal(Box* self, BoxedString* name, GetattrRewriteArgs*
                     return res;
                 } else if (return_convention == ReturnConvention::NO_RETURN) {
                     assert(!res);
+                } else if (return_convention == ReturnConvention::MAYBE_EXC) {
+                    rewrite_args = NULL;
                 } else if (return_convention == ReturnConvention::CAPI_RETURN
                            || return_convention == ReturnConvention::NOEXC_POSSIBLE) {
                     // If we get a CAPI return, we probably did a function call, and these guards

src/runtime/builtin_modules/builtins.cpp

@@ -576,8 +576,17 @@ Box* getattrFuncInternal(BoxedFunctionBase* func, CallRewriteArgs* rewrite_args,
             std::tie(r_rtn, return_convention) = grewrite_args.getReturn();
 
             // Convert to NOEXC_POSSIBLE:
-            if (return_convention == ReturnConvention::NO_RETURN)
+            if (return_convention == ReturnConvention::NO_RETURN) {
+                return_convention = ReturnConvention::NOEXC_POSSIBLE;
                 r_rtn = rewrite_args->rewriter->loadConst(0);
+            } else if (return_convention == ReturnConvention::MAYBE_EXC) {
+                if (default_value)
+                    rewrite_args = NULL;
+            }
+            assert(!rewrite_args || return_convention == ReturnConvention::NOEXC_POSSIBLE
+                   || return_convention == ReturnConvention::HAS_RETURN
+                   || return_convention == ReturnConvention::CAPI_RETURN
+                   || (default_value == NULL && return_convention == ReturnConvention::MAYBE_EXC));
         }
     } else {
         rtn = getattrInternal<CAPI>(obj, str);
@@ -687,8 +696,15 @@ Box* hasattrFuncInternal(BoxedFunctionBase* func, CallRewriteArgs* rewrite_args,
             std::tie(r_rtn, return_convention) = grewrite_args.getReturn();
 
             // Convert to NOEXC_POSSIBLE:
-            if (return_convention == ReturnConvention::NO_RETURN)
+            if (return_convention == ReturnConvention::NO_RETURN) {
+                return_convention = ReturnConvention::NOEXC_POSSIBLE;
                 r_rtn = rewrite_args->rewriter->loadConst(0);
+            } else if (return_convention == ReturnConvention::MAYBE_EXC) {
+                rewrite_args = NULL;
+            }
+            assert(!rewrite_args || return_convention == ReturnConvention::NOEXC_POSSIBLE
+                   || return_convention == ReturnConvention::HAS_RETURN
+                   || return_convention == ReturnConvention::CAPI_RETURN);
         }
     } else {
         rtn = getattrInternal<CAPI>(obj, str);

src/runtime/objmodel.cpp

@@ -1470,7 +1470,7 @@ Box* dataDescriptorInstanceSpecialCases(GetattrRewriteArgs* rewrite_args, BoxedS
             if (!crewrite_args.out_success) {
                 rewrite_args = NULL;
             } else {
-                rewrite_args->setReturn(crewrite_args.out_rtn, ReturnConvention::HAS_RETURN);
+                rewrite_args->setReturn(crewrite_args.out_rtn, ReturnConvention::MAYBE_EXC);
             }
             return rtn;
         }
@@ -1498,6 +1498,8 @@ Box* dataDescriptorInstanceSpecialCases(GetattrRewriteArgs* rewrite_args, BoxedS
             rewrite_args = NULL;
         }
 
+        Box* rtn = getset_descr->get(obj, getset_descr->closure);
+
         if (rewrite_args) {
             // hmm, maybe we should write assembly which can look up the function address and call any function
             r_descr->addAttrGuard(offsetof(BoxedGetsetDescriptor, get), (intptr_t)getset_descr->get);
@@ -1507,10 +1509,9 @@ Box* dataDescriptorInstanceSpecialCases(GetattrRewriteArgs* rewrite_args, BoxedS
                 /* has_side_effects */ true, (void*)getset_descr->get, rewrite_args->obj, r_closure);
 
             rewrite_args->setReturn(r_rtn, descr->cls == capi_getset_cls ? ReturnConvention::CAPI_RETURN
-                                                                         : ReturnConvention::HAS_RETURN);
+                                                                         : ReturnConvention::MAYBE_EXC);
         }
-
-        return getset_descr->get(obj, getset_descr->closure);
+        return rtn;
     }
 
     return NULL;
@@ -1693,7 +1694,12 @@ extern "C" Box* getclsattr(Box* obj, BoxedString* attr) {
         gotten = getclsattrInternal<REWRITABLE>(obj, attr, &rewrite_args);
 
         if (rewrite_args.isSuccessful() && gotten) {
-            RewriterVar* r_rtn = rewrite_args.getReturn(ReturnConvention::HAS_RETURN);
+            RewriterVar* r_rtn;
+            ReturnConvention return_convention;
+            std::tie(r_rtn, return_convention) = rewrite_args.getReturn();
+
+            assert(return_convention == ReturnConvention::HAS_RETURN
+                   || return_convention == ReturnConvention::MAYBE_EXC);
             rewriter->commitReturning(r_rtn);
         }
 #endif

src/runtime/rewrite_args.h

@@ -49,6 +49,7 @@ enum class ReturnConvention {
     NO_RETURN,
     CAPI_RETURN,
     NOEXC_POSSIBLE,
+    MAYBE_EXC,
 };
 
 class _ReturnConventionBase {
@@ -99,6 +100,8 @@ public:
                     assert(!PyErr_Occurred());
                 } else if (r == ReturnConvention::CAPI_RETURN) {
                     assert((bool)b ^ (bool)PyErr_Occurred());
+                } else if (r == ReturnConvention::MAYBE_EXC) {
+                    assert(b);
                 } else {
                     assert(r == ReturnConvention::NOEXC_POSSIBLE);
                 }

test/tests/getattr_start_failing_test.py

+g = 0
+class C(object):
+    @property
+    def f(self):
+        print "f"
+        if g:
+            raise AttributeError()
+
+c = C()
+
+for i in xrange(100):
+    print i, getattr(c, 'f', 0)
+
+    if i == 50:
+        g = 1
+