Merge pull request #949 from kmod/return_conventions

Add more asserts to aggressively enforce return conventions

src/asm_writing/rewriter.h

@@ -277,6 +277,8 @@ public:
         assert(rewriter);
     }
 
+    Rewriter* getRewriter() { return rewriter; }
+
     friend class Rewriter;
     friend class JitFragmentWriter;
 };

src/capi/typeobject.cpp

@@ -908,7 +908,11 @@ Box* slotTpGetattrHookInternal(Box* self, BoxedString* name, GetattrRewriteArgs*
     getattr = typeLookup(self->cls, _getattr_str, NULL);
 
     if (getattr == NULL) {
-        assert(!rewrite_args || !rewrite_args->out_success);
+        if (rewrite_args) {
+            // Don't bother rewriting this case:
+            assert(!rewrite_args->isSuccessful());
+            rewrite_args = NULL;
+        }
 
         /* No __getattr__ hook: use a simpler dispatcher */
         self->cls->tp_getattro = slot_tp_getattro;
@@ -931,13 +935,12 @@ Box* slotTpGetattrHookInternal(Box* self, BoxedString* name, GetattrRewriteArgs*
         GetattrRewriteArgs grewrite_args(rewrite_args->rewriter, r_obj_cls, Location::any());
 
         getattribute = typeLookup(self->cls, _getattribute_str, &grewrite_args);
-        if (!grewrite_args.out_success)
+        if (!grewrite_args.isSuccessful())
             rewrite_args = NULL;
         else if (getattribute) {
-            assert(grewrite_args.out_return_convention == GetattrRewriteArgs::VALID_RETURN);
-            r_getattribute = grewrite_args.out_rtn;
+            r_getattribute = grewrite_args.getReturn(ReturnConvention::HAS_RETURN);
         } else {
-            assert(grewrite_args.out_return_convention == GetattrRewriteArgs::NO_RETURN);
+            grewrite_args.assertReturnConvention(ReturnConvention::NO_RETURN);
         }
     } else {
         getattribute = typeLookup(self->cls, _getattribute_str, NULL);
@@ -973,35 +976,38 @@ Box* slotTpGetattrHookInternal(Box* self, BoxedString* name, GetattrRewriteArgs*
                         throw e;
                 }
 
-                grewrite_args.out_success = false;
+                if (grewrite_args.isSuccessful()) {
+                    grewrite_args.getReturn(); // to make the asserts happy
+                    grewrite_args.clearReturn();
+                }
                 res = NULL;
             }
 
-            if (!grewrite_args.out_success)
+            if (!grewrite_args.isSuccessful())
                 rewrite_args = NULL;
-            else if (res) {
-                rewrite_args->out_rtn = grewrite_args.out_rtn;
-                rewrite_args->out_return_convention = grewrite_args.out_return_convention;
-            }
+            else {
+                RewriterVar* rtn;
+                ReturnConvention return_convention;
+                std::tie(rtn, return_convention) = grewrite_args.getReturn();
 
-            // Guarding here is a bit tricky, since we need to make sure that we call getattr
-            // (or not) at the right times.
-            // Right now this section is a bit conservative.
-            if (rewrite_args) {
-                if (grewrite_args.out_return_convention == GetattrRewriteArgs::NO_RETURN) {
-                    // Do nothing
-                } else if (grewrite_args.out_return_convention == GetattrRewriteArgs::VALID_RETURN) {
-                    // TODO we should have a HAS_RETURN that takes out the NULL case
+                if (return_convention == ReturnConvention::HAS_RETURN) {
                     assert(res);
-                    if (res)
-                        grewrite_args.out_rtn->addGuardNotEq(0);
-                    else
-                        grewrite_args.out_rtn->addGuard(0);
-                } else if (grewrite_args.out_return_convention == GetattrRewriteArgs::NOEXC_POSSIBLE) {
-                    // TODO maybe we could handle this
-                    rewrite_args = NULL;
+                    rewrite_args->setReturn(rtn, ReturnConvention::HAS_RETURN);
+                    return res;
+                } else if (return_convention == ReturnConvention::NO_RETURN) {
+                    assert(!res);
+                } else if (return_convention == ReturnConvention::CAPI_RETURN) {
+                    // If we get a CAPI return, we probably did a function call, and these guards
+                    // will probably just make the rewrite fail:
+                    if (res) {
+                        rtn->addGuardNotEq(0);
+                        rewrite_args->setReturn(rtn, ReturnConvention::HAS_RETURN);
+                        return res;
+                    } else
+                        rtn->addGuard(0);
                 } else {
-                    RELEASE_ASSERT(0, "%d", grewrite_args.out_return_convention);
+                    assert(return_convention == ReturnConvention::NOEXC_POSSIBLE);
+                    rewrite_args = NULL;
                 }
             }
         } else {
@@ -1044,8 +1050,7 @@ Box* slotTpGetattrHookInternal(Box* self, BoxedString* name, GetattrRewriteArgs*
     // doesn't exist.
 
     if (res) {
-        if (rewrite_args)
-            rewrite_args->out_success = true;
+        assert(!rewrite_args); // should have been handled already
         return res;
     }
 
@@ -1059,7 +1064,7 @@ Box* slotTpGetattrHookInternal(Box* self, BoxedString* name, GetattrRewriteArgs*
         // - 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);
+        CallattrRewriteArgs 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));
 
@@ -1067,11 +1072,10 @@ Box* slotTpGetattrHookInternal(Box* self, BoxedString* name, GetattrRewriteArgs*
                                   NULL, NULL, NULL, NULL);
         assert(res || S == CAPI);
 
-        if (!crewrite_args.out_success)
+        if (!crewrite_args.isSuccessful())
             rewrite_args = NULL;
         else {
-            rewrite_args->out_rtn = crewrite_args.out_rtn;
-            rewrite_args->out_return_convention = GetattrRewriteArgs::VALID_RETURN;
+            rewrite_args->setReturn(crewrite_args.getReturn());
         }
     } else {
         // TODO: we already fetched the getattr attribute, it would be faster to call it rather than do
@@ -1084,8 +1088,6 @@ Box* slotTpGetattrHookInternal(Box* self, BoxedString* name, GetattrRewriteArgs*
         assert(res || S == CAPI);
     }
 
-    if (rewrite_args)
-        rewrite_args->out_success = true;
     return res;
 }
 // Force instantiation of the template

src/runtime/builtin_modules/builtins.cpp

@@ -569,16 +569,15 @@ Box* getattrFuncInternal(BoxedFunctionBase* func, CallRewriteArgs* rewrite_args,
         GetattrRewriteArgs grewrite_args(rewrite_args->rewriter, rewrite_args->arg1, rewrite_args->destination);
         rtn = getattrInternal<CAPI>(obj, str, &grewrite_args);
         // TODO could make the return valid in the NOEXC_POSSIBLE case via a helper
-        if (!grewrite_args.out_success || grewrite_args.out_return_convention == GetattrRewriteArgs::NOEXC_POSSIBLE)
+        if (!grewrite_args.isSuccessful())
             rewrite_args = NULL;
         else {
-            if (!rtn && !PyErr_Occurred()) {
-                assert(grewrite_args.out_return_convention == GetattrRewriteArgs::NO_RETURN);
+            ReturnConvention return_convention;
+            std::tie(r_rtn, return_convention) = grewrite_args.getReturn();
+
+            // Convert to NOEXC_POSSIBLE:
+            if (return_convention == ReturnConvention::NO_RETURN)
                 r_rtn = rewrite_args->rewriter->loadConst(0);
-            } else {
-                assert(grewrite_args.out_return_convention == GetattrRewriteArgs::VALID_RETURN);
-                r_rtn = grewrite_args.out_rtn;
-            }
         }
     } else {
         rtn = getattrInternal<CAPI>(obj, str, NULL);
@@ -681,16 +680,15 @@ Box* hasattrFuncInternal(BoxedFunctionBase* func, CallRewriteArgs* rewrite_args,
     if (rewrite_args) {
         GetattrRewriteArgs grewrite_args(rewrite_args->rewriter, rewrite_args->arg1, rewrite_args->destination);
         rtn = getattrInternal<CAPI>(obj, str, &grewrite_args);
-        if (!grewrite_args.out_success || grewrite_args.out_return_convention == GetattrRewriteArgs::NOEXC_POSSIBLE)
+        if (!grewrite_args.isSuccessful())
             rewrite_args = NULL;
         else {
-            if (!rtn && !PyErr_Occurred()) {
-                assert(grewrite_args.out_return_convention == GetattrRewriteArgs::NO_RETURN);
+            ReturnConvention return_convention;
+            std::tie(r_rtn, return_convention) = grewrite_args.getReturn();
+
+            // Convert to NOEXC_POSSIBLE:
+            if (return_convention == ReturnConvention::NO_RETURN)
                 r_rtn = rewrite_args->rewriter->loadConst(0);
-            } else {
-                assert(grewrite_args.out_return_convention == GetattrRewriteArgs::VALID_RETURN);
-                r_rtn = grewrite_args.out_rtn;
-            }
         }
     } else {
         rtn = getattrInternal<CAPI>(obj, str, NULL);

src/runtime/classobj.cpp

@@ -32,15 +32,20 @@ BoxedClass* classobj_cls, *instance_cls;
 
 static Box* classLookup(BoxedClassobj* cls, BoxedString* attr, GetattrRewriteArgs* rewrite_args = NULL) {
     if (rewrite_args)
-        assert(!rewrite_args->out_success);
+        assert(!rewrite_args->isSuccessful());
 
     Box* r = cls->getattr(attr, rewrite_args);
-    if (r)
+    if (r) {
+        if (rewrite_args)
+            rewrite_args->assertReturnConvention(ReturnConvention::HAS_RETURN);
         return r;
+    }
 
     if (rewrite_args) {
-        // abort rewriting because we currenly don't guard the particular 'bases' hierarchy
-        rewrite_args->out_success = false;
+        if (rewrite_args->isSuccessful()) {
+            rewrite_args->getReturn(); // just to make the asserts happy
+            rewrite_args->clearReturn();
+        }
         rewrite_args = NULL;
     }
 
@@ -322,62 +327,71 @@ Box* classobjRepr(Box* _obj) {
 
 // Analogous to CPython's instance_getattr2
 static Box* instanceGetattributeSimple(BoxedInstance* inst, BoxedString* attr_str,
-                                       GetattrRewriteArgs* rewriter_args = NULL) {
-    Box* r = inst->getattr(attr_str, rewriter_args);
-    if (r)
+                                       GetattrRewriteArgs* rewrite_args = NULL) {
+    Box* r = inst->getattr(attr_str, rewrite_args);
+    if (r) {
+        if (rewrite_args)
+            rewrite_args->assertReturnConvention(ReturnConvention::HAS_RETURN);
         return r;
+    }
 
     RewriterVar* r_inst = NULL;
     RewriterVar* r_inst_cls = NULL;
-    if (rewriter_args) {
-        if (!rewriter_args->out_success)
-            rewriter_args = NULL;
+    if (rewrite_args) {
+        if (!rewrite_args->isSuccessful())
+            rewrite_args = NULL;
         else {
-            rewriter_args->out_success = false;
-            r_inst = rewriter_args->obj;
+            rewrite_args->assertReturnConvention(ReturnConvention::NO_RETURN);
+            rewrite_args->clearReturn();
+
+            r_inst = rewrite_args->obj;
             r_inst_cls = r_inst->getAttr(offsetof(BoxedInstance, inst_cls));
         }
     }
-    GetattrRewriteArgs grewriter_inst_args(rewriter_args ? rewriter_args->rewriter : NULL, r_inst_cls,
-                                           rewriter_args ? rewriter_args->rewriter->getReturnDestination()
-                                                         : Location());
-    r = classLookup(inst->inst_cls, attr_str, rewriter_args ? &grewriter_inst_args : NULL);
-    if (!grewriter_inst_args.out_success)
-        rewriter_args = NULL;
-    else
-        assert(grewriter_inst_args.out_return_convention == GetattrRewriteArgs::VALID_RETURN);
+    GetattrRewriteArgs grewriter_inst_args(rewrite_args ? rewrite_args->rewriter : NULL, r_inst_cls,
+                                           rewrite_args ? rewrite_args->rewriter->getReturnDestination() : Location());
+    r = classLookup(inst->inst_cls, attr_str, rewrite_args ? &grewriter_inst_args : NULL);
+    if (!grewriter_inst_args.isSuccessful())
+        rewrite_args = NULL;
 
     if (r) {
         Box* rtn = processDescriptor(r, inst, inst->inst_cls);
-        if (rewriter_args) {
-            RewriterVar* r_rtn = rewriter_args->rewriter->call(true, (void*)processDescriptor,
-                                                               grewriter_inst_args.out_rtn, r_inst, r_inst_cls);
-            rewriter_args->out_rtn = r_rtn;
-            rewriter_args->out_success = true;
-            rewriter_args->out_return_convention = GetattrRewriteArgs::VALID_RETURN;
+        if (rewrite_args) {
+            RewriterVar* r_rtn = rewrite_args->rewriter->call(
+                true, (void*)processDescriptor, grewriter_inst_args.getReturn(ReturnConvention::HAS_RETURN), r_inst,
+                r_inst_cls);
+            rewrite_args->setReturn(r_rtn, ReturnConvention::HAS_RETURN);
         }
         return rtn;
     }
 
+    if (rewrite_args)
+        grewriter_inst_args.assertReturnConvention(ReturnConvention::NO_RETURN);
+
     return NULL;
 }
 
 static Box* instanceGetattributeWithFallback(BoxedInstance* inst, BoxedString* attr_str,
-                                             GetattrRewriteArgs* rewriter_args = NULL) {
-    Box* attr_obj = instanceGetattributeSimple(inst, attr_str, rewriter_args);
+                                             GetattrRewriteArgs* rewrite_args = NULL) {
+    Box* attr_obj = instanceGetattributeSimple(inst, attr_str, rewrite_args);
 
     if (attr_obj) {
+        if (rewrite_args && rewrite_args->isSuccessful())
+            rewrite_args->assertReturnConvention(
+                ReturnConvention::HAS_RETURN); // otherwise need to guard on the success
         return attr_obj;
     }
 
-    if (rewriter_args) {
-        if (!rewriter_args->out_success)
-            rewriter_args = NULL;
-        else
-            rewriter_args->out_success = false;
+    if (rewrite_args) {
+        if (!rewrite_args->isSuccessful())
+            rewrite_args = NULL;
+        else {
+            rewrite_args->assertReturnConvention(ReturnConvention::NO_RETURN);
+            rewrite_args->clearReturn();
+        }
 
         // abort rewriting for now
-        rewriter_args = NULL;
+        rewrite_args = NULL;
     }
 
     static BoxedString* getattr_str = internStringImmortal("__getattr__");
@@ -392,7 +406,7 @@ static Box* instanceGetattributeWithFallback(BoxedInstance* inst, BoxedString* a
 }
 
 static Box* _instanceGetattribute(Box* _inst, BoxedString* attr_str, bool raise_on_missing,
-                                  GetattrRewriteArgs* rewriter_args = NULL) {
+                                  GetattrRewriteArgs* rewrite_args = NULL) {
     RELEASE_ASSERT(_inst->cls == instance_cls, "");
     BoxedInstance* inst = static_cast<BoxedInstance*>(_inst);
 
@@ -405,7 +419,7 @@ static Box* _instanceGetattribute(Box* _inst, BoxedString* attr_str, bool raise_
             return inst->inst_cls;
     }
 
-    Box* attr = instanceGetattributeWithFallback(inst, attr_str, rewriter_args);
+    Box* attr = instanceGetattributeWithFallback(inst, attr_str, rewrite_args);
     if (attr) {
         return attr;
     } else if (!raise_on_missing) {

src/runtime/cxx_unwind.cpp

@@ -92,6 +92,7 @@ static thread_local Timer per_thread_cleanup_timer(-1);
 #ifndef NDEBUG
 static __thread bool in_cleanup_code = false;
 #endif
+static __thread bool is_unwinding = false;
 
 extern "C" {
 
@@ -567,6 +568,7 @@ static inline void unwind_loop(ExcInfo* exc_data) {
 #if STAT_TIMERS
             pyston::StatTimer::finishOverride();
 #endif
+            pyston::is_unwinding = false;
         }
         static_assert(THREADING_USE_GIL, "have to make the unwind session usage in this file thread safe!");
         // there is a python unwinding implementation detail leaked
@@ -610,6 +612,10 @@ void std::terminate() noexcept {
     RELEASE_ASSERT(0, "std::terminate() called!");
 }
 
+bool std::uncaught_exception() noexcept {
+    return pyston::is_unwinding;
+}
+
 // wrong type signature, but that's okay, it's extern "C"
 extern "C" void __gxx_personality_v0() {
     RELEASE_ASSERT(0, "__gxx_personality_v0 should never get called");
@@ -684,9 +690,13 @@ extern "C" void __cxa_throw(void* exc_obj, std::type_info* tinfo, void (*dtor)(v
     pyston::ExcInfo* exc_data = (pyston::ExcInfo*)exc_obj;
     checkExcInfo(exc_data);
 
+    ASSERT(!pyston::is_unwinding, "We don't support throwing exceptions in destructors!");
+
+    pyston::is_unwinding = true;
 #if STAT_TIMERS
     pyston::StatTimer::overrideCounter(unwinding_stattimer);
 #endif
+
     // let unwinding.cpp know we've started unwinding
     pyston::logException(exc_data);
     pyston::unwind(exc_data);

src/runtime/objmodel.h

@@ -141,8 +141,9 @@ enum LookupScope {
     INST_ONLY = 2,
     CLASS_OR_INST = 3,
 };
+struct CallattrRewriteArgs;
 template <ExceptionStyle S>
-Box* callattrInternal(Box* obj, BoxedString* attr, LookupScope, CallRewriteArgs* rewrite_args, ArgPassSpec argspec,
+Box* callattrInternal(Box* obj, BoxedString* attr, LookupScope, CallattrRewriteArgs* rewrite_args, ArgPassSpec argspec,
                       Box* arg1, Box* arg2, Box* arg3, Box** args,
                       const std::vector<BoxedString*>* keyword_names) noexcept(S == CAPI);
 extern "C" void delattr_internal(Box* obj, BoxedString* attr, bool allow_custom, DelattrRewriteArgs* rewrite_args);

src/runtime/types.cpp

@@ -908,13 +908,12 @@ static Box* typeCallInner(CallRewriteArgs* rewrite_args, ArgPassSpec argspec, Bo
         GetattrRewriteArgs grewrite_args(rewrite_args->rewriter, r_ccls, rewrite_args->destination);
         // TODO: if tp_new != Py_CallPythonNew, call that instead?
         new_attr = typeLookup(cls, new_str, &grewrite_args);
+        assert(new_attr);
 
-        if (!grewrite_args.out_success)
+        if (!grewrite_args.isSuccessful())
             rewrite_args = NULL;
         else {
-            assert(new_attr);
-            assert(grewrite_args.out_return_convention = GetattrRewriteArgs::VALID_RETURN);
-            r_new = grewrite_args.out_rtn;
+            r_new = grewrite_args.getReturn(ReturnConvention::HAS_RETURN);
             r_new->addGuard((intptr_t)new_attr);
         }
 
@@ -1052,15 +1051,14 @@ static Box* typeCallInner(CallRewriteArgs* rewrite_args, ArgPassSpec argspec, Bo
             GetattrRewriteArgs grewrite_args(rewrite_args->rewriter, r_ccls, rewrite_args->destination);
             init_attr = typeLookup(cls, init_str, &grewrite_args);
 
-            if (!grewrite_args.out_success)
+            if (!grewrite_args.isSuccessful())
                 rewrite_args = NULL;
             else {
                 if (init_attr) {
-                    assert(grewrite_args.out_return_convention = GetattrRewriteArgs::VALID_RETURN);
-                    r_init = grewrite_args.out_rtn;
+                    r_init = grewrite_args.getReturn(ReturnConvention::HAS_RETURN);
                     r_init->addGuard((intptr_t)init_attr);
                 } else {
-                    assert(grewrite_args.out_return_convention = GetattrRewriteArgs::NO_RETURN);
+                    grewrite_args.assertReturnConvention(ReturnConvention::NO_RETURN);
                 }
             }
         } else {

test/tests/binop_guarding.py

+class C(object):
+    def __add__(self, rhs):
+        if rhs == 50:
+            return NotImplemented
+        return 0
+
+    __eq__ = __add__
+
+def f():
+    c = C()
+    for i in xrange(100):
+        try:
+            print i, c + i
+        except TypeError as e:
+            print e
+f()
+
+def f2():
+    c = C()
+    for i in xrange(100):
+        try:
+            print i, c == i
+        except TypeError as e:
+            print e
+f2()